7:18 a.m.
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
7:18 a.m.
New subject: [libvirt] [PATCH 1/3] rbd: Change virStorageBackendRBDCloseRADOSConn to be static void
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(a)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
3:08 a.m.
New subject: [libvirt] [PATCH 1/3] rbd: Change virStorageBackendRBDCloseRADOSConn to be static void
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(a)redhat.com>
---
src/storage/storage_backend_rbd.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
ACK
7:18 a.m.
New subject: [libvirt] [PATCH 2/3] rbd: Change from static to alloc contexts
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(a)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
6:39 a.m.
New subject: [libvirt] [PATCH 2/3] rbd: Change from static to alloc contexts
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(a)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
7:18 a.m.
New subject: [libvirt] [PATCH 3/3] rbd: Move the encryption check in build
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(a)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
3:04 a.m.
New subject: [libvirt] [PATCH 3/3] rbd: Move the encryption check in build
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(a)redhat.com>
---
src/storage/storage_backend_rbd.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
ACK
3001
days inactive
3010
days old
6 comments
2 participants
participants (2)
-
John Ferlan -
Peter Krempa