Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move random_int() to util.cpp #5555

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

computezrmle
Copy link
Contributor

random_int() can easily be used as seed for srand() from various functions if it is in util.cpp.
This is not possible if it is in crypt_prog.cpp since crypt_prog.cpp is not included in the standard build process.
Instead, tools/makefile_sign_executable must be used to build it.

See:
https://boinc.berkeley.edu/trac/wiki/KeySetup#Filesigningutilities

@AenBleidd
Copy link
Member

@computezrmle, please fix the failed build.
Thank you

@computezrmle
Copy link
Contributor Author

As for Release-x64-msbuild
2024-03-26T21:24:45.8536712Z D:\a\boinc\boinc\win_build\vcpkg_3rdparty_dependencies.vcxproj(182,5): error MSB3073: The command "vcpkg.exe install --x-manifest-root=D:\a\boinc\boinc\win_build\..\3rdParty\vcpkg_ports\configs\msbuild/x64 --x-install-root=D:\a\boinc\boinc\win_build\..\3rdParty\Windows\vcpkg/installed/ --overlay-ports=../../vcpkg_ports/ports --overlay-triplets=../../vcpkg_ports/triplets/ci --triplet x64-windows-static --clean-after-build" exited with code -1.
Is it caused by a missing entry in win_build\vcpkg_3rdparty_dependencies.vcxproj?
If so, I would need a hint since I'm not familiar with the syntax.

As for Release-ARM64-msbuild
2024-03-26T21:25:11.3790575Z D:\a\boinc\boinc\lib\util.cpp(552,18): error C2664: 'HINSTANCE LoadLibrary(LPCWSTR)': cannot convert argument 1 from 'const char [13]' to 'LPCWSTR' [D:\a\boinc\boinc\win_build\boincmgr.vcxproj]
Could this be a multi-byte/unicode issue?
If so, could it be solved using LoadLibrary(L"ADVAPI32.DLL")?

lib/util.cpp Outdated Show resolved Hide resolved
lib/util.cpp Outdated Show resolved Hide resolved
lib/util.cpp Outdated Show resolved Hide resolved
@AenBleidd
Copy link
Member

Is it caused by a missing entry in win_build\vcpkg_3rdparty_dependencies.vcxproj?

This was an unrelated issue, now fixed by build rerun.
Others are still here, so please fix them

Copy link
Member

@AenBleidd AenBleidd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still two failing builds.
Please check them and fix.
Thank you in advance.

if (!hLib) {
fprintf(stderr, "Error: can't load ADVAPI32.DLL\n");
}
BOOLEAN (APIENTRY *pfn)(void*, ULONG) =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the library was not loaded correctly two lines above - this will fail with an exception

lib/util.cpp Outdated
}
fclose(f);
#endif
return n;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will be returned in case of an error?
We should indicate somehow that the function returns a proper result.

lib/util.cpp Outdated
@@ -542,3 +542,38 @@ bool process_exists(int pid) {
}

#endif

unsigned int random_int() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to change signature function to something like this:
bool random_int(unsigned int& value)
And return the random value using the output parameter, and return true on success and false in case of an error

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, then you need to adjust crypt_prog.cpp with the new function signature

@davidpanderson
Copy link
Contributor

Is there a need for this outside of crypt_prog?
You can call srand() with, e.g. the time of day in microseconds.

@computezrmle
Copy link
Contributor Author

Can the recent build issues be solved without too much effort?

1st idea to get a random number was to use the standard library functions from C++11.
Looks like certain C++11 functions (including those) don't work for BOINC.

Next idea was to use rand() which needs srand(seed).
Since seed is expected to be an unsigned int random_int() from crypt_prog seemed to be a good idea as

  • it is available in the source tree
  • it delivers an unsigned int
  • it uses a "natural" random source like /dev/random.

As random_int() has build issues for the client when used from crypt_prog the next idea was to move it to lib/util which would make it globally available.
The build issues for the server it has now came unexpected.
If they can be solved, why not leave random_int() in lib/util?

Other sources for seed appear to be less "natural".
A seed base on daytime may deliver a good seed if it is not too coarse.

Can you confirm that dtime() from lib/util returns a double like 86345.123456?
I'm sure it does on Linux.
What about Windows or GCL_SIMULATOR?

If yes, this may be fine enough to cover concurrently starting processes:
seed = (unsigned int)(100000 * dtime())

or, to avoid seed becomes greater than 32-bit max unsigned int:
seed = (unsigned int)(1000 * dtime())
This would give a granularity of 0.1 ms.

lib/util.cpp Outdated
}
FreeLibrary(hLib);
#else
FILE* f = fopen("/dev/random", "r");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
FILE* f = fopen("/dev/random", "r");
FILE* f = boinc::fopen("/dev/random", "r");

lib/util.cpp Outdated
if (!f) {
return 2;
}
if (1 != fread(&n, sizeof(n), 1, f)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (1 != fread(&n, sizeof(n), 1, f)) {
if (1 != boinc::fread(&n, sizeof(n), 1, f)) {

lib/util.cpp Outdated
if (1 != fread(&n, sizeof(n), 1, f)) {
return 3;
}
fclose(f);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fclose(f);
boinc::fclose(f);

die("random_int");
}
srand(srand_seed);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

run locally this command to remove trailing spaces:
python3 ci_tools/trailing_whitespaces_check.py . --fix

@AenBleidd
Copy link
Member

Is there a need for this outside of crypt_prog? You can call srand() with, e.g. the time of day in microseconds.

I think, having this as a generally available function won't hurt and could be helpful.

In addition:
Added 'return 0' at the end of 'random_int()' to mitigate a runtime error.
Copy link

codecov bot commented Mar 28, 2024

Codecov Report

Attention: Patch coverage is 0% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 10.79%. Comparing base (b2af307) to head (d60cab9).
Report is 2 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #5555      +/-   ##
============================================
- Coverage     10.80%   10.79%   -0.01%     
  Complexity     1068     1068              
============================================
  Files           279      279              
  Lines         36294    36303       +9     
  Branches       8409     8412       +3     
============================================
  Hits           3920     3920              
- Misses        31980    31989       +9     
  Partials        394      394              
Files Coverage Δ
lib/util.h 40.00% <ø> (ø)
lib/util.cpp 32.50% <0.00%> (-2.33%) ⬇️
lib/crypt_prog.cpp 0.00% <0.00%> (ø)

Copy link
Member

@AenBleidd AenBleidd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.
@davidpanderson, if you are ok with this - feel free to merge it

@davidpanderson
Copy link
Contributor

Sorry, but I don't see a use case for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Reviewer approved
Development

Successfully merging this pull request may close these issues.

3 participants