On Thu, May 13, 2010 at 10:18:23PM -0400, David Allan wrote:

--- configure.ac | 25 +++++++++++++++++++++++++ 1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/configure.ac b/configure.ac index c187420..b10e4b0 100644 --- a/configure.ac +++ b/configure.ac @@ -42,6 +42,7 @@ HAL_REQUIRED=0.5.0 DEVMAPPER_REQUIRED=1.0.0 LIBCURL_REQUIRED="7.18.0" LIBPCAP_REQUIRED="1.0.0" +LIBBLKID_REQUIRED="2.17"

dnl Checks for C compiler. AC_PROG_CC @@ -1308,6 +1309,25 @@ if test "$with_nwfilter" = "yes" ; then fi AM_CONDITIONAL([WITH_NWFILTER], [test "$with_nwfilter" = "yes"])

+dnl libblkid is used by several storage drivers; therefore we probe +dnl for it unconditionally. +AC_ARG_WITH([libblkid], + [AS_HELP_STRING([--with-libblkid], + [use libblkid to scan for filesystems and partitions @<:@default=check@:>@])], + [], + [with_libblkid=check]) + +if test "x$with_libblkid" = "xyes" || test "x$with_libblkid" = "xcheck"; then + PKG_CHECK_MODULES([BLKID], + [blkid >= $LIBBLKID_REQUIRED], + [with_libblkid="yes"], + [with_libblkid="no"]) +fi + +if test x"$with_libblkid" = x"yes"; then + AC_DEFINE([HAVE_LIBBLKID], [1], [libblkid is present]) +fi +AM_CONDITIONAL([HAVE_LIBBLKID], [test x"$with_libblkid" = x"yes"])

AC_ARG_WITH([storage-fs], AC_HELP_STRING([--with-storage-fs], [with FileSystem backend for the storage driver @<:@default=check@:>@]),[],[with_storage_fs=check]) @@ -1341,12 +1361,15 @@ AM_CONDITIONAL([WITH_STORAGE_DIR], [test "$with_storage_dir" = "yes"]) if test "$with_storage_fs" = "yes" || test "$with_storage_fs" = "check"; then AC_PATH_PROG([MOUNT], [mount], [], [$PATH:/sbin:/usr/sbin]) AC_PATH_PROG([UMOUNT], [umount], [], [$PATH:/sbin:/usr/sbin]) + AC_PATH_PROG([MKFS], [mkfs], [], [$PATH:/sbin:/usr/sbin]) if test "$with_storage_fs" = "yes" ; then if test -z "$MOUNT" ; then AC_MSG_ERROR([We need mount for FS storage driver]) ; fi if test -z "$UMOUNT" ; then AC_MSG_ERROR([We need umount for FS storage driver]) ; fi + if test -z "$MKFS" ; then AC_MSG_ERROR([We need mkfs for FS storage driver]) ; fi else if test -z "$MOUNT" ; then with_storage_fs=no ; fi if test -z "$UMOUNT" ; then with_storage_fs=no ; fi + if test -z "$MKFS" ; then with_storage_fs=no ; fi

if test "$with_storage_fs" = "check" ; then with_storage_fs=yes ; fi fi @@ -1357,6 +1380,8 @@ if test "$with_storage_fs" = "yes" || test "$with_storage_fs" = "check"; then [Location or name of the mount program]) AC_DEFINE_UNQUOTED([UMOUNT],["$UMOUNT"], [Location or name of the mount program]) + AC_DEFINE_UNQUOTED([MKFS],["$MKFS"], + [Location or name of the mkfs program]) fi fi AM_CONDITIONAL([WITH_STORAGE_FS], [test "$with_storage_fs" = "yes"])

It is also neccessary to add dependancies to libvirt.spec.in for these new build time requirements on libblkid-devel Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On 05/13/2010 08:18 PM, David Allan wrote:

* This patch adds the ability to make the filesystem for a filesystem pool during a pool build.

The patch adds two new flags, probe and overwrite, to control when mkfs gets executed. By default, the patch preserves the current behavior, i.e., if no flags are specified, pool build on a filesystem pool only makes the directory on which the filesystem will be mounted.

typedef enum { VIR_STORAGE_POOL_BUILD_NEW = 0, /* Regular build from scratch */ - VIR_STORAGE_POOL_BUILD_REPAIR = 1, /* Repair / reinitialize */ - VIR_STORAGE_POOL_BUILD_RESIZE = 2 /* Extend existing pool */ + VIR_STORAGE_POOL_BUILD_REPAIR = (1 << 0), /* Repair / reinitialize */ + VIR_STORAGE_POOL_BUILD_RESIZE = (1 << 1), /* Extend existing pool */ + VIR_STORAGE_POOL_BUILD_PROBE = (1 << 2), /* Probe for existing pool */ + VIR_STORAGE_POOL_BUILD_OVERWRITE = (1 << 3) /* Overwrite data */

Can one specify both probe and overwrite, or are they mutually exclusive? Maybe it makes sense to allow both - don't overwrite the filesystem if it is not already of the correct type, but if it is the correct type, then erase it and start from scratch (contrasted with probe in isolation doing nothing if the right type is present but overwriting on all other types).

+++ b/src/Makefile.am @@ -747,6 +747,11 @@ libvirt_driver_storage_la_LDFLAGS += -module -avoid-version endif libvirt_driver_storage_la_SOURCES += $(STORAGE_DRIVER_SOURCES) libvirt_driver_storage_la_SOURCES += $(STORAGE_DRIVER_FS_SOURCES) +if HAVE_LIBBLKID +libvirt_driver_storage_la_SOURCES += storage/storage_backend_fs_libblkid.c +libvirt_driver_storage_la_CFLAGS += $(BLKID_CFLAGS) +libvirt_driver_storage_la_LDFLAGS += $(BLKID_LIBS)

Should be LIBADD, not LDFLAGS.

+ +static int +virStorageBackendMakeFileSystem(virStoragePoolObjPtr pool, + unsigned int flags) +{ + const char *device = NULL, *format = NULL; + bool ok_to_mkfs = false; + int ret = -1;

Missing virCheckFlags().

+ + if (flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) { + ok_to_mkfs = true; + } else if (flags & VIR_STORAGE_POOL_BUILD_PROBE && + virStorageBackendFileSystemProbe(device, format) == + FILESYSTEM_PROBE_NOT_FOUND) { + ok_to_mkfs = true; + }

Here's where your logic would need to change - either the two flags are mutually exclusive, and you need to error out if both are given, or the two are both supported, but you need to take into account if both are specified.

--- /dev/null +++ b/src/storage/storage_backend_fs_libblkid.c @@ -0,0 +1,97 @@ +/* + * storage_backend_fs_libblkid.c: libblkid specific code for + * filesystem backend + * + * Copyright (C) 2007-2010 Red Hat, Inc.

Is this just 2010, or did you really borrow significant content from other files with earlier copyrights? If so, list what file you borrowed from.

+ + /* Unfortunately this value comes from the pool definition + * where it is correctly const char, but liblbkid expects it

s/liblbkid/libblkid/

+ * to be char, thus the cast. */ + names[0] = (char *)format; + names[1] = NULL;

Yeah, unfortunate but necessary. Seems safe, though; libblkid has no business modifying the string.

diff --git a/src/util/virterror.c b/src/util/virterror.c index 96dd1e7..bd845fb 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -1019,6 +1019,18 @@ virErrorMsg(virErrorNumber error, const char *info) else errmsg = _("Storage volume not found: %s"); break; + case VIR_ERR_STORAGE_PROBE_FAILED: + if (info == NULL) + errmsg = _("Storage pool probe failed"); + else + errmsg = _("Storage pool probe failed: %s"); + break;

Goody - a merge conflict with my error cleanups. It's a race to see who gets first ACK :)

+++ b/tools/virsh.c @@ -4668,6 +4668,9 @@ static const vshCmdInfo info_pool_build[] = {

static const vshCmdOptDef opts_pool_build[] = { {"pool", VSH_OT_DATA, VSH_OFLAG_REQ, N_("pool name or uuid")}, + {"probe", VSH_OT_BOOL, 0, N_("do not overwrite an existing " + "pool of this type")}, + {"overwrite", VSH_OT_BOOL, 0, N_("overwrite any existing data")},

If we do allow both flags at once, then the wording of "probe" needs to be tweaked. So maybe it's better to make them mutually exclusive.

@@ -4675,7 +4678,7 @@ static int cmdPoolBuild(vshControl *ctl, const vshCmd *cmd) { virStoragePoolPtr pool; - int ret = TRUE; + int ret = TRUE, flags = 0;

Should flags be unsigned int? Looks like we'll need a v2 patch before I feel good giving an ack. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On Thu, May 13, 2010 at 10:18:24PM -0400, David Allan wrote:

diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index c96c4f3..c31f6a6 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -45,6 +45,7 @@ #include "util.h" #include "memory.h" #include "xml.h" +#include "logging.h"

#define VIR_FROM_THIS VIR_FROM_STORAGE

@@ -485,6 +486,83 @@ virStorageBackendFileSystemStart(virConnectPtr conn ATTRIBUTE_UNUSED, #endif /* WITH_STORAGE_FS */

+virStoragePoolProbeResult +virStorageBackendFileSystemProbeDummy(const char *device ATTRIBUTE_UNUSED, + const char *format ATTRIBUTE_UNUSED) +{ + virStorageReportError(VIR_ERR_OPERATION_INVALID, + _("probing for filesystems is unsupported " + "by this build")); + + return FILESYSTEM_PROBE_ERROR; +}

IMHO this doesn't really belong here.

+static int +virStorageBackendMakeFileSystem(virStoragePoolObjPtr pool, + unsigned int flags) +{ + const char *device = NULL, *format = NULL; + bool ok_to_mkfs = false; + int ret = -1; + + if (pool->def->source.devices == NULL) { + virStorageReportError(VIR_ERR_OPERATION_INVALID, + _("No source device specified when formatting pool '%s'"), + pool->def->name); + goto error; + } + + device = pool->def->source.devices[0].path; + format = virStoragePoolFormatFileSystemTypeToString(pool->def->source.format); + VIR_DEBUG("source device: '%s' format: '%s'", device, format); + + if (!virFileExists(device)) { + virStorageReportError(VIR_ERR_OPERATION_INVALID, + _("Source device does not exist when formatting pool '%s'"), + pool->def->name); + goto error; + } + + if (flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) { + ok_to_mkfs = true; + } else if (flags & VIR_STORAGE_POOL_BUILD_PROBE && + virStorageBackendFileSystemProbe(device, format) == + FILESYSTEM_PROBE_NOT_FOUND) { + ok_to_mkfs = true; + }

This block needs to report an error if != FILESYSTEM_PROBE_NOT_FOUND

+ + if (ok_to_mkfs) { + ret = virStorageBackendExecuteMKFS(device, format); + } + +error: + return ret; +} + /** * @conn connection to report errors against * @pool storage pool to build diff --git a/src/storage/storage_backend_fs.h b/src/storage/storage_backend_fs.h index 7def53e..d9e397f 100644 --- a/src/storage/storage_backend_fs.h +++ b/src/storage/storage_backend_fs.h @@ -29,6 +29,25 @@ # if WITH_STORAGE_FS extern virStorageBackend virStorageBackendFileSystem; extern virStorageBackend virStorageBackendNetFileSystem; + +typedef enum { + FILESYSTEM_PROBE_FOUND, + FILESYSTEM_PROBE_NOT_FOUND, + FILESYSTEM_PROBE_ERROR, +} virStoragePoolProbeResult; + +# if HAVE_LIBBLKID +# define virStorageBackendFileSystemProbe virStorageBackendFileSystemProbeLibblkid +virStoragePoolProbeResult +virStorageBackendFileSystemProbeLibblkid(const char *device, + const char *format); +# else /* if HAVE_LIBBLKID */ +# define virStorageBackendFileSystemProbe virStorageBackendFileSystemProbeDummy +# endif /* if HAVE_LIBBLKID */ +virStoragePoolProbeResult +virStorageBackendFileSystemProbeDummy(const char *device, + const char *format); +

This is rather gross

# endif extern virStorageBackend virStorageBackendDirectory;

diff --git a/src/storage/storage_backend_fs_libblkid.c b/src/storage/storage_backend_fs_libblkid.c new file mode 100644 index 0000000..ddd9264 --- /dev/null +++ b/src/storage/storage_backend_fs_libblkid.c @@ -0,0 +1,97 @@ +/* + * storage_backend_fs_libblkid.c: libblkid specific code for + * filesystem backend + * + * Copyright (C) 2007-2010 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: David Allan <dallan@redhat.com> + */ + +#include <config.h> +#include <blkid/blkid.h> + +#include "virterror_internal.h" +#include "storage_backend_fs.h" +#include "logging.h" + +#define VIR_FROM_THIS VIR_FROM_STORAGE +

Wrap this code in #ifdef HAVE_LIBBLKID

+virStoragePoolProbeResult +virStorageBackendFileSystemProbeLibblkid(const char *device, + const char *format) {

s/virStorageBackendFileSystemProbeLibblkid/virStorageBackendFileSystemProbe/

+ + virStoragePoolProbeResult ret = FILESYSTEM_PROBE_ERROR; + blkid_probe probe = NULL; + const char *fstype = NULL; + char *names[2]; + + VIR_DEBUG("Probing for existing filesystem of type %s on device %s", + format, device); + + if (blkid_known_fstype(format) == 0) { + virStorageReportError(VIR_ERR_STORAGE_PROBE_FAILED, + _("Not capable of probing for " + "filesystem of type %s"), + format); + goto out; + } + + probe = blkid_new_probe_from_filename(device); + if (probe == NULL) { + virStorageReportError(VIR_ERR_STORAGE_PROBE_FAILED, + _("Failed to create filesystem probe " + "for device %s"), + device); + goto out; + } + + /* Unfortunately this value comes from the pool definition + * where it is correctly const char, but liblbkid expects it + * to be char, thus the cast. */ + names[0] = (char *)format; + names[1] = NULL; + + blkid_probe_filter_superblocks_type(probe, + BLKID_FLTR_ONLYIN, + names); + + if (blkid_do_probe(probe) != 0) { + VIR_INFO("No filesystem of type '%s' found on device '%s'", + format, device); + ret = FILESYSTEM_PROBE_NOT_FOUND; + } else if (blkid_probe_lookup_value(probe, "TYPE", &fstype, NULL) == 0) { + virStorageReportError(VIR_ERR_STORAGE_PROBE_BUILT, + _("Existing filesystem of type '%s' found on " + "device '%s'"), + fstype, device); + ret = FILESYSTEM_PROBE_FOUND; + } + + if (blkid_do_probe(probe) != 1) { + virStorageReportError(VIR_ERR_STORAGE_PROBE_FAILED, + _("Found additional probes to run, " + "filesystem probing may be incorrect")); + ret = FILESYSTEM_PROBE_ERROR; + } + +out: + if (probe != NULL) { + blkid_free_probe(probe); + } + + return ret; +}

#else virStoragePoolProbeResult virStorageBackendFileSystemProbe(const char *device ATTRIBUTE_UNUSED, const char *format ATTRIBUTE_UNUSED) { virStorageReportError(VIR_ERR_OPERATION_INVALID, _("probing for filesystems is unsupported " "by this build")); return FILESYSTEM_PROBE_ERROR; } #endif

diff --git a/tools/virsh.c b/tools/virsh.c index 693d409..5f7804a 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -4668,6 +4668,9 @@ static const vshCmdInfo info_pool_build[] = {

static const vshCmdOptDef opts_pool_build[] = { {"pool", VSH_OT_DATA, VSH_OFLAG_REQ, N_("pool name or uuid")}, + {"probe", VSH_OT_BOOL, 0, N_("do not overwrite an existing " + "pool of this type")}, + {"overwrite", VSH_OT_BOOL, 0, N_("overwrite any existing data")}, {NULL, 0, 0, NULL} };

I'm not sure that 'probe' is a great name here. Perhaps '--no-overwrite' is better. Likewise even for the include file constant name. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Change per feedback from Dan B: * Add libblkid dependency to libvirt.spec.in --- configure.ac | 25 +++++++++++++++++++++++++ libvirt.spec.in | 5 +++++ 2 files changed, 30 insertions(+), 0 deletions(-) diff --git a/configure.ac b/configure.ac index 36ba703..6923ce2 100644 --- a/configure.ac +++ b/configure.ac @@ -43,6 +43,7 @@ DEVMAPPER_REQUIRED=1.0.0 LIBCURL_REQUIRED="7.18.0" LIBPCAP_REQUIRED="1.0.0" LIBNL_REQUIRED="1.1" +LIBBLKID_REQUIRED="2.17" dnl Checks for C compiler. AC_PROG_CC @@ -1309,6 +1310,25 @@ if test "$with_nwfilter" = "yes" ; then fi AM_CONDITIONAL([WITH_NWFILTER], [test "$with_nwfilter" = "yes"]) +dnl libblkid is used by several storage drivers; therefore we probe +dnl for it unconditionally. +AC_ARG_WITH([libblkid], + [AS_HELP_STRING([--with-libblkid], + [use libblkid to scan for filesystems and partitions @<:@default=check@:>@])], + [], + [with_libblkid=check]) + +if test "x$with_libblkid" = "xyes" || test "x$with_libblkid" = "xcheck"; then + PKG_CHECK_MODULES([BLKID], + [blkid >= $LIBBLKID_REQUIRED], + [with_libblkid="yes"], + [with_libblkid="no"]) +fi + +if test x"$with_libblkid" = x"yes"; then + AC_DEFINE([HAVE_LIBBLKID], [1], [libblkid is present]) +fi +AM_CONDITIONAL([HAVE_LIBBLKID], [test x"$with_libblkid" = x"yes"]) AC_ARG_WITH([storage-fs], AC_HELP_STRING([--with-storage-fs], [with FileSystem backend for the storage driver @<:@default=check@:>@]),[],[with_storage_fs=check]) @@ -1342,12 +1362,15 @@ AM_CONDITIONAL([WITH_STORAGE_DIR], [test "$with_storage_dir" = "yes"]) if test "$with_storage_fs" = "yes" || test "$with_storage_fs" = "check"; then AC_PATH_PROG([MOUNT], [mount], [], [$PATH:/sbin:/usr/sbin]) AC_PATH_PROG([UMOUNT], [umount], [], [$PATH:/sbin:/usr/sbin]) + AC_PATH_PROG([MKFS], [mkfs], [], [$PATH:/sbin:/usr/sbin]) if test "$with_storage_fs" = "yes" ; then if test -z "$MOUNT" ; then AC_MSG_ERROR([We need mount for FS storage driver]) ; fi if test -z "$UMOUNT" ; then AC_MSG_ERROR([We need umount for FS storage driver]) ; fi + if test -z "$MKFS" ; then AC_MSG_ERROR([We need mkfs for FS storage driver]) ; fi else if test -z "$MOUNT" ; then with_storage_fs=no ; fi if test -z "$UMOUNT" ; then with_storage_fs=no ; fi + if test -z "$MKFS" ; then with_storage_fs=no ; fi if test "$with_storage_fs" = "check" ; then with_storage_fs=yes ; fi fi @@ -1358,6 +1381,8 @@ if test "$with_storage_fs" = "yes" || test "$with_storage_fs" = "check"; then [Location or name of the mount program]) AC_DEFINE_UNQUOTED([UMOUNT],["$UMOUNT"], [Location or name of the mount program]) + AC_DEFINE_UNQUOTED([MKFS],["$MKFS"], + [Location or name of the mkfs program]) fi fi AM_CONDITIONAL([WITH_STORAGE_FS], [test "$with_storage_fs" = "yes"]) diff --git a/libvirt.spec.in b/libvirt.spec.in index 6edbf2f..1bb58db 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -228,6 +228,11 @@ BuildRequires: util-linux # For showmount in FS driver (netfs discovery) BuildRequires: nfs-utils Requires: nfs-utils +# For mkfs +Requires: util-linux +# For pool-build probing for existing pools +BuildRequires: libblkid-devel >= 2.17 +Requires: libblkid >= 2.17 # For glusterfs %if 0%{?fedora} >= 11 Requires: glusterfs-client >= 2.0.1 -- 1.7.0.1

* This patch adds the ability to make the filesystem for a filesystem pool during a pool build. The patch adds two new flags, no overwrite and overwrite, to control when mkfs gets executed. By default, the patch preserves the current behavior, i.e., if no flags are specified, pool build on a filesystem pool only makes the directory on which the filesystem will be mounted. If the no overwrite flag is specified, the target device is checked to determine if a filesystem of the type specified in the pool is present. If a filesystem of that type is already present, mkfs is not executed and the build call returns an error. Otherwise, mkfs is executed and any data present on the device is overwritten. If the overwrite flag is specified, mkfs is always executed, and any existing data on the target device is overwritten unconditionally. Changes per feedback from eblake: * Made probe & overwrite flags exclusive * Changed LDFLAGS to LIBADD in Makefile.am * Added missing virCheckFlags() * Fixed copyright dates * Removed cast of char * passed to libblkid and replaced it with a strdup'd copy * Changed flags to an unsigned int in virsh.c Changes per feedback from Dan B. * Changed probe flag to no-overwrite * Moved libblkid probe code into storage_backend_fs.c --- include/libvirt/libvirt.h.in | 6 +- include/libvirt/virterror.h | 2 + src/Makefile.am | 4 + src/libvirt_private.syms | 4 + src/storage/storage_backend_fs.c | 180 +++++++++++++++++++++++++++++++++++++- src/storage/storage_backend_fs.h | 9 ++- src/util/virterror.c | 12 +++ tools/virsh.c | 14 +++- 8 files changed, 224 insertions(+), 7 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 19d5205..075beee 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1126,8 +1126,10 @@ typedef enum { typedef enum { VIR_STORAGE_POOL_BUILD_NEW = 0, /* Regular build from scratch */ - VIR_STORAGE_POOL_BUILD_REPAIR = 1, /* Repair / reinitialize */ - VIR_STORAGE_POOL_BUILD_RESIZE = 2 /* Extend existing pool */ + VIR_STORAGE_POOL_BUILD_REPAIR = (1 << 0), /* Repair / reinitialize */ + VIR_STORAGE_POOL_BUILD_RESIZE = (1 << 1), /* Extend existing pool */ + VIR_STORAGE_POOL_BUILD_NO_OVERWRITE = (1 << 2), /* Do not overwrite existing pool */ + VIR_STORAGE_POOL_BUILD_OVERWRITE = (1 << 3) /* Overwrite data */ } virStoragePoolBuildFlags; typedef enum { diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index 3bbb293..8d15671 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -163,6 +163,8 @@ typedef enum { VIR_WAR_NO_STORAGE, /* failed to start storage */ VIR_ERR_NO_STORAGE_POOL, /* storage pool not found */ VIR_ERR_NO_STORAGE_VOL, /* storage pool not found */ + VIR_ERR_STORAGE_PROBE_FAILED, /* storage pool probe failed */ + VIR_ERR_STORAGE_PROBE_BUILT, /* storage pool already built */ VIR_WAR_NO_NODE, /* failed to start node driver */ VIR_ERR_INVALID_NODE_DEVICE,/* invalid node device object */ VIR_ERR_NO_NODE_DEVICE,/* node device not found */ diff --git a/src/Makefile.am b/src/Makefile.am index 6bdf73c..bc4db48 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -773,6 +773,10 @@ libvirt_driver_storage_la_LDFLAGS += -module -avoid-version endif libvirt_driver_storage_la_SOURCES += $(STORAGE_DRIVER_SOURCES) libvirt_driver_storage_la_SOURCES += $(STORAGE_DRIVER_FS_SOURCES) +if HAVE_LIBBLKID +libvirt_driver_storage_la_CFLAGS += $(BLKID_CFLAGS) +libvirt_driver_storage_la_LIBADD += $(BLKID_LIBS) +endif endif if WITH_STORAGE_LVM diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6cb3d66..abe9a2a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -624,6 +624,10 @@ virStorageFileGetMetadata; virStorageFileGetMetadataFromFD; virStorageFileIsSharedFS; +# storage_backend_fs.h +virStorageBackendFileSystemProbeLibblkid; +virStorageBackendFileSystemProbeDummy; + # threads.h virMutexInit; virMutexDestroy; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index f0cd770..eb083cd 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -34,6 +34,10 @@ #include <unistd.h> #include <string.h> +#if HAVE_LIBBLKID +# include <blkid/blkid.h> +#endif + #include <libxml/parser.h> #include <libxml/tree.h> #include <libxml/xpath.h> @@ -45,6 +49,7 @@ #include "util.h" #include "memory.h" #include "xml.h" +#include "logging.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -483,6 +488,157 @@ virStorageBackendFileSystemStart(virConnectPtr conn ATTRIBUTE_UNUSED, #endif /* WITH_STORAGE_FS */ +#if HAVE_LIBBLKID +static virStoragePoolProbeResult +virStorageBackendFileSystemProbe(const char *device, + const char *format) { + + virStoragePoolProbeResult ret = FILESYSTEM_PROBE_ERROR; + blkid_probe probe = NULL; + const char *fstype = NULL; + char *names[2], *libblkid_format = NULL; + + VIR_DEBUG("Probing for existing filesystem of type %s on device %s", + format, device); + + if (blkid_known_fstype(format) == 0) { + virStorageReportError(VIR_ERR_STORAGE_PROBE_FAILED, + _("Not capable of probing for " + "filesystem of type %s"), + format); + goto error; + } + + probe = blkid_new_probe_from_filename(device); + if (probe == NULL) { + virStorageReportError(VIR_ERR_STORAGE_PROBE_FAILED, + _("Failed to create filesystem probe " + "for device %s"), + device); + goto error; + } + + if ((libblkid_format = strdup(format)) == NULL) { + virReportOOMError(); + goto error; + } + + names[0] = libblkid_format; + names[1] = NULL; + + blkid_probe_filter_superblocks_type(probe, + BLKID_FLTR_ONLYIN, + names); + + if (blkid_do_probe(probe) != 0) { + VIR_INFO("No filesystem of type '%s' found on device '%s'", + format, device); + ret = FILESYSTEM_PROBE_NOT_FOUND; + } else if (blkid_probe_lookup_value(probe, "TYPE", &fstype, NULL) == 0) { + virStorageReportError(VIR_ERR_STORAGE_PROBE_BUILT, + _("Existing filesystem of type '%s' found on " + "device '%s'"), + fstype, device); + ret = FILESYSTEM_PROBE_FOUND; + } + + if (blkid_do_probe(probe) != 1) { + virStorageReportError(VIR_ERR_STORAGE_PROBE_FAILED, + _("Found additional probes to run, " + "filesystem probing may be incorrect")); + ret = FILESYSTEM_PROBE_ERROR; + } + +error: + VIR_FREE(libblkid_format); + + if (probe != NULL) { + blkid_free_probe(probe); + } + + return ret; +} + +#else /* #if HAVE_LIBBLKID */ + +static virStoragePoolProbeResult +virStorageBackendFileSystemProbe(const char *device ATTRIBUTE_UNUSED, + const char *format ATTRIBUTE_UNUSED) +{ + virStorageReportError(VIR_ERR_OPERATION_INVALID, + _("probing for filesystems is unsupported " + "by this build")); + + return FILESYSTEM_PROBE_ERROR; +} + +#endif /* #if HAVE_LIBBLKID */ + +static int +virStorageBackendExecuteMKFS(const char *device, + const char *format) +{ + int ret = 0; + const char *mkfsargv[5] = { MKFS, + "-t", + format, + device, + NULL }; + + if (virRun(mkfsargv, NULL) < 0) { + virReportSystemError(errno, + _("Failed to make filesystem of " + "type '%s' on device '%s'"), + format, device); + ret = -1; + } + + return ret; +} + + +static int +virStorageBackendMakeFileSystem(virStoragePoolObjPtr pool, + unsigned int flags) +{ + const char *device = NULL, *format = NULL; + bool ok_to_mkfs = false; + int ret = -1; + + if (pool->def->source.devices == NULL) { + virStorageReportError(VIR_ERR_OPERATION_INVALID, + _("No source device specified when formatting pool '%s'"), + pool->def->name); + goto error; + } + + device = pool->def->source.devices[0].path; + format = virStoragePoolFormatFileSystemTypeToString(pool->def->source.format); + VIR_DEBUG("source device: '%s' format: '%s'", device, format); + + if (!virFileExists(device)) { + virStorageReportError(VIR_ERR_OPERATION_INVALID, + _("Source device does not exist when formatting pool '%s'"), + pool->def->name); + goto error; + } + + if (flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) { + ok_to_mkfs = true; + } else if (flags & VIR_STORAGE_POOL_BUILD_NO_OVERWRITE && + virStorageBackendFileSystemProbe(device, format) == + FILESYSTEM_PROBE_NOT_FOUND) { + ok_to_mkfs = true; + } + + if (ok_to_mkfs) { + ret = virStorageBackendExecuteMKFS(device, format); + } + +error: + return ret; +} + /** * @conn connection to report errors against * @pool storage pool to build @@ -496,12 +652,24 @@ virStorageBackendFileSystemStart(virConnectPtr conn ATTRIBUTE_UNUSED, static int virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { int err, ret = -1; - char *parent; + char *parent = NULL; char *p; + virCheckFlags(VIR_STORAGE_POOL_BUILD_OVERWRITE | + VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, ret); + + if (flags == (VIR_STORAGE_POOL_BUILD_OVERWRITE | + VIR_STORAGE_POOL_BUILD_NO_OVERWRITE)) { + + virStorageReportError(VIR_ERR_OPERATION_INVALID, + _("Overwrite and no overwrite flags" + " are mutually exclusive")); + goto error; + } + if ((parent = strdup(pool->def->target.path)) == NULL) { virReportOOMError(); goto error; @@ -550,7 +718,13 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, goto error; } } - ret = 0; + + if (flags != 0) { + ret = virStorageBackendMakeFileSystem(pool, flags); + } else { + ret = 0; + } + error: VIR_FREE(parent); return ret; diff --git a/src/storage/storage_backend_fs.h b/src/storage/storage_backend_fs.h index 7def53e..f4ae4e3 100644 --- a/src/storage/storage_backend_fs.h +++ b/src/storage/storage_backend_fs.h @@ -29,7 +29,14 @@ # if WITH_STORAGE_FS extern virStorageBackend virStorageBackendFileSystem; extern virStorageBackend virStorageBackendNetFileSystem; -# endif + +typedef enum { + FILESYSTEM_PROBE_FOUND, + FILESYSTEM_PROBE_NOT_FOUND, + FILESYSTEM_PROBE_ERROR, +} virStoragePoolProbeResult; + extern virStorageBackend virStorageBackendDirectory; +# endif #endif /* __VIR_STORAGE_BACKEND_FS_H__ */ diff --git a/src/util/virterror.c b/src/util/virterror.c index 96dd1e7..bd845fb 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -1019,6 +1019,18 @@ virErrorMsg(virErrorNumber error, const char *info) else errmsg = _("Storage volume not found: %s"); break; + case VIR_ERR_STORAGE_PROBE_FAILED: + if (info == NULL) + errmsg = _("Storage pool probe failed"); + else + errmsg = _("Storage pool probe failed: %s"); + break; + case VIR_ERR_STORAGE_PROBE_BUILT: + if (info == NULL) + errmsg = _("Storage pool already built"); + else + errmsg = _("Storage pool already built: %s"); + break; case VIR_ERR_INVALID_STORAGE_POOL: if (info == NULL) errmsg = _("invalid storage pool pointer in"); diff --git a/tools/virsh.c b/tools/virsh.c index 1279f41..e4bc100 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -4672,6 +4672,9 @@ static const vshCmdInfo info_pool_build[] = { static const vshCmdOptDef opts_pool_build[] = { {"pool", VSH_OT_DATA, VSH_OFLAG_REQ, N_("pool name or uuid")}, + {"no-overwrite", VSH_OT_BOOL, 0, N_("do not overwrite an existing " + "pool of this type")}, + {"overwrite", VSH_OT_BOOL, 0, N_("overwrite any existing data")}, {NULL, 0, 0, NULL} }; @@ -4680,6 +4683,7 @@ cmdPoolBuild(vshControl *ctl, const vshCmd *cmd) { virStoragePoolPtr pool; int ret = TRUE; + unsigned int flags = 0; char *name; if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) @@ -4688,7 +4692,15 @@ cmdPoolBuild(vshControl *ctl, const vshCmd *cmd) if (!(pool = vshCommandOptPool(ctl, cmd, "pool", &name))) return FALSE; - if (virStoragePoolBuild(pool, 0) == 0) { + if (vshCommandOptBool (cmd, "no-overwrite")) { + flags |= VIR_STORAGE_POOL_BUILD_NO_OVERWRITE; + } + + if (vshCommandOptBool (cmd, "overwrite")) { + flags |= VIR_STORAGE_POOL_BUILD_OVERWRITE; + } + + if (virStoragePoolBuild(pool, flags) == 0) { vshPrint(ctl, _("Pool %s built\n"), name); } else { vshError(ctl, _("Failed to build pool %s"), name); -- 1.7.0.1