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

[Martin Kletzander]

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.

>Signed-off-by: Luke Yue <lukedyue at gmail.com>
>---
> src/util/virfile.c | 42 ++++++------------------------------------
> 1 file changed, 6 insertions(+), 36 deletions(-)
>
>diff --git a/src/util/virfile.c b/src/util/virfile.c
>index dc2834fd1c..0d1c2ba518 100644
>--- a/src/util/virfile.c
>+++ b/src/util/virfile.c
>@@ -1662,27 +1662,14 @@ virFileIsLink(const char *linkpath)
> char *
> virFindFileInPath(const char *file)
> {
>-    const char *origpath = NULL;
>-    g_auto(GStrv) paths = NULL;
>-    char **pathiter;
>-
>     if (file == NULL)
>         return NULL;
>
>-    /* if we are passed an absolute path (starting with /), return a
>-     * copy of that path, after validating that it is executable
>-     */
>-    if (g_path_is_absolute(file)) {
>-        if (!virFileIsExecutable(file))
>-            return NULL;
>-
>-        return g_strdup(file);
>-    }
>-
>-    /* 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.

>         if (!virFileIsExecutable(file))
>@@ -1692,25 +1679,8 @@ virFindFileInPath(const char *file)
>         return abspath;
>     }
>
>-    /* copy PATH env so we can tweak it */
>-    origpath = getenv("PATH");
>-    if (!origpath)
>-        origpath = "/bin:/usr/bin";
>-
>-    /* for each path segment, append the file to search for and test for
>-     * it. return it if found.
>-     */
>-
>-    if (!(paths = g_strsplit(origpath, ":", 0)))
>-        return NULL;
>-
>-    for (pathiter = paths; *pathiter; pathiter++) {
>-        g_autofree char *fullpath = g_build_filename(*pathiter, file, NULL);
>-        if (virFileIsExecutable(fullpath))
>-            return g_steal_pointer(&fullpath);
>-    }
>-
>-    return NULL;
>+    /* 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.

> }
>
>
>-- 
>2.31.1
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20210604/1c7f0029/attachment-0001.sig>