On Fri, Jun 04, 2021 at 07:29:36PM +0800, Luke Yue wrote:
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.
I missed that, and I read it like 4 times =D
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().
Nice!!!
> + /* 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?
That's one of the reasons why there is src/util/glibcompat.c which handles cases similar to this. You can wrap the behaviour around a condition based on the version that will include the fix.
Luke
Attachment:
signature.asc
Description: PGP signature
