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

[Martin Kletzander]

On Fri, Jun 04, 2021 at 07:29:36PM +0800, Luke Yue wrote:
>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. 
>

I missed that, and I read it like 4 times =D

>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().
>

Nice!!!

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

That's one of the reasons why there is src/util/glibcompat.c which
handles cases similar to this.  You can wrap the behaviour around a
condition based on the version that will include the fix.

>Luke
>>
>
>
-------------- 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/30b5f6b4/attachment-0001.sig>