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@xxxxxxxxx>
---
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
Attachment:
signature.asc
Description: PGP signature
