8:48 p.m.
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
8:48 p.m.
New subject: [PATCH 1/3] virfile: Use g_build_filename() when building paths
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(a)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
5:01 a.m.
New subject: [PATCH 1/3] virfile: Use g_build_filename() when building paths
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(a)redhat.com>
8:48 p.m.
New subject: [PATCH 2/3] virfile: Simplify virFindFileInPath() with g_find_program_in_path()
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(a)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
5:26 a.m.
New subject: [PATCH 2/3] virfile: Simplify virFindFileInPath() with g_find_program_in_path()
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(a)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
6:29 a.m.
New subject: [PATCH 2/3] virfile: Simplify virFindFileInPath() with g_find_program_in_path()
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
7:39 a.m.
New subject: [PATCH 2/3] virfile: Simplify virFindFileInPath() with g_find_program_in_path()
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
>
8:48 p.m.
New subject: [PATCH 3/3] virfile: Use g_canonicalize_file() to simplify virFileAbsPath()
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(a)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
5:28 a.m.
New subject: [PATCH 3/3] virfile: Use g_canonicalize_file() to simplify virFileAbsPath()
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(a)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
6:38 a.m.
New subject: [PATCH 3/3] virfile: Use g_canonicalize_file() to simplify virFileAbsPath()
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
7:41 a.m.
New subject: [PATCH 3/3] virfile: Use g_canonicalize_file() to simplify virFileAbsPath()
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
1302
days inactive
1306
days old
10 comments
2 participants
participants (2)
-
Luke Yue -
Martin Kletzander