[libvirt] [PATCH] virFileIsSharedFSType: Check for fuse.glusterfs too

[Jiri Denemark]

On Mon, Oct 01, 2018 at 09:56:01 +0200, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1632711
> 
> GlusterFS is typically safe when it comes to migration. It's a
> network FS after all. However, it can be mounted via FUSE driver
> they provide. If that is the case we fail to identify it and
> think migration is not safe and require VIR_MIGRATE_UNSAFE flag.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/util/virfile.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 75 insertions(+), 2 deletions(-)
> 
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index f8ae07fe4a..ccffa063a6 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -3458,6 +3458,75 @@ int virFilePrintf(FILE *fp, const char *msg, ...)
>  # ifndef HUGETLBFS_MAGIC
>  #  define HUGETLBFS_MAGIC 0x958458f6
>  # endif
> +# ifndef FUSE_SUPER_MAGIC
> +#  define FUSE_SUPER_MAGIC 0x65735546
> +# endif
> +
> +# define PROC_MOUNTS "/proc/mounts"
> +
> +static int
> +virFileIsShareFixFUSE(const char *path,

s/Share/Shared/


> +                      long *f_type)
> +{
> +    char *dirpath = NULL;
> +    const char **mounts = NULL;
> +    size_t nmounts = 0;
> +    size_t i;
> +    char *p;
> +    FILE *f = NULL;
> +    struct mntent mb;
> +    char mntbuf[1024];
> +    bool found = false;
> +    int ret = -1;
> +
> +    if (VIR_STRDUP(dirpath, path) < 0)
> +        return -1;
> +
> +    if (!(f = setmntent(PROC_MOUNTS, "r"))) {
> +        virReportSystemError(errno,
> +                             _("Unable to open %s"),
> +                             PROC_MOUNTS);
> +        goto cleanup;
> +    }
> +
> +    while (getmntent_r(f, &mb, mntbuf, sizeof(mntbuf))) {
> +        if (STRNEQ("fuse.glusterfs", mb.mnt_type))
> +            continue;
> +
> +        if (VIR_APPEND_ELEMENT_COPY(mounts, nmounts, mb.mnt_dir) < 0)
> +            goto cleanup;
> +    }
> +
> +    do {
> +        if ((p = strrchr(dirpath, '/')) == NULL) {
> +            virReportSystemError(EINVAL,
> +                                 _("Invalid relative path '%s'"), path);
> +            goto cleanup;
> +        }
> +
> +        if (p == dirpath)
> +            *(p+1) = '\0';
> +        else
> +            *p = '\0';
> +
> +        for (i = 0; i < nmounts; i++) {
> +            if (STREQ(dirpath, mounts[i])) {
> +                found = true;
> +                VIR_DEBUG("Found gluster FUSE mountpoint=%s for path=%s. "
> +                          "Fixing shared FS type", mounts[i], path);
> +                *f_type = GFS2_MAGIC;
> +            }
> +        }

You could have used a string list and virStringListHasString instead of
this loop and get rid of "found" and just break the loop. Even in the
current form you should at least break the for loop if dirpath is found.

On the other hand, the code would need to be changed if we ever want to
extend it for other fuse-mounted file systems. So why not modifying it a
bit right away even if we'll still be checking just fuse.glusterfs? It
could save us some refactoring later...

> +    } while (!found && p != dirpath);
> +
> +    ret = 0;
> + cleanup:
> +    endmntent(f);
> +    VIR_FREE(mounts);
> +    VIR_FREE(dirpath);
> +    return ret;
> +}
> +
>  
>  int
>  virFileIsSharedFSType(const char *path,
> @@ -3503,6 +3572,12 @@ virFileIsSharedFSType(const char *path,
>          return -1;
>      }
>  
> +    if (sb.f_type == FUSE_SUPER_MAGIC) {
> +        VIR_DEBUG("Found FUSE mount for path=%s. Trying to fix it", path);
> +        if (virFileIsShareFixFUSE(path, (long *) &sb.f_type) < 0)
> +            return -1;

I don't think any failure while parsing /proc/mounts should be fatal
here. If it fails, we can just keep the FUSE fs type.

Jirka