[libvirt] [PATCH 0/3] Cleanup/unify RBD backend context setup
While working on a way to create a luks volume on an RBD backend - I figured I'd clean up the initialization of the environment a bit. Rather than a lot of copy/paste code - use a common initializer. John Ferlan (3): rbd: Change virStorageBackendRBDCloseRADOSConn to be static void rbd: Change from static to alloc contexts rbd: Move the encryption check in build src/storage/storage_backend_rbd.c | 150 +++++++++++++++++++------------------- 1 file changed, 73 insertions(+), 77 deletions(-) -- 2.7.4
Since none of the callers check the status, let's just alter it from a (global!) int to be a static void. While we're at it - scrap the local runtime variable and just do the math in the VIR_DEBUG directly. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_rbd.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 21693c4..4e82232 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -205,29 +205,23 @@ virStorageBackendRBDOpenIoCTX(virStorageBackendRBDStatePtr ptr, return r; } -static int +static void virStorageBackendRBDCloseRADOSConn(virStorageBackendRBDStatePtr ptr) { - int ret = 0; - if (ptr->ioctx != NULL) { VIR_DEBUG("Closing RADOS IoCTX"); rados_ioctx_destroy(ptr->ioctx); - ret = -1; } ptr->ioctx = NULL; if (ptr->cluster != NULL) { VIR_DEBUG("Closing RADOS connection"); rados_shutdown(ptr->cluster); - ret = -2; } ptr->cluster = NULL; - time_t runtime = time(0) - ptr->starttime; - VIR_DEBUG("RADOS connection existed for %ld seconds", runtime); - - return ret; + VIR_DEBUG("RADOS connection existed for %ld seconds", + time(0) - ptr->starttime); } static int -- 2.7.4
On Mon, Sep 26, 2016 at 08:18:29 -0400, John Ferlan wrote:
Since none of the callers check the status, let's just alter it from a (global!) int to be a static void.
global?
While we're at it - scrap the local runtime variable and just do the math in the VIR_DEBUG directly.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_rbd.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)
ACK
Rather than use static/stack state context pointers, let's allocate and free the state context pointer. In doing so, we'll shrink the code a bit since many routines perform the same initialization sequence. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_rbd.c | 136 +++++++++++++++++++------------------- 1 file changed, 69 insertions(+), 67 deletions(-) diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 4e82232..37375c0 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -224,6 +224,42 @@ virStorageBackendRBDCloseRADOSConn(virStorageBackendRBDStatePtr ptr) time(0) - ptr->starttime); } + +static void +virStorageBackendRBDFreeStateContext(virStorageBackendRBDStatePtr *ptr) +{ + if (!*ptr) + return; + + virStorageBackendRBDCloseRADOSConn(*ptr); + + VIR_FREE(*ptr); +} + + +static virStorageBackendRBDStatePtr +virStorageBackendRBDAllocStateContext(virConnectPtr conn, + virStoragePoolObjPtr pool) +{ + virStorageBackendRBDStatePtr ptr; + + if (VIR_ALLOC(ptr) < 0) + return NULL; + + if (virStorageBackendRBDOpenRADOSConn(ptr, conn, &pool->def->source) < 0) + goto error; + + if (virStorageBackendRBDOpenIoCTX(ptr, pool) < 0) + goto error; + + return ptr; + + error: + virStorageBackendRBDFreeStateContext(&ptr); + return NULL; +} + + static int volStorageBackendRBDGetFeatures(rbd_image_t image, const char *volname, @@ -381,24 +417,19 @@ virStorageBackendRBDRefreshPool(virConnectPtr conn, int len = -1; int r = 0; char *name, *names = NULL; - virStorageBackendRBDState ptr; - ptr.cluster = NULL; - ptr.ioctx = NULL; + virStorageBackendRBDStatePtr ptr = NULL; struct rados_cluster_stat_t clusterstat; struct rados_pool_stat_t poolstat; - if (virStorageBackendRBDOpenRADOSConn(&ptr, conn, &pool->def->source) < 0) - goto cleanup; - - if (virStorageBackendRBDOpenIoCTX(&ptr, pool) < 0) + if (!(ptr = virStorageBackendRBDAllocStateContext(conn, pool))) goto cleanup; - if ((r = rados_cluster_stat(ptr.cluster, &clusterstat)) < 0) { + if ((r = rados_cluster_stat(ptr->cluster, &clusterstat)) < 0) { virReportSystemError(-r, "%s", _("failed to stat the RADOS cluster")); goto cleanup; } - if ((r = rados_ioctx_pool_stat(ptr.ioctx, &poolstat)) < 0) { + if ((r = rados_ioctx_pool_stat(ptr->ioctx, &poolstat)) < 0) { virReportSystemError(-r, _("failed to stat the RADOS pool '%s'"), pool->def->source.name); goto cleanup; @@ -417,7 +448,7 @@ virStorageBackendRBDRefreshPool(virConnectPtr conn, if (VIR_ALLOC_N(names, max_size) < 0) goto cleanup; - len = rbd_list(ptr.ioctx, names, &max_size); + len = rbd_list(ptr->ioctx, names, &max_size); if (len >= 0) break; if (len != -ERANGE) { @@ -443,7 +474,7 @@ virStorageBackendRBDRefreshPool(virConnectPtr conn, name += strlen(name) + 1; - r = volStorageBackendRBDRefreshVolInfo(vol, pool, &ptr); + r = volStorageBackendRBDRefreshVolInfo(vol, pool, ptr); /* It could be that a volume has been deleted through a different route * then libvirt and that will cause a -ENOENT to be returned. @@ -476,7 +507,7 @@ virStorageBackendRBDRefreshPool(virConnectPtr conn, cleanup: VIR_FREE(names); - virStorageBackendRBDCloseRADOSConn(&ptr); + virStorageBackendRBDFreeStateContext(&ptr); return ret; } @@ -566,9 +597,7 @@ virStorageBackendRBDDeleteVol(virConnectPtr conn, { int ret = -1; int r = 0; - virStorageBackendRBDState ptr; - ptr.cluster = NULL; - ptr.ioctx = NULL; + virStorageBackendRBDStatePtr ptr = NULL; virCheckFlags(VIR_STORAGE_VOL_DELETE_ZEROED | VIR_STORAGE_VOL_DELETE_WITH_SNAPSHOTS, -1); @@ -578,21 +607,18 @@ virStorageBackendRBDDeleteVol(virConnectPtr conn, if (flags & VIR_STORAGE_VOL_DELETE_ZEROED) VIR_WARN("%s", "This storage backend does not support zeroed removal of volumes"); - if (virStorageBackendRBDOpenRADOSConn(&ptr, conn, &pool->def->source) < 0) - goto cleanup; - - if (virStorageBackendRBDOpenIoCTX(&ptr, pool) < 0) + if (!(ptr = virStorageBackendRBDAllocStateContext(conn, pool))) goto cleanup; if (flags & VIR_STORAGE_VOL_DELETE_WITH_SNAPSHOTS) { - if (virStorageBackendRBDCleanupSnapshots(ptr.ioctx, &pool->def->source, + if (virStorageBackendRBDCleanupSnapshots(ptr->ioctx, &pool->def->source, vol) < 0) goto cleanup; } VIR_DEBUG("Removing volume %s/%s", pool->def->source.name, vol->name); - r = rbd_remove(ptr.ioctx, vol->name); + r = rbd_remove(ptr->ioctx, vol->name); if (r < 0 && (-r) != ENOENT) { virReportSystemError(-r, _("failed to remove volume '%s/%s'"), pool->def->source.name, vol->name); @@ -602,7 +628,7 @@ virStorageBackendRBDDeleteVol(virConnectPtr conn, ret = 0; cleanup: - virStorageBackendRBDCloseRADOSConn(&ptr); + virStorageBackendRBDFreeStateContext(&ptr); return ret; } @@ -648,9 +674,7 @@ virStorageBackendRBDBuildVol(virConnectPtr conn, virStorageVolDefPtr vol, unsigned int flags) { - virStorageBackendRBDState ptr; - ptr.cluster = NULL; - ptr.ioctx = NULL; + virStorageBackendRBDStatePtr ptr = NULL; int ret = -1; int r = 0; @@ -672,10 +696,7 @@ virStorageBackendRBDBuildVol(virConnectPtr conn, goto cleanup; } - if (virStorageBackendRBDOpenRADOSConn(&ptr, conn, &pool->def->source) < 0) - goto cleanup; - - if (virStorageBackendRBDOpenIoCTX(&ptr, pool) < 0) + if (!(ptr = virStorageBackendRBDAllocStateContext(conn, pool))) goto cleanup; if (vol->target.encryption != NULL) { @@ -684,7 +705,7 @@ virStorageBackendRBDBuildVol(virConnectPtr conn, goto cleanup; } - if ((r = virStorageBackendRBDCreateImage(ptr.ioctx, vol->name, + if ((r = virStorageBackendRBDCreateImage(ptr->ioctx, vol->name, vol->target.capacity)) < 0) { virReportSystemError(-r, _("failed to create volume '%s/%s'"), pool->def->source.name, @@ -695,7 +716,7 @@ virStorageBackendRBDBuildVol(virConnectPtr conn, ret = 0; cleanup: - virStorageBackendRBDCloseRADOSConn(&ptr); + virStorageBackendRBDFreeStateContext(&ptr); return ret; } @@ -1011,9 +1032,7 @@ virStorageBackendRBDBuildVolFrom(virConnectPtr conn, virStorageVolDefPtr origvol, unsigned int flags) { - virStorageBackendRBDState ptr; - ptr.cluster = NULL; - ptr.ioctx = NULL; + virStorageBackendRBDStatePtr ptr = NULL; int ret = -1; VIR_DEBUG("Creating clone of RBD image %s/%s with name %s", @@ -1021,19 +1040,17 @@ virStorageBackendRBDBuildVolFrom(virConnectPtr conn, virCheckFlags(0, -1); - if (virStorageBackendRBDOpenRADOSConn(&ptr, conn, &pool->def->source) < 0) + if (!(ptr = virStorageBackendRBDAllocStateContext(conn, pool))) goto cleanup; - if (virStorageBackendRBDOpenIoCTX(&ptr, pool) < 0) - goto cleanup; - - if ((virStorageBackendRBDCloneImage(ptr.ioctx, origvol->name, newvol->name)) < 0) + if ((virStorageBackendRBDCloneImage(ptr->ioctx, origvol->name, + newvol->name)) < 0) goto cleanup; ret = 0; cleanup: - virStorageBackendRBDCloseRADOSConn(&ptr); + virStorageBackendRBDFreeStateContext(&ptr); return ret; } @@ -1042,24 +1059,19 @@ virStorageBackendRBDRefreshVol(virConnectPtr conn, virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, virStorageVolDefPtr vol) { - virStorageBackendRBDState ptr; - ptr.cluster = NULL; - ptr.ioctx = NULL; + virStorageBackendRBDStatePtr ptr = NULL; int ret = -1; - if (virStorageBackendRBDOpenRADOSConn(&ptr, conn, &pool->def->source) < 0) - goto cleanup; - - if (virStorageBackendRBDOpenIoCTX(&ptr, pool) < 0) + if (!(ptr = virStorageBackendRBDAllocStateContext(conn, pool))) goto cleanup; - if (volStorageBackendRBDRefreshVolInfo(vol, pool, &ptr) < 0) + if (volStorageBackendRBDRefreshVolInfo(vol, pool, ptr) < 0) goto cleanup; ret = 0; cleanup: - virStorageBackendRBDCloseRADOSConn(&ptr); + virStorageBackendRBDFreeStateContext(&ptr); return ret; } @@ -1070,22 +1082,17 @@ virStorageBackendRBDResizeVol(virConnectPtr conn ATTRIBUTE_UNUSED, unsigned long long capacity, unsigned int flags) { - virStorageBackendRBDState ptr; - ptr.cluster = NULL; - ptr.ioctx = NULL; + virStorageBackendRBDStatePtr ptr = NULL; rbd_image_t image = NULL; int ret = -1; int r = 0; virCheckFlags(0, -1); - if (virStorageBackendRBDOpenRADOSConn(&ptr, conn, &pool->def->source) < 0) + if (!(ptr = virStorageBackendRBDAllocStateContext(conn, pool))) goto cleanup; - if (virStorageBackendRBDOpenIoCTX(&ptr, pool) < 0) - goto cleanup; - - if ((r = rbd_open(ptr.ioctx, vol->name, &image, NULL)) < 0) { + if ((r = rbd_open(ptr->ioctx, vol->name, &image, NULL)) < 0) { virReportSystemError(-r, _("failed to open the RBD image '%s'"), vol->name); goto cleanup; @@ -1102,7 +1109,7 @@ virStorageBackendRBDResizeVol(virConnectPtr conn ATTRIBUTE_UNUSED, cleanup: if (image != NULL) rbd_close(image); - virStorageBackendRBDCloseRADOSConn(&ptr); + virStorageBackendRBDFreeStateContext(&ptr); return ret; } @@ -1187,9 +1194,7 @@ virStorageBackendRBDVolWipe(virConnectPtr conn, unsigned int algorithm, unsigned int flags) { - virStorageBackendRBDState ptr; - ptr.cluster = NULL; - ptr.ioctx = NULL; + virStorageBackendRBDStatePtr ptr = NULL; rbd_image_t image = NULL; rbd_image_info_t info; uint64_t stripe_count; @@ -1200,13 +1205,10 @@ virStorageBackendRBDVolWipe(virConnectPtr conn, VIR_DEBUG("Wiping RBD image %s/%s", pool->def->source.name, vol->name); - if (virStorageBackendRBDOpenRADOSConn(&ptr, conn, &pool->def->source) < 0) - goto cleanup; - - if (virStorageBackendRBDOpenIoCTX(&ptr, pool) < 0) + if (!(ptr = virStorageBackendRBDAllocStateContext(conn, pool))) goto cleanup; - if ((r = rbd_open(ptr.ioctx, vol->name, &image, NULL)) < 0) { + if ((r = rbd_open(ptr->ioctx, vol->name, &image, NULL)) < 0) { virReportSystemError(-r, _("failed to open the RBD image %s"), vol->name); goto cleanup; @@ -1262,7 +1264,7 @@ virStorageBackendRBDVolWipe(virConnectPtr conn, if (image) rbd_close(image); - virStorageBackendRBDCloseRADOSConn(&ptr); + virStorageBackendRBDFreeStateContext(&ptr); return ret; } -- 2.7.4
On Mon, Sep 26, 2016 at 08:18:30 -0400, John Ferlan wrote: In subject: Static is not the same as stack allocated.
Rather than use static/stack state context pointers, let's allocate and
same here.
free the state context pointer. In doing so, we'll shrink the code a bit since many routines perform the same initialization sequence.
Fair point in removing duplicity, but you did not justify the change from stack allocated to heap allocated, just explained it.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_rbd.c | 136 +++++++++++++++++++------------------- 1 file changed, 69 insertions(+), 67 deletions(-)
diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 4e82232..37375c0 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -224,6 +224,42 @@ virStorageBackendRBDCloseRADOSConn(virStorageBackendRBDStatePtr ptr) time(0) - ptr->starttime); }
+ +static void +virStorageBackendRBDFreeStateContext(virStorageBackendRBDStatePtr *ptr)
The word "Context" seems a bit unnecessary in the name.
+{ + if (!*ptr) + return; + + virStorageBackendRBDCloseRADOSConn(*ptr); + + VIR_FREE(*ptr); +} + + +static virStorageBackendRBDStatePtr +virStorageBackendRBDAllocStateContext(virConnectPtr conn, + virStoragePoolObjPtr pool)
Same here. Also I think we prefer the word "New" instead of "Alloc"
+{ + virStorageBackendRBDStatePtr ptr; + + if (VIR_ALLOC(ptr) < 0) + return NULL;
ACK
No sense opening a connection only to fail because we don't support the type of build being attempted. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_rbd.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 37375c0..0d029c5 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -696,15 +696,15 @@ virStorageBackendRBDBuildVol(virConnectPtr conn, goto cleanup; } - if (!(ptr = virStorageBackendRBDAllocStateContext(conn, pool))) - goto cleanup; - if (vol->target.encryption != NULL) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("storage pool does not support encrypted volumes")); goto cleanup; } + if (!(ptr = virStorageBackendRBDAllocStateContext(conn, pool))) + goto cleanup; + if ((r = virStorageBackendRBDCreateImage(ptr->ioctx, vol->name, vol->target.capacity)) < 0) { virReportSystemError(-r, _("failed to create volume '%s/%s'"), -- 2.7.4
On Mon, Sep 26, 2016 at 08:18:31 -0400, John Ferlan wrote:
No sense opening a connection only to fail because we don't support the type of build being attempted.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_rbd.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
ACK
participants (2)
-
John Ferlan -
Peter Krempa