[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Mon, Mar 15, 2010 at 05:21:26PM -0400, Dave Allan wrote:
> 
> 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.

There are two alternatives that are safe

 1. Do a per filesystem magic check on the volume in question. libvirt has
    a list of filesystems that I knows about "ext2", "ext3", "ext4", 
    "ufs", "iso9660", "udf", "gfs", "gfs2", "vfat", "hfs+", "xfs", "ocfs2"
    All of these will have an easily identified magic header that could
    be positively checked for.

Or

 2. Do a check for the first 512 or 4096 bytes being all zeros. This should
    detect the absence of any filesystem AFAIK.

The semantics we should have for these APIs are

 - virStoragePoolBuild(flags=0)   - format the filesystem/volgroup/partition table
                                    only if not already formatted.
 - virStoragePoolBuild(flags=OVERWRITE) - unconditionally format
 - virStoragePoolDelete()         - wipe data such that Build(flags=0) is guaranteed
                                    to be successful next time.

So I see several things that need to be implemented

 - Make disk & logical  pool backends check for existing formatted data
 - Implement the 'Delete' operation for all pool types, 
 - Add (checked) formatting of filesystem pools
 - Add  OVERWRITE flag to disk, logical, filesystem pool types to skip the check

> The attached patch implements this behavior.

NACK in this form.

> From cf05596823881448725cd67094f39c136144683b Mon Sep 17 00:00:00 2001
> From: David Allan <dallan redhat com>
> 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
> 

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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]