[PATCH 1/2] util: Avoid statfs in virFileGetExistingParent

Jiri Denemark via Devel posted 2 patches 3 months, 1 week ago
[PATCH 1/2] util: Avoid statfs in virFileGetExistingParent
Posted by Jiri Denemark via Devel 3 months, 1 week ago
From: Jiri Denemark <jdenemar@redhat.com>

The code was separated from virFileIsSharedFSType which is Linux-only,
but virFileGetExistingParent is also called from
virFileIsSharedFSOverride which is OS independent. Thus we can't use
statfs. Let's use virFileExists (access) instead, we were not interested
in anything but success/failure from statfs anyway.

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
---
 src/util/virfile.c  |  3 +--
 tests/virfilemock.c | 28 ++++++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/src/util/virfile.c b/src/util/virfile.c
index 06e8e08ddc..d79a60baee 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -3561,14 +3561,13 @@ 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) {
+    while (!virFileExists(dirpath) && p != dirpath) {
         if (!(p = strrchr(dirpath, '/'))) {
             virReportSystemError(EINVAL,
                                  _("Invalid relative path '%1$s'"), path);
diff --git a/tests/virfilemock.c b/tests/virfilemock.c
index 4f1b8aecd7..5a09cce855 100644
--- a/tests/virfilemock.c
+++ b/tests/virfilemock.c
@@ -21,6 +21,7 @@
 #include <stdio.h>
 #include <mntent.h>
 #include <sys/vfs.h>
+#include <unistd.h>
 #ifdef __linux__
 # include <linux/magic.h>
 #endif
@@ -32,6 +33,7 @@
 static FILE *(*real_setmntent)(const char *filename, const char *type);
 static int (*real_statfs)(const char *path, struct statfs *buf);
 static char *(*real_realpath)(const char *path, char *resolved);
+static int (*real_access)(const char *path, int mode);
 
 
 static void
@@ -43,6 +45,7 @@ init_syms(void)
     VIR_MOCK_REAL_INIT(setmntent);
     VIR_MOCK_REAL_INIT(statfs);
     VIR_MOCK_REAL_INIT(realpath);
+    VIR_MOCK_REAL_INIT(access);
 }
 
 
@@ -200,3 +203,28 @@ realpath(const char *path, char *resolved)
 
     return real_realpath(path, resolved);
 }
+
+
+int
+access(const char *path, int mode)
+{
+    const char *mtab = getenv("LIBVIRT_MTAB");
+
+    init_syms();
+
+    if (mtab && mode == F_OK) {
+        struct statfs buf;
+
+        /* The real statfs works on any existing file on a filesystem, while
+         * our mocked version only works on the mount point. Thus we have to
+         * pretend no files on the filesystem exist to make sure
+         * virFileGetExistingParent returns the mount point which can later be
+         * checked by statfs. Instead of checking we were called for a mount
+         * point by walking through the mtab, we just call our mocked statfs
+         * that does it for us.
+         */
+        return statfs_mock(mtab, path, &buf);
+    }
+
+    return real_access(path, mode);
+}
-- 
2.49.0
Re: [PATCH 1/2] util: Avoid statfs in virFileGetExistingParent
Posted by Martin Kletzander via Devel 3 months, 1 week ago
On Tue, Jun 03, 2025 at 11:07:43AM +0200, Jiri Denemark via Devel wrote:
>From: Jiri Denemark <jdenemar@redhat.com>
>
>The code was separated from virFileIsSharedFSType which is Linux-only,
>but virFileGetExistingParent is also called from
>virFileIsSharedFSOverride which is OS independent. Thus we can't use
>statfs. Let's use virFileExists (access) instead, we were not interested
>in anything but success/failure from statfs anyway.
>
>Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
>---
> src/util/virfile.c  |  3 +--
> tests/virfilemock.c | 28 ++++++++++++++++++++++++++++
> 2 files changed, 29 insertions(+), 2 deletions(-)
>
>diff --git a/src/util/virfile.c b/src/util/virfile.c
>index 06e8e08ddc..d79a60baee 100644
>--- a/src/util/virfile.c
>+++ b/src/util/virfile.c
>@@ -3561,14 +3561,13 @@ 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.

s/stat/access/ maybe?

>      * 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) {
>+    while (!virFileExists(dirpath) && p != dirpath) {
>         if (!(p = strrchr(dirpath, '/'))) {
>             virReportSystemError(EINVAL,
>                                  _("Invalid relative path '%1$s'"), path);