8:53 p.m.
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
8:53 p.m.
New subject: [PATCH 1/3] util: Document limitation of virFileCanonicalizePath
From: Jiri Denemark <jdenemar(a)redhat.com>
On most platforms virFileCanonicalizePath is implemented using
realpath(), which only works on existing paths.
Signed-off-by: Jiri Denemark <jdenemar(a)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
8:53 p.m.
New subject: [PATCH 2/3] util: Introduce virFileGetExistingParent helper
From: Jiri Denemark <jdenemar(a)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(a)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
8:53 p.m.
New subject: [PATCH 3/3] util: Fix virFileIsSharedFSOverride on nonexistent paths
From: Jiri Denemark <jdenemar(a)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(a)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
9:14 p.m.
New subject: [PATCH 0/3] util: Fix virFileIsSharedFSOverride on nonexistent
paths
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(a)redhat.com>
Jano
2
days inactive
2
days old
4 comments
2 participants
participants (2)
-
Jiri Denemark -
Ján Tomko