[libvirt] 'build' on FS pool now unconditionally formats?
Dave Allan
dallan at redhat.com
Tue Mar 16 15:01:10 UTC 2010
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 at 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
More information about the libvir-list
mailing list