Re: [libvirt] 'build' on FS pool now unconditionally formats?

Now that 0.7.7 is done, we need to revisit fs pool building. Because it is impossible to implement a check for arbitrary existing data, the only safe option is to require the overwrite flag in all cases. If we do not require the flag in all cases, virt-manager and virsh will format unknown partitions without providing any indication to the user that a destructive operation is about to take place. The only input from the user will be the selection of the partition. All other instances of destructive behavior require explicit confirmation from the user, or as Cole aptly put it, are loudly warned about by virt-manager. I wish that a safe alternative existed, but none does.

The attached patch implements this behavior.

From cf05596823881448725cd67094f39c136144683b Mon Sep 17 00:00:00 2001 From: David Allan <dallan(a)redhat.com&gt; Date: Mon, 15 Mar 2010 14:04:41 -0400 Subject: [PATCH 1/1] Add fs pool formatting * This patch reinstates pool formatting and adds a flag to enable overwriting of data. --- configure.ac | 5 +++++ include/libvirt/libvirt.h.in | 5 +++-- libvirt.spec.in | 2 ++ src/storage/storage_backend_fs.c | 34 +++++++++++++++++++++++++++++++++- tools/virsh.c | 8 +++++++- 5 files changed, 50 insertions(+), 4 deletions(-) diff --git a/configure.ac b/configure.ac index 654b9a8..3869f45 100644 --- a/configure.ac +++ b/configure.ac @@ -1300,12 +1300,15 @@ AM_CONDITIONAL([WITH_STORAGE_DIR], [test "$with_storage_dir" = "yes"]) if test "$with_storage_fs" = "yes" -o "$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 @@ -1316,6 +1319,8 @@ if test "$with_storage_fs" = "yes" -o "$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/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 0d1b5b5..77e6b8d 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1052,8 +1052,9 @@ 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_OVERWRITE = (1 << 2) /* Overwrite data */ } virStoragePoolBuildFlags; typedef enum { diff --git a/libvirt.spec.in b/libvirt.spec.in index a54d546..c6e0678 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -209,6 +209,8 @@ BuildRequires: util-linux # For showmount in FS driver (netfs discovery) BuildRequires: nfs-utils Requires: nfs-utils +# For mkfs +Requires: util-linux # For glusterfs %if 0%{?fedora} >= 11 Requires: glusterfs-client >= 2.0.1 diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index a83fc01..fe43af2 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 @@ -498,8 +499,9 @@ virStorageBackendFileSystemStart(virConnectPtr conn ATTRIBUTE_UNUSED, static int virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { + const char *mkfsargv[5], *device = NULL, *format = NULL; int err, ret = -1; char *parent; char *p; @@ -552,6 +554,36 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, goto error; } } + + if (flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) { + + if (pool->def->source.devices == NULL) { + virStorageReportError(VIR_ERR_INVALID_ARG, + _("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); + + mkfsargv[0] = MKFS; + mkfsargv[1] = "-t"; + mkfsargv[2] = format; + mkfsargv[3] = device; + mkfsargv[4] = NULL; + + if (virRun(mkfsargv, NULL) < 0) { + virReportSystemError(errno, + _("Failed to make filesystem of " + "type '%s' on device '%s'"), + format, device); + goto error; + } + } + ret = 0; error: VIR_FREE(parent); diff --git a/tools/virsh.c b/tools/virsh.c index aa85ee6..4b2e3e9 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -4216,6 +4216,7 @@ static const vshCmdInfo info_pool_build[] = { static const vshCmdOptDef opts_pool_build[] = { {"pool", VSH_OT_DATA, VSH_OFLAG_REQ, N_("pool name or uuid")}, + {"overwrite", VSH_OT_BOOL, 0, gettext_noop("overwrite existing data")}, {NULL, 0, 0, NULL} }; @@ -4224,6 +4225,7 @@ cmdPoolBuild(vshControl *ctl, const vshCmd *cmd) { virStoragePoolPtr pool; int ret = TRUE; + int flags = 0; char *name; if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) @@ -4232,7 +4234,11 @@ cmdPoolBuild(vshControl *ctl, const vshCmd *cmd) if (!(pool = vshCommandOptPool(ctl, cmd, "pool", &name))) return FALSE; - if (virStoragePoolBuild(pool, 0) == 0) { + 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.6.5.5