client: fixes for launching GPU detection #2713
Conversation
The function will be used in launching GPU detection.
GPU detection launches a copy of the client based on the contents of argv[0]. If argv[0] doesn't include path to the client and the current directory is not BOINC program directory then the GPU detection fails. Hard coding the client name in --detach_console code only works because of the extra work CreateProcess() does to find the executable. Launching GPU detection uses CreateProcess() in a way that it doesn't try to find the executable. Fix this by using the full path to the client executable in --detach_console code. This in turn makes the full path available to GPU detection code. This also takes care of the case of the client executable not being named boinc.exe.
GPU detection tries to launch a copy of the client using argv[0] but argv[0] doesn't always include path. If path is not included and current directory is not the same as BOINC program directory then the GPU detection fails. Use full path to the client executable instead if possible. Quote the path on Windows to handle paths with spaces.
GPU detection tries to launch a copy of the client using full path to the client executable but getting the full path may fail. In this case the code falls back to using argv[0] but argv[0] doesn't always contain path to the client. This may happen if the client was started from shell and the shell used $PATH to find the client executable. Change run_program() to search $PATH for the program file if the file name doesn't include path.
On Unixes, if GPU detection fails the client logs an error message containing a raw value from waitpid() call. This raw value generally requires writing an external program to decode it before it is useful for troubleshooting. Decode the raw value to something more useful to humans. Make a similar change to Windows code too.
GPU detection code goes back and forth between program and data directories but this is unnecessary because run_program() already takes care of running the program in correct directory. Fixes two "ignoring return value" warnings from GCC/GLIBC.
When building BOINC on MSYS+MinGW the build time environment does have /proc but the run time environment never has /proc. BSDs consider /proc to be an optional feature and, as such, /proc may be present at build time but missing at run time or vice versa. This makes checking for /proc/self/exe in configure unreliable and the check is better done at run time. get_real_executable_path() is the only user of the test result and has already been changed to do the check at run time.
|
@CharlieFenton Please test.
char path[MAXPATHLEN] = "";
int ret = get_real_executable_path(path, sizeof(path));
fprintf(stderr, "ret %d path %s\n", ret, path);
exit(0);If |
|
@JuhaSointusalo: |
On Windows, the directory that is passed to On others, the directory is used in a call to Lines 471 to 476 in 460a6db Maybe it's the I intend to review all child process launching code in BOINC to see if these bugs are made elsewhere but that's another day another PR. |
|
Has anyone tested this on Mac?
I was hoping to make the 7.14 branch today.
…On Mon, Sep 24, 2018 at 12:45 PM Juha Sointusalo ***@***.***> wrote:
However, I believe that without chdir(data_dir) after run_program() to
restore the working directory, the original instance of the client will no
longer properly access the BOINC Data directory.
On Windows, the directory that is passed to run_program() is only used in
a call to CreateProcess(). CreateProcess() shouldn't modify the parent
process in any way.
On others, the directory is used in a call to chdir() but that happens
after fork(), that is, it's changing the current working directory of the
child process.
https://github.com/BOINC/boinc/blob/460a6dba1477e222621c2f0b2fc1288d992a8466/lib/util.cpp#L471-L476
Maybe it's the return after failed chdir() that's confusing here. That's
actually a bug in run_program() and the code should call _exit() instead.
If the code returns we'll have two processes running, the parent and a
crippled copy of it.
I intend to review all child process launching code in BOINC to see if
these bugs are made elsewhere but that's another day another PR.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#2713 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AA8KgWra1okdQ4Tkoq6XYG5tTyY-DkTCks5ueTZcgaJpZM4W1VPC>
.
|
|
I logged current working directory before and after |
|
Although Windows isn't the primary concern of recent comments, the private build I made yesterday has been running since 23-Sep-2018 14:07:24 [---] Starting BOINC client version 7.13.0 for windows_x86_64 The report I made earlier on tests for the original reported problems was with a secondary instance: the primary client has been running constantly and updating files in the data directory as normal. The list of files updated since I started typing this report is client_state.xml |
|
@JuhaSointusalo: You are correct. I misread the run_program code. I now see that This log_flags message should also be removed, as it is no longer correct since you have modified the code to use an absolute path: |
@davidpanderson: I tested it and further changes are needed, as I wrote in my last comment. At the very least, the incorrect log_flags message must be removed. All references to the now unnecessary static variable client_dir should also be removed, including the entire |
|
Juha, can you please address Charlie's suggestions? |
Well, the code tries to use absolute path but that doesn't work everywhere. I know it doesn't work on OpenBSD at all because OpenBSD provides no way to find out the filename and path of the current program. It also doesn't work on NetBSD 7 if One could argue that OpenBSD doesn't support GPU crunching and never will and we don't need to care about it but I still would like to keep Sidenote: I suppose implementing |
Fair enough. Perhaps this is getting too fussy, but it would be nice if this log_flags message would appear only when |
|
@CharlieFenton Ready for re-review. |
|
@JuhaSointusalo I tested this and it works as intended. Thank you. I approve this PR for merging into master. |
Launching GPU detection has a few issues.
--detach_console$PATHThis PR fixes those issues. To support the fixes
get_real_executable_path()is implemented for several more operating systems andrun_program()is changed to use$PATH. See individual commit messages for details.Fixes #2657.