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@xxxxxxxxxx>
> 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list