[PATCH v2 0/2] Replace some libvirt handling function with GLib APIs
Compared to original: - Ensure the virFindFileInPath() return an absolute path (tried to use glibcompat, failed to solve some Windows related issue) - Just remove virFileAbsPath() and use g_canonicalize_filename() instead Luke Yue (2): Replace virFileAbsPath() with g_canonicalize_filename() virfile: Simplify virFindFileInPath() with g_find_program_in_path() src/libvirt-domain.c | 16 +++++----- src/libvirt_private.syms | 1 - src/util/virfile.c | 69 ++-------------------------------------- src/util/virfile.h | 3 -- src/util/virlog.c | 2 +- 5 files changed, 12 insertions(+), 79 deletions(-) -- 2.31.1
Signed-off-by: Luke Yue <lukedyue@gmail.com> --- src/libvirt-domain.c | 16 ++++++++-------- src/libvirt_private.syms | 1 - src/util/virfile.c | 23 +---------------------- src/util/virfile.h | 3 --- src/util/virlog.c | 2 +- 5 files changed, 10 insertions(+), 35 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 42c75f6cc5..750e32f0ca 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -827,7 +827,7 @@ virDomainSave(virDomainPtr domain, const char *to) char *absolute_to; /* We must absolutize the file path as the save is done out of process */ - if (virFileAbsPath(to, &absolute_to) < 0) { + if (!(absolute_to = g_canonicalize_filename(to, NULL))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("could not build absolute output file path")); goto error; @@ -915,7 +915,7 @@ virDomainSaveFlags(virDomainPtr domain, const char *to, char *absolute_to; /* We must absolutize the file path as the save is done out of process */ - if (virFileAbsPath(to, &absolute_to) < 0) { + if (!(absolute_to = g_canonicalize_filename(to, NULL))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("could not build absolute output file path")); goto error; @@ -965,7 +965,7 @@ virDomainRestore(virConnectPtr conn, const char *from) char *absolute_from; /* We must absolutize the file path as the restore is done out of process */ - if (virFileAbsPath(from, &absolute_from) < 0) { + if (!(absolute_from = g_canonicalize_filename(from, NULL))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("could not build absolute input file path")); goto error; @@ -1039,7 +1039,7 @@ virDomainRestoreFlags(virConnectPtr conn, const char *from, const char *dxml, char *absolute_from; /* We must absolutize the file path as the restore is done out of process */ - if (virFileAbsPath(from, &absolute_from) < 0) { + if (!(absolute_from = g_canonicalize_filename(from, NULL))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("could not build absolute input file path")); goto error; @@ -1097,7 +1097,7 @@ virDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *file, char *absolute_file; /* We must absolutize the file path as the read is done out of process */ - if (virFileAbsPath(file, &absolute_file) < 0) { + if (!(absolute_file = g_canonicalize_filename(file, NULL))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("could not build absolute input file path")); goto error; @@ -1170,7 +1170,7 @@ virDomainSaveImageDefineXML(virConnectPtr conn, const char *file, char *absolute_file; /* We must absolutize the file path as the read is done out of process */ - if (virFileAbsPath(file, &absolute_file) < 0) { + if (!(absolute_file = g_canonicalize_filename(file, NULL))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("could not build absolute input file path")); goto error; @@ -1245,7 +1245,7 @@ virDomainCoreDump(virDomainPtr domain, const char *to, unsigned int flags) char *absolute_to; /* We must absolutize the file path as the save is done out of process */ - if (virFileAbsPath(to, &absolute_to) < 0) { + if (!(absolute_to = g_canonicalize_filename(to, NULL))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("could not build absolute core file path")); goto error; @@ -1329,7 +1329,7 @@ virDomainCoreDumpWithFormat(virDomainPtr domain, const char *to, char *absolute_to; /* We must absolutize the file path as the save is done out of process */ - if (virFileAbsPath(to, &absolute_to) < 0) { + if (!(absolute_to = g_canonicalize_filename(to, NULL))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("could not build absolute core file path")); goto error; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0ced2a7990..e4b2d386f7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2176,7 +2176,6 @@ virDirOpen; virDirOpenIfExists; virDirOpenQuiet; virDirRead; -virFileAbsPath; virFileAccessibleAs; virFileActivateDirOverrideForLib; virFileActivateDirOverrideForProg; diff --git a/src/util/virfile.c b/src/util/virfile.c index f32f3e37e4..7fe357ab16 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1688,7 +1688,7 @@ virFindFileInPath(const char *file) if (!virFileIsExecutable(file)) return NULL; - ignore_value(virFileAbsPath(file, &abspath)); + ignore_value(abspath = g_canonicalize_filename(file, NULL)); return abspath; } @@ -3146,27 +3146,6 @@ virFileOpenTty(int *ttyprimary G_GNUC_UNUSED, } #endif /* WIN32 */ -/* - * Creates an absolute path for a potentially relative path. - * Return 0 if the path was not relative, or on success. - * Return -1 on error. - * - * You must free the result. - */ -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); - } - - return 0; -} - /* Remove spurious / characters from a path. The result must be freed */ char * virFileSanitizePath(const char *path) diff --git a/src/util/virfile.h b/src/util/virfile.h index b9f7b1766f..72368495bf 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -283,9 +283,6 @@ char *virFileBuildPath(const char *dir, const char *name, const char *ext) G_GNUC_WARN_UNUSED_RESULT; - -int virFileAbsPath(const char *path, - char **abspath) G_GNUC_WARN_UNUSED_RESULT; void virFileRemoveLastComponent(char *path); int virFileOpenTty(int *ttymaster, diff --git a/src/util/virlog.c b/src/util/virlog.c index 78be1993cd..3ad043d98a 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1512,7 +1512,7 @@ virLogParseOutput(const char *src) #endif break; case VIR_LOG_TO_FILE: - if (virFileAbsPath(tokens[2], &abspath) < 0) + if (!(abspath = g_canonicalize_filename(tokens[2], NULL))) return NULL; ret = virLogNewOutputToFile(prio, abspath); VIR_FREE(abspath); -- 2.31.1
On Mon, Jun 07, 2021 at 02:10:47PM +0800, Luke Yue wrote:
Signed-off-by: Luke Yue <lukedyue@gmail.com> --- src/libvirt-domain.c | 16 ++++++++-------- src/libvirt_private.syms | 1 - src/util/virfile.c | 23 +---------------------- src/util/virfile.h | 3 --- src/util/virlog.c | 2 +- 5 files changed, 10 insertions(+), 35 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 42c75f6cc5..750e32f0ca 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -827,7 +827,7 @@ virDomainSave(virDomainPtr domain, const char *to) char *absolute_to;
/* We must absolutize the file path as the save is done out of process */ - if (virFileAbsPath(to, &absolute_to) < 0) { + if (!(absolute_to = g_canonicalize_filename(to, NULL))) {
I must say I'm not a fan of this "over-encapsulation" and I prefer the more-readable: var = func(args); if (!var) { ... but I know this is very subjective *and* we do not have a rule for it *and* if we had a rule it would be based on other contributors' preferences which I think actually prefer the way you did it. So this is fine, just a tiny rant to make me feel better ;) [...]
diff --git a/src/util/virfile.c b/src/util/virfile.c index f32f3e37e4..7fe357ab16 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1688,7 +1688,7 @@ virFindFileInPath(const char *file) if (!virFileIsExecutable(file)) return NULL;
- ignore_value(virFileAbsPath(file, &abspath)); + ignore_value(abspath = g_canonicalize_filename(file, NULL)); return abspath;
ignore_value here does not make sense after the change. And if you look at it, you can just return the result of g_canonicalize_filename() here directly. Reviewed-by: Martin Kletzander <mkletzan@redhat.com> I'll fix up this one and push it later.
Signed-off-by: Luke Yue <lukedyue@gmail.com> --- src/util/virfile.c | 48 +++------------------------------------------- 1 file changed, 3 insertions(+), 45 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 7fe357ab16..14b45f4e1b 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1662,55 +1662,13 @@ virFileIsLink(const char *linkpath) char * virFindFileInPath(const char *file) { - const char *origpath = NULL; - g_auto(GStrv) paths = NULL; - char **pathiter; - + g_autofree char *path = NULL; 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 (strchr(file, '/')) { - char *abspath = NULL; - - if (!virFileIsExecutable(file)) - return NULL; - - ignore_value(abspath = g_canonicalize_filename(file, NULL)); - return abspath; - } + path = g_find_program_in_path(file); - /* 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; + return g_canonicalize_filename(path, NULL); } -- 2.31.1
On Mon, Jun 07, 2021 at 02:10:48PM +0800, Luke Yue wrote:
Signed-off-by: Luke Yue <lukedyue@gmail.com> --- src/util/virfile.c | 48 +++------------------------------------------- 1 file changed, 3 insertions(+), 45 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c index 7fe357ab16..14b45f4e1b 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1662,55 +1662,13 @@ virFileIsLink(const char *linkpath) char * virFindFileInPath(const char *file) { - const char *origpath = NULL; - g_auto(GStrv) paths = NULL; - char **pathiter; - + g_autofree char *path = NULL; 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 (strchr(file, '/')) { - char *abspath = NULL; - - if (!virFileIsExecutable(file)) - return NULL; - - ignore_value(abspath = g_canonicalize_filename(file, NULL)); - return abspath;
Oh, I see my fixup does not need to be made here since it is removed anyway. It is not completely pointless to have the code clean even between commits, although you already know and do that, but just to point out it can sometimes happen that the second patch needs to be reverted in which case the code would end up not as clean as possible, but that is very rarely the case and if that happens it is usually with more complex code I imagine. So no big deal here.
- } + path = g_find_program_in_path(file);
- /* 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; + return g_canonicalize_filename(path, NULL);
Since you did not go with the glibcompat approach, it would be nice to add a comment here describing why this is done as we might be baffled by this in a couple of years when glib fix for the g_find_program_in_path() reaches all distros. I'll mention it here before pushing. Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
}
-- 2.31.1
participants (2)
-
Luke Yue -
Martin Kletzander