On 11/01/2011 09:02 AM, Daniel P. Berrange wrote:

From: "Daniel P. Berrange"<berrange@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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org