[PATCH 0/7] Clean up virFileIsSharedFSType
Few cleanups for problems I've noticed when I had to understand how the shared FS detection is supposed to work. Peter Krempa (7): util: virFileIsSharedFSType: Pass bitmap of checked fs types as unsigned util: virFileIsSharedFSType: Annotate (some) shared filesystem names util: virFileIsSharedFixFUSE: Refactor cleanup util: virfile: Rewrite matching of FUSE-based shared filesystems util: virfile: Don't use VIR_FILE_SHFS_GFS2 for glusterfs util: virfile: Drop QB_MAGIC constant util: virFileIsSharedFSType: Simplify shared fs type declarations src/util/virfile.c | 139 +++++++++++++++++++++------------------------ src/util/virfile.h | 19 ++++--- 2 files changed, 76 insertions(+), 82 deletions(-) -- 2.37.1
We populate the bits individualy so unsigned is the proper type. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virfile.c | 4 ++-- src/util/virfile.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index ce541b8946..1def05b6c4 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -3394,7 +3394,7 @@ virFileIsSharedFixFUSE(const char *path, int virFileIsSharedFSType(const char *path, - int fstypes) + unsigned int fstypes) { g_autofree char *dirpath = NULL; char *p = NULL; @@ -3601,7 +3601,7 @@ virFileFindHugeTLBFS(virHugeTLBFS **ret_fs, #else /* defined __linux__ */ int virFileIsSharedFSType(const char *path G_GNUC_UNUSED, - int fstypes G_GNUC_UNUSED) + unsigned int fstypes G_GNUC_UNUSED) { /* XXX implement me :-) */ return 0; diff --git a/src/util/virfile.h b/src/util/virfile.h index 4af1ad9136..8d6df034f8 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -224,7 +224,7 @@ enum { VIR_FILE_SHFS_ACFS = (1 << 9), }; -int virFileIsSharedFSType(const char *path, int fstypes) ATTRIBUTE_NONNULL(1); +int virFileIsSharedFSType(const char *path, unsigned int fstypes) ATTRIBUTE_NONNULL(1); int virFileIsSharedFS(const char *path) ATTRIBUTE_NONNULL(1); int virFileIsClusterFS(const char *path) ATTRIBUTE_NONNULL(1); int virFileIsMountPoint(const char *file) ATTRIBUTE_NONNULL(1); -- 2.37.1
On a Monday in 2022, Peter Krempa wrote:
We populate the bits individualy so unsigned is the proper type.
*individually
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virfile.c | 4 ++-- src/util/virfile.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Expand some of the uncommon or unobvious filesystem names in a comment. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virfile.h | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/util/virfile.h b/src/util/virfile.h index 8d6df034f8..c4d11ea21a 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -213,15 +213,15 @@ bool virFileIsRegular(const char *file) ATTRIBUTE_NONNULL(1); enum { VIR_FILE_SHFS_NFS = (1 << 0), - VIR_FILE_SHFS_GFS2 = (1 << 1), - VIR_FILE_SHFS_OCFS = (1 << 2), - VIR_FILE_SHFS_AFS = (1 << 3), - VIR_FILE_SHFS_SMB = (1 << 4), - VIR_FILE_SHFS_CIFS = (1 << 5), + VIR_FILE_SHFS_GFS2 = (1 << 1), /* Global File System 2 */ + VIR_FILE_SHFS_OCFS = (1 << 2), /* Oracle Cluster FS (2) */ + VIR_FILE_SHFS_AFS = (1 << 3), /* Andrew File System */ + VIR_FILE_SHFS_SMB = (1 << 4), /* Server message block - windows shares */ + VIR_FILE_SHFS_CIFS = (1 << 5), /* Common Internet File System - windows shares */ VIR_FILE_SHFS_CEPH = (1 << 6), - VIR_FILE_SHFS_GPFS = (1 << 7), - VIR_FILE_SHFS_QB = (1 << 8), - VIR_FILE_SHFS_ACFS = (1 << 9), + VIR_FILE_SHFS_GPFS = (1 << 7), /* General Parallel File System/IBM Spectrum Scale */ + VIR_FILE_SHFS_QB = (1 << 8), /* Quobyte shared filesystem */ + VIR_FILE_SHFS_ACFS = (1 << 9), /* Oracle ASM Cluster File System */ }; int virFileIsSharedFSType(const char *path, unsigned int fstypes) ATTRIBUTE_NONNULL(1); -- 2.37.1
Automatically free memory of 'canonPath' so that the failure of 'setmntent' doesn't have to go to 'cleanup'. This allows us to remove the cleanup section and the 'ret' variable as the rest of the function can't fail. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virfile.c | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 1def05b6c4..a8443acf8d 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -3333,24 +3333,19 @@ virFileIsSharedFixFUSE(const char *path, char mntbuf[1024]; char *mntDir = NULL; char *mntType = NULL; - char *canonPath = NULL; + g_autofree char *canonPath = NULL; size_t maxMatching = 0; - int ret = -1; if (!(canonPath = virFileCanonicalizePath(path))) { - virReportSystemError(errno, - _("unable to canonicalize %s"), - path); + virReportSystemError(errno, _("unable to canonicalize %s"), path); return -1; } VIR_DEBUG("Path canonicalization: %s->%s", path, canonPath); if (!(f = setmntent(PROC_MOUNTS, "r"))) { - virReportSystemError(errno, - _("Unable to open %s"), - PROC_MOUNTS); - goto cleanup; + virReportSystemError(errno, _("Unable to open %s"), PROC_MOUNTS); + return -1; } while (getmntent_r(f, &mb, mntbuf, sizeof(mntbuf))) { @@ -3372,6 +3367,8 @@ virFileIsSharedFixFUSE(const char *path, } } + endmntent(f); + if (STREQ_NULLABLE(mntType, "fuse.glusterfs")) { VIR_DEBUG("Found gluster FUSE mountpoint=%s for path=%s. " "Fixing shared FS type", mntDir, canonPath); @@ -3382,13 +3379,9 @@ virFileIsSharedFixFUSE(const char *path, *f_type = QB_MAGIC; } - ret = 0; - cleanup: - VIR_FREE(canonPath); VIR_FREE(mntType); VIR_FREE(mntDir); - endmntent(f); - return ret; + return 0; } -- 2.37.1
'virFileIsSharedFixFUSE' was used to update the 'f_type' field for certain shared filesystem types. This patch renames it to 'virFileIsSharedFsFUSE' and makes it directly return whether the FUSE filesystem is shared or not and simplifies additions to the list of shared FUSE filesystems. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virfile.c | 54 ++++++++++++++++++++++++++++------------------ 1 file changed, 33 insertions(+), 21 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index a8443acf8d..33a3ffe8f7 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -3324,17 +3324,27 @@ virFileRemoveLastComponent(char *path) # define PROC_MOUNTS "/proc/mounts" + +struct virFileSharedFsData { + const char *mnttype; + unsigned int fstype; +}; + +static const struct virFileSharedFsData virFileSharedFsFUSE[] = { + { .mnttype = "fuse.glusterfs", .fstype = VIR_FILE_SHFS_GFS2 }, + { .mnttype = "fuse.quobyte", .fstype = VIR_FILE_SHFS_QB }, +}; + static int -virFileIsSharedFixFUSE(const char *path, - long long *f_type) +virFileIsSharedFsFUSE(const char *path, + unsigned int fstypes) { FILE *f = NULL; struct mntent mb; char mntbuf[1024]; - char *mntDir = NULL; - char *mntType = NULL; g_autofree char *canonPath = NULL; size_t maxMatching = 0; + bool isShared = false; if (!(canonPath = virFileCanonicalizePath(path))) { virReportSystemError(errno, _("unable to canonicalize %s"), path); @@ -3359,28 +3369,30 @@ virFileIsSharedFixFUSE(const char *path, continue; if (len > maxMatching) { + size_t i; + bool found = false; + + for (i = 0; i < G_N_ELEMENTS(virFileSharedFsFUSE); i++) { + if (STREQ_NULLABLE(mb.mnt_type, virFileSharedFsFUSE[i].mnttype) && + (fstypes & virFileSharedFsFUSE[i].fstype) > 0) { + found = true; + break; + } + } + + VIR_DEBUG("Updating shared='%d' for mountpoint '%s' type '%s'", + found, p, mb.mnt_type); + + isShared = found; maxMatching = len; - VIR_FREE(mntType); - VIR_FREE(mntDir); - mntDir = g_strdup(mb.mnt_dir); - mntType = g_strdup(mb.mnt_type); } } endmntent(f); - if (STREQ_NULLABLE(mntType, "fuse.glusterfs")) { - VIR_DEBUG("Found gluster FUSE mountpoint=%s for path=%s. " - "Fixing shared FS type", mntDir, canonPath); - *f_type = GFS2_MAGIC; - } else if (STREQ_NULLABLE(mntType, "fuse.quobyte")) { - VIR_DEBUG("Found Quobyte FUSE mountpoint=%s for path=%s. " - "Fixing shared FS type", mntDir, canonPath); - *f_type = QB_MAGIC; - } + if (isShared) + return 1; - VIR_FREE(mntType); - VIR_FREE(mntDir); return 0; } @@ -3432,8 +3444,8 @@ virFileIsSharedFSType(const char *path, f_type = sb.f_type; if (f_type == FUSE_SUPER_MAGIC) { - VIR_DEBUG("Found FUSE mount for path=%s. Trying to fix it", path); - virFileIsSharedFixFUSE(path, &f_type); + VIR_DEBUG("Found FUSE mount for path=%s", path); + return virFileIsSharedFsFUSE(path, fstypes); } VIR_DEBUG("Check if path %s with FS magic %lld is shared", -- 2.37.1
While the code works properly as no code path is specifically wanting to check for glusterfs, we should properly declare glusterfs as a separate from GFS2. Fixes: 478da65fb46 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virfile.c | 8 +++++--- src/util/virfile.h | 1 + 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 33a3ffe8f7..f5a61abd9f 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -3331,7 +3331,7 @@ struct virFileSharedFsData { }; static const struct virFileSharedFsData virFileSharedFsFUSE[] = { - { .mnttype = "fuse.glusterfs", .fstype = VIR_FILE_SHFS_GFS2 }, + { .mnttype = "fuse.glusterfs", .fstype = VIR_FILE_SHFS_GLUSTERFS }, { .mnttype = "fuse.quobyte", .fstype = VIR_FILE_SHFS_QB }, }; @@ -3668,7 +3668,8 @@ int virFileIsSharedFS(const char *path) VIR_FILE_SHFS_CEPH | VIR_FILE_SHFS_GPFS| VIR_FILE_SHFS_QB | - VIR_FILE_SHFS_ACFS); + VIR_FILE_SHFS_ACFS | + VIR_FILE_SHFS_GLUSTERFS); } @@ -3681,7 +3682,8 @@ virFileIsClusterFS(const char *path) return virFileIsSharedFSType(path, VIR_FILE_SHFS_GFS2 | VIR_FILE_SHFS_OCFS | - VIR_FILE_SHFS_CEPH); + VIR_FILE_SHFS_CEPH | + VIR_FILE_SHFS_GLUSTERFS); } diff --git a/src/util/virfile.h b/src/util/virfile.h index c4d11ea21a..f7a31d9f57 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -222,6 +222,7 @@ enum { VIR_FILE_SHFS_GPFS = (1 << 7), /* General Parallel File System/IBM Spectrum Scale */ VIR_FILE_SHFS_QB = (1 << 8), /* Quobyte shared filesystem */ VIR_FILE_SHFS_ACFS = (1 << 9), /* Oracle ASM Cluster File System */ + VIR_FILE_SHFS_GLUSTERFS = (1 << 10), /* gluster's FUSE-based client */ }; int virFileIsSharedFSType(const char *path, unsigned int fstypes) ATTRIBUTE_NONNULL(1); -- 2.37.1
The filesystem type magic constant was added for the 'quobyte' shared filesystem in commit 451094bd153 but is present neither in the kernel sources nor in coreutils which we've historically used as source of information. Since the code dealing with FUSE-based filesystems doesn't need the constand we can remove it and the now-dead check for it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virfile.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index f5a61abd9f..6302364797 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -3316,9 +3316,6 @@ virFileRemoveLastComponent(char *path) # ifndef GPFS_SUPER_MAGIC # define GPFS_SUPER_MAGIC 0x47504653 # endif -# ifndef QB_MAGIC -# define QB_MAGIC 0x51626d6e -# endif # define VIR_ACFS_MAGIC 0x61636673 @@ -3476,9 +3473,6 @@ virFileIsSharedFSType(const char *path, if ((fstypes & VIR_FILE_SHFS_GPFS) && (f_type == GPFS_SUPER_MAGIC)) return 1; - if ((fstypes & VIR_FILE_SHFS_QB) && - (f_type == QB_MAGIC)) - return 1; if ((fstypes & VIR_FILE_SHFS_ACFS) && (f_type == VIR_ACFS_MAGIC)) return 1; -- 2.37.1
On a Monday in 2022, Peter Krempa wrote:
The filesystem type magic constant was added for the 'quobyte' shared filesystem in commit 451094bd153 but is present neither in the kernel sources nor in coreutils which we've historically used as source of information.
I'm afraid the constant is some downstream magic number: https://listman.redhat.com/archives/libvir-list/2019-July/187233.html
Since the code dealing with FUSE-based filesystems doesn't need the constand we can remove it and the now-dead check for it.
*constant
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virfile.c | 6 ------ 1 file changed, 6 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Use approach similar to virFileIsSharedFsFUSE to declaratively handle the filesystem magic numbers mapping to libvirt's fstypes. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virfile.c | 48 +++++++++++++++++++--------------------------- 1 file changed, 20 insertions(+), 28 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 6302364797..ec40c04b1f 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -3324,6 +3324,7 @@ virFileRemoveLastComponent(char *path) struct virFileSharedFsData { const char *mnttype; + unsigned int magic; unsigned int fstype; }; @@ -3394,6 +3395,19 @@ virFileIsSharedFsFUSE(const char *path, } +static const struct virFileSharedFsData virFileSharedFs[] = { + { .fstype = VIR_FILE_SHFS_NFS, .magic = NFS_SUPER_MAGIC }, + { .fstype = VIR_FILE_SHFS_GFS2, .magic = GFS2_MAGIC }, + { .fstype = VIR_FILE_SHFS_OCFS, .magic = OCFS2_SUPER_MAGIC }, + { .fstype = VIR_FILE_SHFS_AFS, .magic = AFS_FS_MAGIC }, + { .fstype = VIR_FILE_SHFS_SMB, .magic = SMB_SUPER_MAGIC }, + { .fstype = VIR_FILE_SHFS_CIFS, .magic = CIFS_SUPER_MAGIC }, + { .fstype = VIR_FILE_SHFS_CEPH, .magic = CEPH_SUPER_MAGIC }, + { .fstype = VIR_FILE_SHFS_GPFS, .magic = GPFS_SUPER_MAGIC }, + { .fstype = VIR_FILE_SHFS_ACFS, .magic = VIR_ACFS_MAGIC }, +}; + + int virFileIsSharedFSType(const char *path, unsigned int fstypes) @@ -3403,6 +3417,7 @@ virFileIsSharedFSType(const char *path, struct statfs sb; int statfs_ret; long long f_type = 0; + size_t i; dirpath = g_strdup(path); @@ -3448,34 +3463,11 @@ virFileIsSharedFSType(const char *path, VIR_DEBUG("Check if path %s with FS magic %lld is shared", path, f_type); - if ((fstypes & VIR_FILE_SHFS_NFS) && - (f_type == NFS_SUPER_MAGIC)) - return 1; - - if ((fstypes & VIR_FILE_SHFS_GFS2) && - (f_type == GFS2_MAGIC)) - return 1; - if ((fstypes & VIR_FILE_SHFS_OCFS) && - (f_type == OCFS2_SUPER_MAGIC)) - return 1; - if ((fstypes & VIR_FILE_SHFS_AFS) && - (f_type == AFS_FS_MAGIC)) - return 1; - if ((fstypes & VIR_FILE_SHFS_SMB) && - (f_type == SMB_SUPER_MAGIC)) - return 1; - if ((fstypes & VIR_FILE_SHFS_CIFS) && - (f_type == CIFS_SUPER_MAGIC)) - return 1; - if ((fstypes & VIR_FILE_SHFS_CEPH) && - (f_type == CEPH_SUPER_MAGIC)) - return 1; - if ((fstypes & VIR_FILE_SHFS_GPFS) && - (f_type == GPFS_SUPER_MAGIC)) - return 1; - if ((fstypes & VIR_FILE_SHFS_ACFS) && - (f_type == VIR_ACFS_MAGIC)) - return 1; + for (i = 0; i < G_N_ELEMENTS(virFileSharedFs); i++) { + if (f_type == virFileSharedFs[i].magic && + (fstypes & virFileSharedFs[i].fstype) > 0) + return 1; + } return 0; } -- 2.37.1
On a Monday in 2022, Peter Krempa wrote:
Few cleanups for problems I've noticed when I had to understand how the shared FS detection is supposed to work.
Peter Krempa (7): util: virFileIsSharedFSType: Pass bitmap of checked fs types as unsigned util: virFileIsSharedFSType: Annotate (some) shared filesystem names util: virFileIsSharedFixFUSE: Refactor cleanup util: virfile: Rewrite matching of FUSE-based shared filesystems util: virfile: Don't use VIR_FILE_SHFS_GFS2 for glusterfs util: virfile: Drop QB_MAGIC constant util: virFileIsSharedFSType: Simplify shared fs type declarations
src/util/virfile.c | 139 +++++++++++++++++++++------------------------ src/util/virfile.h | 19 ++++--- 2 files changed, 76 insertions(+), 82 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa