[libvirt] [PATCH 1/6] storage: Add flags to allow building pool during create processing

[Michal Privoznik]

On 25.11.2015 20:11, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=830056
> 
> Add flags handling to the virStoragePoolCreate and virStoragePoolCreateXML
> API's which will allow the caller to provide the capability for the storage
> pool create API's to also perform a pool build during creation rather than
> requiring the additional buildPool step. This will allow transient pools
> to be defined, built, and started.
> 
> The new flags are:
> 
>     * VIR_STORAGE_POOL_CREATE_WITH_BUILD
>       Perform buildPool without any flags passed.
> 
>     * VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE
>       Perform buildPool using VIR_STORAGE_POOL_BUILD_OVERWRITE flag.
> 
>     * VIR_STORAGE_POOL_CREATE_WITH_BUILD_NO_OVERWRITE
>       Perform buildPool using VIR_STORAGE_POOL_BUILD_NO_OVERWRITE flag.
> 
> It is up to the backend to handle the processing of build flags. The
> overwrite and no-overwrite flags are mutually exclusive.
> 
> NB:
> This patch is loosely based upon code originally authored by Osier
> Yang that were not reviewed and pushed, see:
> 
> https://www.redhat.com/archives/libvir-list/2012-July/msg01328.html
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  include/libvirt/libvirt-storage.h | 16 ++++++++++++++-
>  src/libvirt-storage.c             |  4 ++--
>  src/storage/storage_driver.c      | 42 +++++++++++++++++++++++++++++++++++++--
>  3 files changed, 57 insertions(+), 5 deletions(-)
> 
> diff --git a/include/libvirt/libvirt-storage.h b/include/libvirt/libvirt-storage.h
> index 9fc3c2d..996a726 100644
> --- a/include/libvirt/libvirt-storage.h
> +++ b/include/libvirt/libvirt-storage.h
> @@ -57,7 +57,6 @@ typedef enum {
>  # endif
>  } virStoragePoolState;
>  
> -
>  typedef enum {
>      VIR_STORAGE_POOL_BUILD_NEW  = 0,   /* Regular build from scratch */
>      VIR_STORAGE_POOL_BUILD_REPAIR = (1 << 0), /* Repair / reinitialize */
> @@ -71,6 +70,21 @@ typedef enum {
>      VIR_STORAGE_POOL_DELETE_ZEROED = 1 << 0,  /* Clear all data to zeros (slow) */
>  } virStoragePoolDeleteFlags;
>  
> +typedef enum {
> +    VIR_STORAGE_POOL_CREATE_NORMAL = 0,
> +
> +    /* Create the pool and perform pool build without any flags */
> +    VIR_STORAGE_POOL_CREATE_WITH_BUILD = 1 << 0,
> +
> +    /* Create the pool and perform pool build using the
> +     * VIR_STORAGE_POOL_BUILD_OVERWRITE flag */
> +    VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE = 1 << 1,
> +
> +    /* Create the pool and perform pool build using the
> +     * VIR_STORAGE_POOL_BUILD_NO_OVERWRITE flag */
> +    VIR_STORAGE_POOL_CREATE_WITH_BUILD_NO_OVERWRITE = 1 << 2,

So _OVERWRITE and _NO_OVERWRITE flags are mutually exclusive then,
right? Probably worth noting.

> +} virStoragePoolCreateFlags;
> +
>  typedef struct _virStoragePoolInfo virStoragePoolInfo;
>  
>  struct _virStoragePoolInfo {
> diff --git a/src/libvirt-storage.c b/src/libvirt-storage.c
> index 66dd9f0..238a6cd 100644
> --- a/src/libvirt-storage.c
> +++ b/src/libvirt-storage.c
> @@ -505,7 +505,7 @@ virStoragePoolLookupByVolume(virStorageVolPtr vol)
>   * virStoragePoolCreateXML:
>   * @conn: pointer to hypervisor connection
>   * @xmlDesc: XML description for new pool
> - * @flags: extra flags; not used yet, so callers should always pass 0
> + * @flags: bitwise-OR of virStoragePoolCreateFlags
>   *
>   * Create a new storage based on its XML description. The
>   * pool is not persistent, so its definition will disappear
> @@ -670,7 +670,7 @@ virStoragePoolUndefine(virStoragePoolPtr pool)
>  /**
>   * virStoragePoolCreate:
>   * @pool: pointer to storage pool
> - * @flags: extra flags; not used yet, so callers should always pass 0
> + * @flags: bitwise-OR of virStoragePoolCreateFlags
>   *
>   * Starts an inactive storage pool
>   *
> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index bbf21f6..59a18bf 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -671,8 +671,11 @@ storagePoolCreateXML(virConnectPtr conn,
>      virStoragePoolPtr ret = NULL;
>      virStorageBackendPtr backend;
>      char *stateFile = NULL;
> +    unsigned int build_flags = 0;
>  
> -    virCheckFlags(0, NULL);
> +    virCheckFlags(VIR_STORAGE_POOL_CREATE_WITH_BUILD |
> +                  VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE |
> +                  VIR_STORAGE_POOL_CREATE_WITH_BUILD_NO_OVERWRITE, NULL);

How about VIR_EXCLUSIVE_FLAGS_RET(_OVERWRITE, _NO_OVERWRITE, NULL);

>  
>      storageDriverLock();
>      if (!(def = virStoragePoolDefParseString(xml)))
> @@ -694,6 +697,22 @@ storagePoolCreateXML(virConnectPtr conn,
>          goto cleanup;
>      def = NULL;
>  
> +    if (backend->buildPool) {
> +        if (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE)
> +            build_flags |= VIR_STORAGE_POOL_BUILD_OVERWRITE;
> +        else if (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD_NO_OVERWRITE)
> +            build_flags |= VIR_STORAGE_POOL_BUILD_NO_OVERWRITE;
> +
> +        if (build_flags ||
> +            (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD)) {

a-ha! So I need to pass _BUILD flag, and optionally one of _OVERWRITE
and _NO_OVERWRITE too. Okay.

> +            if (backend->buildPool(conn, pool, build_flags) < 0) {
> +                virStoragePoolObjRemove(&driver->pools, pool);
> +                pool = NULL;
> +                goto cleanup;
> +            }
> +        }
> +    }
> +
>      if (backend->startPool &&
>          backend->startPool(conn, pool) < 0) {
>          virStoragePoolObjRemove(&driver->pools, pool);
> @@ -845,8 +864,11 @@ storagePoolCreate(virStoragePoolPtr obj,
>      virStorageBackendPtr backend;
>      int ret = -1;
>      char *stateFile = NULL;
> +    unsigned int build_flags = 0;
>  
> -    virCheckFlags(0, -1);
> +    virCheckFlags(VIR_STORAGE_POOL_CREATE_WITH_BUILD |
> +                  VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE |
> +                  VIR_STORAGE_POOL_CREATE_WITH_BUILD_NO_OVERWRITE, NULL);

s/NULL/-1/. The function is returning an integer, not a pointer.

Plus it would be nice if we check the mutually exclusive flags here too.

>  
>      if (!(pool = virStoragePoolObjFromStoragePool(obj)))
>          return -1;
> @@ -864,6 +886,22 @@ storagePoolCreate(virStoragePoolPtr obj,
>          goto cleanup;
>      }
>  
> +    if (backend->buildPool) {
> +        if (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE)
> +            build_flags |= VIR_STORAGE_POOL_BUILD_OVERWRITE;
> +        else if (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD_NO_OVERWRITE)
> +            build_flags |= VIR_STORAGE_POOL_BUILD_NO_OVERWRITE;
> +
> +        if (build_flags ||
> +            (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD)) {
> +            if (backend->buildPool(obj->conn, pool, build_flags) < 0) {
> +                virStoragePoolObjRemove(&driver->pools, pool);
> +                pool = NULL;
> +                goto cleanup;
> +            }
> +        }
> +    }
> +
>      VIR_INFO("Starting up storage pool '%s'", pool->def->name);
>      if (backend->startPool &&
>          backend->startPool(obj->conn, pool) < 0)
> 

Otherwise looking good. ACK.

Michal