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

Re: [libvirt] [PATCH 2/3] storage: Add fs pool formatting



On Wed, Aug 31, 2011 at 10:34:47PM +0800, Osier Yang wrote:
> This patch adds the ability to make the filesystem for a filesystem
> pool during a pool build.
> 
> The patch adds two new flags, no overwrite 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.
> 
> If the no overwrite flag is specified, the target device is checked
> to determine if a filesystem of the type specified in the pool is
> present.  If a filesystem of that type is already present, mkfs is
> not executed and the build call returns an error.  Otherwise, mkfs
> is executed and any data present on the device is overwritten.
> 
> If the overwrite flag is specified, mkfs is always executed, and any
> existing data on the target device is overwritten unconditionally.
> ---
>  include/libvirt/libvirt.h.in     |    6 +-
>  include/libvirt/virterror.h      |    2 +
>  src/Makefile.am                  |    4 +
>  src/libvirt.c                    |    4 +
>  src/storage/storage_backend_fs.c |  188 +++++++++++++++++++++++++++++++++++++-
>  src/storage/storage_backend_fs.h |    5 +
>  src/util/virterror.c             |   12 +++
>  7 files changed, 215 insertions(+), 6 deletions(-)
> 
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index c51a5b9..92e1d62 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -1666,8 +1666,10 @@ 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_NO_OVERWRITE = (1 << 2),  /* Do not overwrite existing pool */
> +  VIR_STORAGE_POOL_BUILD_OVERWRITE = (1 << 3),  /* Overwrite data */
>  } virStoragePoolBuildFlags;
>  
>  typedef enum {
> diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
> index c270b54..0aae622 100644
> --- a/include/libvirt/virterror.h
> +++ b/include/libvirt/virterror.h
> @@ -235,6 +235,8 @@ typedef enum {
>      VIR_ERR_INVALID_STREAM = 73,        /* stream pointer not valid */
>      VIR_ERR_ARGUMENT_UNSUPPORTED = 74,  /* valid API use but unsupported by
>                                             the given driver */
> +    VIR_ERR_STORAGE_PROBE_FAILED = 75,  /* storage pool proble failed */
> +    VIR_ERR_STORAGE_POOL_BUILT = 76,    /* storage pool already built */
>  } virErrorNumber;
>  
>  /**
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 322c900..14f09e4 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -954,6 +954,10 @@ endif
>  if WITH_SECDRIVER_APPARMOR
>  libvirt_driver_storage_la_LIBADD += $(APPARMOR_LIBS)
>  endif
> +if HAVE_LIBBLKID
> +libvirt_driver_storage_la_CFLAGS += $(BLKID_CFLAGS)
> +libvirt_driver_storage_la_LIBADD += $(BLKID_LIBS)
> +endif
>  if WITH_STORAGE_DIR
>  if WITH_DRIVER_MODULES
>  mod_LTLIBRARIES += libvirt_driver_storage.la
> diff --git a/src/libvirt.c b/src/libvirt.c
> index e4a21b6..b876b1c 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -10466,6 +10466,10 @@ error:
>   * virStoragePoolBuild:
>   * @pool: pointer to storage pool
>   * @flags: future flags, use 0 for now
> + * @flags: flags to control pool build behaviour
> + *
> + * Currently only filesystem pool accepts flags VIR_STORAGE_POOL_BUILD_OVERWRITE
> + * and VIR_STORAGE_POOL_BUILD_NO_OVERWRITE.
>   *
>   * Build the underlying storage pool
>   *
> diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
> index 4f53d3f..36f1c60 100644
> --- a/src/storage/storage_backend_fs.c
> +++ b/src/storage/storage_backend_fs.c
> @@ -37,6 +37,10 @@
>  #include <libxml/tree.h>
>  #include <libxml/xpath.h>
>  
> +#if HAVE_LIBBLKID
> +# include <blkid/blkid.h>
> +#endif
> +
>  #include "virterror_internal.h"
>  #include "storage_backend_fs.h"
>  #include "storage_conf.h"
> @@ -45,6 +49,7 @@
>  #include "memory.h"
>  #include "xml.h"
>  #include "virfile.h"
> +#include "logging.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_STORAGE
>  
> @@ -534,13 +539,172 @@ virStorageBackendFileSystemStart(virConnectPtr conn ATTRIBUTE_UNUSED,
>  }
>  #endif /* WITH_STORAGE_FS */
>  
> +#if HAVE_LIBBLKID
> +static virStoragePoolProbeResult
> +virStorageBackendFileSystemProbe(const char *device,
> +                                 const char *format) {
> +
> +    virStoragePoolProbeResult ret = FILESYSTEM_PROBE_ERROR;
> +    blkid_probe probe = NULL;
> +    const char *fstype = NULL;
> +    char *names[2], *libblkid_format = NULL;
> +
> +    VIR_DEBUG("Probing for existing filesystem of type %s on device %s",
> +              format, device);
> +
> +    if (blkid_known_fstype(format) == 0) {
> +        virStorageReportError(VIR_ERR_STORAGE_PROBE_FAILED,
> +                              _("Not capable of probing for "
> +                                "filesystem of type %s"),
> +                              format);
> +        goto error;
> +    }
> +
> +    probe = blkid_new_probe_from_filename(device);
> +    if (probe == NULL) {
> +        virStorageReportError(VIR_ERR_STORAGE_PROBE_FAILED,
> +                                  _("Failed to create filesystem probe "
> +                                  "for device %s"),
> +                                  device);
> +        goto error;
> +    }
> +
> +    if ((libblkid_format = strdup(format)) == NULL) {
> +        virReportOOMError();
> +        goto error;
> +    }
> +
> +    names[0] = libblkid_format;
> +    names[1] = NULL;
> +
> +    blkid_probe_filter_superblocks_type(probe,
> +                                        BLKID_FLTR_ONLYIN,
> +                                        names);
> +
> +    if (blkid_do_probe(probe) != 0) {
> +        VIR_INFO("No filesystem of type '%s' found on device '%s'",
> +                 format, device);
> +        ret = FILESYSTEM_PROBE_NOT_FOUND;
> +    } else if (blkid_probe_lookup_value(probe, "TYPE", &fstype, NULL) == 0) {
> +        virStorageReportError(VIR_ERR_STORAGE_POOL_BUILT,
> +                              _("Existing filesystem of type '%s' found on "
> +                                "device '%s'"),
> +                              fstype, device);
> +        ret = FILESYSTEM_PROBE_FOUND;
> +    }
> +
> +    if (blkid_do_probe(probe) != 1) {
> +        virStorageReportError(VIR_ERR_STORAGE_PROBE_FAILED,
> +                                  _("Found additional probes to run, "
> +                                    "filesystem probing may be incorrect"));
> +        ret = FILESYSTEM_PROBE_ERROR;
> +    }
> +
> +error:
> +    VIR_FREE(libblkid_format);
> +
> +    if (probe != NULL) {
> +        blkid_free_probe(probe);
> +    }
> +
> +    return ret;
> +}
> +
> +#else /* #if HAVE_LIBBLKID */
> +
> +static virStoragePoolProbeResult
> +virStorageBackendFileSystemProbe(const char *device ATTRIBUTE_UNUSED,
> +                                 const char *format ATTRIBUTE_UNUSED)
> +{
> +    virStorageReportError(VIR_ERR_OPERATION_INVALID,
> +                          _("probing for filesystems is unsupported "
> +                            "by this build"));
> +
> +    return FILESYSTEM_PROBE_ERROR;
> +}
> +
> +#endif /* #if HAVE_LIBBLKID */
> +
> +static int
> +virStorageBackendExecuteMKFS(const char *device,
> +                             const char *format)
> +{
> +    int ret = 0;
> +    virCommandPtr cmd = NULL;
> +
> +    cmd = virCommandNewArgList(MKFS,
> +                               "-t",
> +                               format,
> +                               device,
> +                               NULL);
> +
> +    if (virCommandRun(cmd, NULL) < 0) {
> +        virReportSystemError(errno,
> +                             _("Failed to make filesystem of "
> +                               "type '%s' on device '%s'"),
> +                             format, device);
> +        ret = -1;
> +    }
> +    return ret;
> +}
> +
> +static int
> +virStorageBackendMakeFileSystem(virStoragePoolObjPtr pool,
> +                                unsigned int flags)
> +{
> +    const char *device = NULL, *format = NULL;
> +    bool ok_to_mkfs = false;
> +    int ret = -1;
> +
> +    if (pool->def->source.devices == NULL) {
> +        virStorageReportError(VIR_ERR_OPERATION_INVALID,
> +                              _("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);
> +
> +    if (!virFileExists(device)) {
> +        virStorageReportError(VIR_ERR_OPERATION_INVALID,
> +                              _("Source device does not exist when formatting pool '%s'"),
> +                              pool->def->name);
> +        goto error;
> +    }
> +
> +    if (flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) {
> +        ok_to_mkfs = true;
> +    } else if (flags & VIR_STORAGE_POOL_BUILD_NO_OVERWRITE &&
> +               virStorageBackendFileSystemProbe(device, format) ==
> +               FILESYSTEM_PROBE_NOT_FOUND) {
> +        ok_to_mkfs = true;
> +    }
> +
> +    if (ok_to_mkfs) {
> +        ret = virStorageBackendExecuteMKFS(device, format);
> +    }
> +
> +error:
> +    return ret;
> +}
> +
>  
>  /**
>   * @conn connection to report errors against
>   * @pool storage pool to build
> + * @flags controls the pool formating behaviour
>   *
>   * Build a directory or FS based storage pool.
>   *
> + * If no flag is set, it only makes the directory; If
> + * VIR_STORAGE_POOL_BUILD_NO_OVERWRITE set, it probes to determine if
> + * filesystem already exists on the target device, renurning an error
> + * if exists, or using mkfs to format the target device if not; If
> + * VIR_STORAGE_POOL_BUILD_OVERWRITE is set, mkfs is always executed,
> + * any existed data on the target device is overwritten unconditionally.
> + *
>   *  - If it is a FS based pool, mounts the unlying source device on the pool
>   *
>   * Returns 0 on success, -1 on error
> @@ -551,10 +715,20 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED,
>                                   unsigned int flags)
>  {
>      int err, ret = -1;
> -    char *parent;
> -    char *p;
> +    char *parent = NULL;
> +    char *p = NULL;
>  
> -    virCheckFlags(0, -1);
> +    virCheckFlags(VIR_STORAGE_POOL_BUILD_OVERWRITE |
> +                  VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, ret);
> +
> +    if (flags == (VIR_STORAGE_POOL_BUILD_OVERWRITE |
> +                  VIR_STORAGE_POOL_BUILD_NO_OVERWRITE)) {
> +
> +        virStorageReportError(VIR_ERR_OPERATION_INVALID,
> +                              _("Overwrite and no overwrite flags"
> +                                " are mutually exclusive"));
> +        goto error;
> +    }
>  
>      if ((parent = strdup(pool->def->target.path)) == NULL) {
>          virReportOOMError();
> @@ -604,7 +778,13 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED,
>              goto error;
>          }
>      }
> -    ret = 0;
> +
> +    if (flags != 0) {
> +        ret = virStorageBackendMakeFileSystem(pool, flags);
> +    } else {
> +        ret = 0;
> +    }
> +
>  error:
>      VIR_FREE(parent);
>      return ret;
> diff --git a/src/storage/storage_backend_fs.h b/src/storage/storage_backend_fs.h
> index 7def53e..54c6739 100644
> --- a/src/storage/storage_backend_fs.h
> +++ b/src/storage/storage_backend_fs.h
> @@ -29,6 +29,11 @@
>  # if WITH_STORAGE_FS
>  extern virStorageBackend virStorageBackendFileSystem;
>  extern virStorageBackend virStorageBackendNetFileSystem;
> +typedef enum {
> +    FILESYSTEM_PROBE_FOUND,
> +    FILESYSTEM_PROBE_NOT_FOUND,
> +    FILESYSTEM_PROBE_ERROR,
> +} virStoragePoolProbeResult;
>  # endif
>  extern virStorageBackend virStorageBackendDirectory;
>  
> diff --git a/src/util/virterror.c b/src/util/virterror.c
> index b50fbfd..26c4981 100644
> --- a/src/util/virterror.c
> +++ b/src/util/virterror.c
> @@ -1030,6 +1030,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;
> +        case VIR_ERR_STORAGE_POOL_BUILT:
> +            if (info == NULL)
> +                errmsg = _("Storage pool already built");
> +            else
> +                errmsg = _("Storage pool already built: %s");
> +            break;
>          case VIR_ERR_INVALID_STORAGE_POOL:
>              if (info == NULL)
>                  errmsg = _("invalid storage pool pointer in");

  ACK the logic looks right. I would love to see some ways to test
the various behaviours though,

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


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