Re: [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 <luked...@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

signature.asc
Description: PGP signature