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

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.

> +    /* 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.