Re: [PATCH 2/3] virfile: Simplify virFindFileInPath() with g_find_program_in_path()

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 >