[libvirt] [PATCH 2/2] Add fs pool formatting

[Eric Blake]

On 05/13/2010 08:18 PM, David Allan wrote:
> * This patch adds the ability to make the filesystem for a filesystem
>   pool during a pool build.
> 
>   The patch adds two new flags, probe and overwrite, to control when
>   mkfs gets executed.  By default, the patch preserves the current
>   behavior, i.e., if no flags are specified, pool build on a
>   filesystem pool only makes the directory on which the filesystem
>   will be mounted.
> 
>  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_PROBE = (1 << 2),  /* Probe for existing pool */
> +  VIR_STORAGE_POOL_BUILD_OVERWRITE = (1 << 3)  /* Overwrite data */

Can one specify both probe and overwrite, or are they mutually
exclusive?  Maybe it makes sense to allow both - don't overwrite the
filesystem if it is not already of the correct type, but if it is the
correct type, then erase it and start from scratch (contrasted with
probe in isolation doing nothing if the right type is present but
overwriting on all other types).

> +++ b/src/Makefile.am
> @@ -747,6 +747,11 @@ libvirt_driver_storage_la_LDFLAGS += -module -avoid-version
>  endif
>  libvirt_driver_storage_la_SOURCES += $(STORAGE_DRIVER_SOURCES)
>  libvirt_driver_storage_la_SOURCES += $(STORAGE_DRIVER_FS_SOURCES)
> +if HAVE_LIBBLKID
> +libvirt_driver_storage_la_SOURCES += storage/storage_backend_fs_libblkid.c
> +libvirt_driver_storage_la_CFLAGS += $(BLKID_CFLAGS)
> +libvirt_driver_storage_la_LDFLAGS += $(BLKID_LIBS)

Should be LIBADD, not LDFLAGS.

> +
> +static int
> +virStorageBackendMakeFileSystem(virStoragePoolObjPtr pool,
> +                                unsigned int flags)
> +{
> +    const char *device = NULL, *format = NULL;
> +    bool ok_to_mkfs = false;
> +    int ret = -1;

Missing virCheckFlags().

> +
> +    if (flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) {
> +        ok_to_mkfs = true;
> +    } else if (flags & VIR_STORAGE_POOL_BUILD_PROBE &&
> +               virStorageBackendFileSystemProbe(device, format) ==
> +               FILESYSTEM_PROBE_NOT_FOUND) {
> +        ok_to_mkfs = true;
> +    }

Here's where your logic would need to change - either the two flags are
mutually exclusive, and you need to error out if both are given, or the
two are both supported, but you need to take into account if both are
specified.

> --- /dev/null
> +++ b/src/storage/storage_backend_fs_libblkid.c
> @@ -0,0 +1,97 @@
> +/*
> + * storage_backend_fs_libblkid.c: libblkid specific code for
> + * filesystem backend
> + *
> + * Copyright (C) 2007-2010 Red Hat, Inc.

Is this just 2010, or did you really borrow significant content from
other files with earlier copyrights?  If so, list what file you borrowed
from.

> +
> +    /* Unfortunately this value comes from the pool definition
> +     * where it is correctly const char, but liblbkid expects it

s/liblbkid/libblkid/

> +     * to be char, thus the cast. */
> +    names[0] = (char *)format;
> +    names[1] = NULL;

Yeah, unfortunate but necessary.  Seems safe, though; libblkid has no
business modifying the string.

> diff --git a/src/util/virterror.c b/src/util/virterror.c
> index 96dd1e7..bd845fb 100644
> --- a/src/util/virterror.c
> +++ b/src/util/virterror.c
> @@ -1019,6 +1019,18 @@ virErrorMsg(virErrorNumber error, const char *info)
>              else
>                      errmsg = _("Storage volume not found: %s");
>              break;
> +        case VIR_ERR_STORAGE_PROBE_FAILED:
> +            if (info == NULL)
> +                    errmsg = _("Storage pool probe failed");
> +            else
> +                    errmsg = _("Storage pool probe failed: %s");
> +            break;

Goody - a merge conflict with my error cleanups.  It's a race to see who
gets first ACK :)

> +++ b/tools/virsh.c
> @@ -4668,6 +4668,9 @@ static const vshCmdInfo info_pool_build[] = {
> 
>  static const vshCmdOptDef opts_pool_build[] = {
>      {"pool", VSH_OT_DATA, VSH_OFLAG_REQ, N_("pool name or uuid")},
> +    {"probe", VSH_OT_BOOL, 0, N_("do not overwrite an existing "
> +                                 "pool of this type")},
> +    {"overwrite", VSH_OT_BOOL, 0, N_("overwrite any existing data")},

If we do allow both flags at once, then the wording of "probe" needs to
be tweaked.  So maybe it's better to make them mutually exclusive.

> @@ -4675,7 +4678,7 @@ static int
>  cmdPoolBuild(vshControl *ctl, const vshCmd *cmd)
>  {
>      virStoragePoolPtr pool;
> -    int ret = TRUE;
> +    int ret = TRUE, flags = 0;

Should flags be unsigned int?

Looks like we'll need a v2 patch before I feel good giving an ack.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20100514/7f9e0eb8/attachment-0001.sig>