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

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



On 03/16/2010 06:22 AM, Daniel P. Berrange wrote:
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

Unfortunately, no. There is no programatic way to detect the presence of arbitrary data on a partition. Any attempt to do so is false security. We can, as you point out, determine in some cases that the user *is* going to overwrite something, but we cannot determine in all cases that the user *is not* going to overwrite something.

  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.

And that's the problem. We can detect filesystems *that we know of*. We do not and cannot know the universe of partition formats that exist. Again, if we pretend we do all we're going to do is give users a false sense of security when the only real security is asking the user "You're about to destroy data, are you certain you have the right partition?"

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.

We clearly disagree, so I think we need some additional voices to weigh in here.

Dave


 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


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