[PATCH 0/3] util: Fix virFileIsSharedFSOverride on nonexistent paths
Jiri Denemark (3): util: Document limitation of virFileCanonicalizePath util: Introduce virFileGetExistingParent helper util: Fix virFileIsSharedFSOverride on nonexistent paths src/util/virfile.c | 76 +++++++++++++++++++++++++++------------------- 1 file changed, 45 insertions(+), 31 deletions(-) -- 2.49.0
From: Jiri Denemark <jdenemar@redhat.com> On most platforms virFileCanonicalizePath is implemented using realpath(), which only works on existing paths. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/util/virfile.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 7cab3d0cd6..242aa7ff88 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -3408,7 +3408,8 @@ virFileSanitizePath(const char *path) /** * virFileCanonicalizePath: * - * Returns the canonical representation of @path. + * Returns the canonical representation of @path. This function is only + * guaranteed to work when @path exists. It may return NULL otherwise. * * The returned string must be freed after use. */ -- 2.49.0
From: Jiri Denemark <jdenemar@redhat.com> The code from virFileIsSharedFSType which finds the longest existing path for a given input is separated into a new helper so that it can be reused elsewhere. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: If you wonder why the code is almost the same as the original one, but not exactly the same, it's because I rewrote the code using stat(), but than realized that mocking it for our tests would be a nightmare. So I replaced stat() with statfs() :-) src/util/virfile.c | 60 ++++++++++++++++++++++++---------------------- 1 file changed, 31 insertions(+), 29 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 242aa7ff88..ef1bc0bbf3 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -3557,6 +3557,34 @@ virFileIsSharedFsFUSE(const char *path, } +static char * +virFileGetExistingParent(const char *path) +{ + g_autofree char *dirpath = g_strdup(path); + struct statfs sb; + char *p = NULL; + + /* Try less and less of the path until we get to a directory we can stat. + * Even if we don't have 'x' permission on any directory in the path on the + * NFS server (assuming it's NFS), we will be able to stat the mount point. + */ + while (statfs(dirpath, &sb) < 0 && p != dirpath) { + if (!(p = strrchr(dirpath, '/'))) { + virReportSystemError(EINVAL, + _("Invalid relative path '%1$s'"), path); + return NULL; + } + + if (p == dirpath) + *(p + 1) = '\0'; + else + *p = '\0'; + } + + return g_steal_pointer(&dirpath); +} + + static const struct virFileSharedFsData virFileSharedFs[] = { { .fstype = VIR_FILE_SHFS_NFS, .magic = NFS_SUPER_MAGIC }, { .fstype = VIR_FILE_SHFS_GFS2, .magic = GFS2_MAGIC }, @@ -3576,40 +3604,14 @@ virFileIsSharedFSType(const char *path, unsigned int fstypes) { g_autofree char *dirpath = NULL; - char *p = NULL; struct statfs sb; - int statfs_ret; long long f_type = 0; size_t i; - dirpath = g_strdup(path); + if (!(dirpath = virFileGetExistingParent(path))) + return -1; - statfs_ret = statfs(dirpath, &sb); - - while ((statfs_ret < 0) && (p != dirpath)) { - /* Try less and less of the path until we get to a - * directory we can stat. Even if we don't have 'x' - * permission on any directory in the path on the NFS - * server (assuming it's NFS), we will be able to stat the - * mount point, and that will properly tell us if the - * fstype is NFS. - */ - - if ((p = strrchr(dirpath, '/')) == NULL) { - virReportSystemError(EINVAL, - _("Invalid relative path '%1$s'"), path); - return -1; - } - - if (p == dirpath) - *(p+1) = '\0'; - else - *p = '\0'; - - statfs_ret = statfs(dirpath, &sb); - } - - if (statfs_ret < 0) { + if (statfs(dirpath, &sb) < 0) { virReportSystemError(errno, _("cannot determine filesystem for '%1$s'"), path); -- 2.49.0
From: Jiri Denemark <jdenemar@redhat.com> Commit v11.0.0-162-gf2023e8018 added path canonicalization to virFileIsSharedFSOverride to make sure we can properly match shared filesystem override paths which include symlinks. But virFileCanonicalizePath only works on existing paths, while virFileIsSharedFSOverride may be called on paths that do not exist yet. Matching paths against overrides has always worked even for nonexistent paths. To fix the regression we need to first get the longest existing sub-path and canonicalize only this part and use the result for searching in overrides. Clearly any portion of the path we dynamically create is not going to end up on a different filesystem to what the parent directory is stored in. So checking just the existing parent is enough. https://issues.redhat.com/browse/RHEL-86592 Fixes: f2023e8018fe18550ad6aec66fe72bd1376f8522 Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/util/virfile.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index ef1bc0bbf3..06e8e08ddc 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -3821,15 +3821,26 @@ virFileIsSharedFSOverride(const char *path, char *const *overrides) { g_autofree char *dirpath = NULL; + g_autofree char *existing = NULL; char *p = NULL; if (!path || path[0] != '/' || !overrides) return false; + /* We only care about the longest existing sub-path. Further components + * may will later be created by libvirt will not magically become a shared + * filesystem. */ + if (!(existing = virFileGetExistingParent(path))) + return false; + /* Overrides have been canonicalized ahead of time, so we need to * do the same for the provided path or we'll never be able to * find a match if symlinks are involved */ - dirpath = virFileCanonicalizePath(path); + if (!(dirpath = virFileCanonicalizePath(existing))) { + VIR_DEBUG("Cannot canonicalize parent '%s' of path '%s'", + existing, path); + return false; + } if (g_strv_contains((const char *const *) overrides, dirpath)) return true; -- 2.49.0
On a Thursday in 2025, Jiri Denemark via Devel wrote:
Jiri Denemark (3): util: Document limitation of virFileCanonicalizePath util: Introduce virFileGetExistingParent helper util: Fix virFileIsSharedFSOverride on nonexistent paths
src/util/virfile.c | 76 +++++++++++++++++++++++++++------------------- 1 file changed, 45 insertions(+), 31 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Jiri Denemark -
Ján Tomko