On 11/01/2011 09:02 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange"<berrange@xxxxxxxxxx>
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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list