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@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 :|