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

[Luke Yue]

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
>