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

[Daniel P. Berrange]

On Tue, Nov 01, 2011 at 09:21:50AM -0600, Eric Blake wrote:
> 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 you failed to open "src", then you're not going to
succeed in mount'ing "src" later, so I don't see a point
in letting it continue with the fallback code. It just
means the clear error you get here, may well be replaced
with a less clear error later.

> >+    if (!(blkid = blkid_new_probe())) {
> >+        virReportSystemError(errno, "%s",
> >+                             _("Unable to create blkid library handle"));
> >+        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.

Opps, it was safe originally, but then I made blkid_new_probe jump
to cleanup on failure, so we do need the check


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|