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.
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().
> > + /* 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?
Luke
>
