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

[Martin Kletzander]

On Mon, Jun 07, 2021 at 02:10:48PM +0800, Luke Yue wrote:
>Signed-off-by: Luke Yue <lukedyue at gmail.com>
>---
> src/util/virfile.c | 48 +++-------------------------------------------
> 1 file changed, 3 insertions(+), 45 deletions(-)
>
>diff --git a/src/util/virfile.c b/src/util/virfile.c
>index 7fe357ab16..14b45f4e1b 100644
>--- a/src/util/virfile.c
>+++ b/src/util/virfile.c
>@@ -1662,55 +1662,13 @@ virFileIsLink(const char *linkpath)
> char *
> virFindFileInPath(const char *file)
> {
>-    const char *origpath = NULL;
>-    g_auto(GStrv) paths = NULL;
>-    char **pathiter;
>-
>+    g_autofree char *path = NULL;
>     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 (strchr(file, '/')) {
>-        char *abspath = NULL;
>-
>-        if (!virFileIsExecutable(file))
>-            return NULL;
>-
>-        ignore_value(abspath = g_canonicalize_filename(file, NULL));
>-        return abspath;

Oh, I see my fixup does not need to be made here since it is removed
anyway.  It is not completely pointless to have the code clean even
between commits, although you already know and do that, but just to
point out it can sometimes happen that the second patch needs to be
reverted in which case the code would end up not as clean as possible,
but that is very rarely the case and if that happens it is usually with
more complex code I imagine.  So no big deal here.

>-    }
>+    path = g_find_program_in_path(file);
>
>-    /* 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;
>+    return g_canonicalize_filename(path, NULL);

Since you did not go with the glibcompat approach, it would be nice to
add a comment here describing why this is done as we might be baffled by
this in a couple of years when glib fix for the g_find_program_in_path()
reaches all distros.

I'll mention it here before pushing.

Reviewed-by: Martin Kletzander <mkletzan at redhat.com>

> }
>
>
>-- 
>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/20210615/e47a5078/attachment-0001.sig>