On Fri, 2021-06-04 at 12:26 +0200, Martin Kletzander wrote:
On Mon, May 31, 2021 at 09:48:23AM +0800, Luke Yue wrote:
The behavior is a little bit different when using g_find_program_in_path(), when the `file` contains a relative path, the original function would return a absolute path, but the g_find_program_in_path() would probably return a relative one.
Other conditions would be fine, so just use g_find_program_in_path() to simplify the implementation. Note that when PATH is not set, g_find_program_in_path() will search in `/bin:/usr/bin:.`.
That is the main issue I see with this patch. Before we were searching in PATH or, if unset, `/bin:/usr/bin`, but not the current directory.
I am a bit worried about that, but since that is the same way execvp() would search for the binary I guess that's fine.
There is one thing that we should keep, though, and that is the fact that the function returns an absolute path as there might be (and I believe there is) a caller that depends on it.
- /* If we are passed an anchored path (containing a /), then there - * is no path search - it must exist in the current directory + /* If we are passed an anchored path (containing a /), and it's not an + * absolute path then there is no path search - it must exist in the current + * directory */ - if (strchr(file, '/')) { + if (!g_path_is_absolute(file) && strchr(file, '/')) { char *abspath = NULL;
This is also already handled by g_find_program_in_path() so it can be removed.
Thanks for the review! As the GLib doc says, g_find_program_in_path() will return a newly- allocated string with the absolute path, or NULL. And the problem here is that the g_find_program_in_path() would return a relative path, that's an unexpected behavior. For example, if we pass `../bin/sh` to the function, we will get `../bin/sh` as return value, but what we want is `/bin/sh` or `/tmp/../bin/sh`, so I left the code here. I fixed this issue in this PR https://gitlab.gnome.org/GNOME/glib/-/merge_requests/2127 we will wait for a long time until the fix lands on most machines, so I decide to left the codes here. Or as you said below, we can wrap the result in virFileAbsPath().
+ /* Otherwise, just use g_find_program_in_path() */ + return g_find_program_in_path(file);
And wrap the result in virFileAbsPath() so that it keeps that functionality. Otherwise it would just be a wrapper for g_find_program_in_path() and could be removed altogether.
So I will wrap the result in virFileAbsPath() (or g_canonicalize_filename()) and remove the left code above. And I guess in the future when the fix above lands on most machines, remove the function and use g_find_program_in_path() instead would be fine? Luke