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@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