[PATCH 0/3] Replace some libvirt handling functions with GLib APIs
Note that the g_find_program_in_path() will search file in current dir when PATH env is not set, while virFindFileInPath() won't. The virFileAbsPath() could be replaced by g_canonicalize_file() when not take the return value into account, and it just simply return 0 now, so maybe remove the function is a better choice? Related issue: https://gitlab.com/libvirt/libvirt/-/issues/12 Luke Yue (3): virfile: Use g_build_filename() when building paths virfile: Simplify virFindFileInPath() with g_find_program_in_path() virfile: Use g_canonicalize_file() to simplify virFileAbsPath() src/util/virfile.c | 71 +++++++++++++--------------------------------- 1 file changed, 19 insertions(+), 52 deletions(-) -- 2.31.1
The g_build_filename() would decide which separator to use instead of hardcoding in g_strdup_printf(). Related issue: https://gitlab.com/libvirt/libvirt/-/issues/12 Signed-off-by: Luke Yue <lukedyue@gmail.com> --- src/util/virfile.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 03a7725dd3..dc2834fd1c 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -733,7 +733,7 @@ static int virFileLoopDeviceOpenSearch(char **dev_name) !g_ascii_isdigit(de->d_name[4])) continue; - looppath = g_strdup_printf("/dev/%s", de->d_name); + looppath = g_build_filename("/dev", de->d_name, NULL); VIR_DEBUG("Checking up on device %s", looppath); if ((fd = open(looppath, O_RDWR)) < 0) { @@ -860,7 +860,7 @@ virFileNBDDeviceIsBusy(const char *dev_name) { g_autofree char *path = NULL; - path = g_strdup_printf(SYSFS_BLOCK_DIR "/%s/pid", dev_name); + path = g_build_filename(SYSFS_BLOCK_DIR, dev_name, "pid", NULL); if (!virFileExists(path)) { if (errno == ENOENT) @@ -893,7 +893,7 @@ virFileNBDDeviceFindUnused(void) return NULL; if (rv == 0) - return g_strdup_printf("/dev/%s", de->d_name); + return g_build_filename("/dev", de->d_name, NULL); } } if (direrr < 0) @@ -1028,7 +1028,7 @@ int virFileDeleteTree(const char *dir) g_autofree char *filepath = NULL; GStatBuf sb; - filepath = g_strdup_printf("%s/%s", dir, de->d_name); + filepath = g_build_filename(dir, de->d_name, NULL); if (g_lstat(filepath, &sb) < 0) { virReportSystemError(errno, _("Cannot access '%s'"), @@ -1568,7 +1568,7 @@ virFileRelLinkPointsTo(const char *directory, checkLink); return -1; } - candidate = g_strdup_printf("%s/%s", directory, checkLink); + candidate = g_build_filename(directory, checkLink, NULL); return virFileLinkPointsTo(candidate, checkDest); } @@ -1705,7 +1705,7 @@ virFindFileInPath(const char *file) return NULL; for (pathiter = paths; *pathiter; pathiter++) { - g_autofree char *fullpath = g_strdup_printf("%s/%s", *pathiter, file); + g_autofree char *fullpath = g_build_filename(*pathiter, file, NULL); if (virFileIsExecutable(fullpath)) return g_steal_pointer(&fullpath); } @@ -1754,6 +1754,7 @@ virFileFindResourceFull(const char *filename, char *ret = NULL; const char *envval = envname ? getenv(envname) : NULL; const char *path; + g_autofree char *fullFilename = NULL; if (!prefix) prefix = ""; @@ -1767,7 +1768,8 @@ virFileFindResourceFull(const char *filename, else path = installdir; - ret = g_strdup_printf("%s/%s%s%s", path, prefix, filename, suffix); + fullFilename = g_strdup_printf("%s%s%s", prefix, filename, suffix); + ret = g_build_filename(path, fullFilename, NULL); VIR_DEBUG("Resolved '%s' to '%s'", filename, ret); return ret; @@ -2986,7 +2988,7 @@ int virFileChownFiles(const char *name, while ((direrr = virDirRead(dir, &ent, name)) > 0) { g_autofree char *path = NULL; - path = g_strdup_printf("%s/%s", name, ent->d_name); + path = g_build_filename(name, ent->d_name, NULL); if (!virFileIsRegular(path)) continue; @@ -3048,9 +3050,10 @@ virFileBuildPath(const char *dir, const char *name, const char *ext) char *path; if (ext == NULL) { - path = g_strdup_printf("%s/%s", dir, name); + path = g_build_filename(dir, name, NULL); } else { - path = g_strdup_printf("%s/%s%s", dir, name, ext); + g_autofree char *extName = g_strdup_printf("%s%s", name, ext); + path = g_build_filename(dir, extName, NULL); } return path; @@ -3158,7 +3161,7 @@ virFileAbsPath(const char *path, char **abspath) } else { g_autofree char *buf = g_get_current_dir(); - *abspath = g_strdup_printf("%s/%s", buf, path); + *abspath = g_build_filename(buf, path, NULL); } return 0; -- 2.31.1
On Mon, May 31, 2021 at 09:48:22AM +0800, Luke Yue wrote:
The g_build_filename() would decide which separator to use instead of hardcoding in g_strdup_printf().
Even though most of these are not going to be used on that wide range of platforms, it is still nice to be consistent and future-proof. Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
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:.`. Signed-off-by: Luke Yue <lukedyue@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; 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); } -- 2.31.1
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@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
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
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
Though the comment says that the function may return -1 on error, but it seems that now it will never return -1 now. So just use g_canonicalize_file() to simplify the implementation. Signed-off-by: Luke Yue <lukedyue@gmail.com> --- src/util/virfile.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 0d1c2ba518..bfff471194 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -3126,13 +3126,7 @@ virFileOpenTty(int *ttyprimary G_GNUC_UNUSED, int virFileAbsPath(const char *path, char **abspath) { - if (g_path_is_absolute(path)) { - *abspath = g_strdup(path); - } else { - g_autofree char *buf = g_get_current_dir(); - - *abspath = g_build_filename(buf, path, NULL); - } + *abspath = g_canonicalize_filename(path, NULL); return 0; } -- 2.31.1
On Mon, May 31, 2021 at 09:48:24AM +0800, Luke Yue wrote:
Though the comment says that the function may return -1 on error, but it seems that now it will never return -1 now. So just use g_canonicalize_file() to simplify the implementation.
Yeah, that is a leftover from before we started using glib and abort()'ing on OOM. It would be nice if that leftover got cleaned up as well. However, looking at it, we can remove the function altogether and just use the glib counterpart. That'd be even more of a clean up ;)
Signed-off-by: Luke Yue <lukedyue@gmail.com> --- src/util/virfile.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c index 0d1c2ba518..bfff471194 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -3126,13 +3126,7 @@ virFileOpenTty(int *ttyprimary G_GNUC_UNUSED, int virFileAbsPath(const char *path, char **abspath) { - if (g_path_is_absolute(path)) { - *abspath = g_strdup(path); - } else { - g_autofree char *buf = g_get_current_dir(); - - *abspath = g_build_filename(buf, path, NULL); - } + *abspath = g_canonicalize_filename(path, NULL);
return 0; } -- 2.31.1
On Fri, 2021-06-04 at 12:28 +0200, Martin Kletzander wrote:
On Mon, May 31, 2021 at 09:48:24AM +0800, Luke Yue wrote:
Though the comment says that the function may return -1 on error, but it seems that now it will never return -1 now. So just use g_canonicalize_file() to simplify the implementation.
Yeah, that is a leftover from before we started using glib and abort()'ing on OOM. It would be nice if that leftover got cleaned up as well. However, looking at it, we can remove the function altogether and just use the glib counterpart. That'd be even more of a clean up ;)
Thanks for the review! I will try to replace all the funciton calls with g_canonicalize_file() and remove the function in a new patch. Luke
On Fri, Jun 04, 2021 at 07:38:42PM +0800, Luke Yue wrote:
On Fri, 2021-06-04 at 12:28 +0200, Martin Kletzander wrote:
On Mon, May 31, 2021 at 09:48:24AM +0800, Luke Yue wrote:
Though the comment says that the function may return -1 on error, but it seems that now it will never return -1 now. So just use g_canonicalize_file() to simplify the implementation.
Yeah, that is a leftover from before we started using glib and abort()'ing on OOM. It would be nice if that leftover got cleaned up as well. However, looking at it, we can remove the function altogether and just use the glib counterpart. That'd be even more of a clean up ;)
Thanks for the review!
I will try to replace all the funciton calls with g_canonicalize_file() and remove the function in a new patch.
Thanks. In the meantime I pushed the PATCH 1/3.
Luke
participants (2)
-
Luke Yue -
Martin Kletzander