[libvirt] [PATCH] Add support for probing filesystem with libblkid

[Eric Blake]

On 11/01/2011 09:02 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange"<berrange at redhat.com>
>
> The LXC code for mounting container filesystems from block devices
> tries all filesystems in /etc/filesystems and possibly those in
> /proc/filesystems. The regular mount binary, however, first tries
> using libblkid to detect the format. Add support for doing the same
> in libvirt, since Fedora's /etc/filesystems is missing many formats,
> most notably ext4 which is the default filesystem Fedora uses!
>
> * src/Makefile.am: Link libvirt_lxc to libblkid
> * src/lxc/lxc_container.c: Probe filesystem format with liblkid

s/liblkid/libblkid/

> ---
>   src/Makefile.am         |    4 ++
>   src/lxc/lxc_container.c |  102 ++++++++++++++++++++++++++++++++++++++++++++++-
>   2 files changed, 105 insertions(+), 1 deletions(-)

configure.ac already unconditionally checked for libblkid, so this is 
simple enough to start using.

> +#ifdef HAVE_LIBBLKID
> +static int
> +lxcContainerMountDetectFilesystem(const char *src, char **type)
> +{
> +    int fd;
> +    int ret = -1;
> +    int rc;
> +    const char *data = NULL;
> +    blkid_probe blkid = NULL;
> +
> +    *type = NULL;
> +
> +    if ((fd = open(src, O_RDONLY))<  0) {
> +        virReportSystemError(errno,
> +                             _("Unable to open filesystem %s"), src);
> +        return -1;

If we fail to open() the file, can't we at least fall back to the mount 
probing?  That is, what do we gain by returning failure here, instead of 
returning 0 and letting the fallback code be attempted?

> +    }
> +
> +    if (!(blkid = blkid_new_probe())) {
> +        virReportSystemError(errno, "%s",
> +                             _("Unable to create blkid library handle"));
> +        goto cleanup;
> +    }
> +    if (blkid_probe_set_device(blkid, fd, 0, 0)<  0) {
> +        virReportSystemError(EINVAL,
> +                             _("Unable to associated device %s with blkid library"),
> +                             src);

s/associated/associate/

> +        goto cleanup;
> +    }
> +
> +    blkid_probe_enable_superblocks(blkid, 1);
> +
> +    blkid_probe_set_superblocks_flags(blkid, BLKID_SUBLKS_TYPE);
> +
> +    rc = blkid_do_safeprobe(blkid);
> +    if (rc != 0) {
> +        if (rc == 1) /* Nothing found, return success with *type == NULL */
> +            goto done;
> +
> +        if (rc == -2) {
> +            virReportSystemError(EINVAL,
> +                                 _("Too many filesystems detected for %s"),
> +                                 src);
> +        } else {
> +            virReportSystemError(errno,
> +                                 _("Unable to detect filesystem for %s"),
> +                                 src);
> +        }
> +        goto cleanup;
> +    }
> +
> +    if (blkid_probe_lookup_value(blkid, "TYPE",&data, NULL)<  0) {
> +        virReportSystemError(ENOENT,
> +                             _("Unable to find filesystem type for %s"),
> +                             src);
> +        goto cleanup;
> +    }
> +
> +    if (!(*type = strdup(data))) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }
> +
> +done:
> +    ret = 0;
> +cleanup:
> +    VIR_FORCE_CLOSE(fd);
> +    blkid_free_probe(blkid);

Is this safe?  In storage_backed_fs.c, you check that probe != NULL 
before calling blkid_free_probe.  If this function is free-like, we 
should add it to cfg.mk and update storage_backend_fs.c to get rid of 
the useless conditional; if not, then this needs fixing to avoid 
crashing libblkid on a NULL deref.

> +    return ret;
> +}
> +#else /* ! HAVE_LIBBLKID */
> +static int
> +lxcContainerMountDetectFilesystem(const char *src ATTRIBUTE_UNUSED,
> +                                  char **type)
> +{
> +    /* No libblkid, so just return success with no detected type */
> +    *type = NULL;
> +    return 0;
> +}
> +#endif /* ! HAVE_LIBBLKID */
>
>   /*
>    * This functions attempts to do automatic detection of filesystem
> @@ -771,6 +855,8 @@ static int lxcContainerMountFSBlockHelper(virDomainFSDefPtr fs,
>   {
>       int fsflags = 0;
>       int ret = -1;
> +    char *format = NULL;
> +
>       if (fs->readonly)
>           fsflags |= MS_RDONLY;
>
> @@ -781,7 +867,21 @@ static int lxcContainerMountFSBlockHelper(virDomainFSDefPtr fs,
>           goto cleanup;
>       }
>
> -    ret = lxcContainerMountFSBlockAuto(fs, fsflags, src, srcprefix);
> +    if (lxcContainerMountDetectFilesystem(src,&format)<  0)
> +        goto cleanup;

Given my earlier question, about the only failures that should avoid the 
fallback would be OOM failure.  But since OOM is a real possibility, I 
agree with the signature (return 0 on success, whether or not format == 
NULL).

> +
> +    if (format) {
> +        VIR_DEBUG("Mount %s with detected format %s", src, format);
> +        if (mount(src, fs->dst, format, fsflags, NULL)<  0) {
> +            virReportSystemError(errno,
> +                                 _("Failed to mount device %s to %s as %s"),
> +                                 src, fs->dst, format);
> +            goto cleanup;
> +        }
> +        ret = 0;
> +    } else {
> +        ret = lxcContainerMountFSBlockAuto(fs, fsflags, src, srcprefix);
> +    }

This part makes sense.  Overall, I like the idea, but it may be worth a 
v2 depending on the answers to my questions above.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org