9:04 a.m.
On 12/16/2015 08:18 AM, Michal Privoznik wrote:
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(a)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);
The disk and fs backends do check as well, but I can add it here.
>
> 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.
Right - if you want to build prior to start, then use one of the flags.
Leaving it up to the backend buildPool function to decide how to handle
the overwrite when that option isn't provided.
> + 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.
Ah right - dang that cut-n-paste...
Plus it would be nice if we check the mutually exclusive flags here
too.
Sure.
tks -
John
>
> 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