[libvirt] [PATCH 0/2] storage: fix rbd deprecations in ceph 14

Latest ceph has deprecated the rbd_list API and this is present in Fedora 30/31-rawhide Technically this is a build breaker fix, but the patches are complex enough that they would benefit from review before pushing. Daniel P. Berrangé (2): storage: split off code for calling rbd_list storage: add support for new rbd_list2 method m4/virt-storage-rbd.m4 | 1 + src/storage/storage_backend_rbd.c | 116 +++++++++++++++++++++++------- 2 files changed, 92 insertions(+), 25 deletions(-) -- 2.20.1

The rbd_list method has a quite unpleasant signature returning an array of strings in a single buffer instead of an array. It is being deprecated in favour of rbd_list2. To maintain clarity of code when supporting both APIs in parallel, split the rbd_list code out into a separate method. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/storage/storage_backend_rbd.c | 84 ++++++++++++++++++++++--------- 1 file changed, 59 insertions(+), 25 deletions(-) diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 3eae780c44..24ddbc8f69 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -565,19 +565,68 @@ volStorageBackendRBDRefreshVolInfo(virStorageVolDefPtr vol, return ret; } + +static char ** +virStorageBackendRBDGetVolNames(virStorageBackendRBDStatePtr ptr) +{ + char **names = NULL; + size_t nnames = 0; + int ret; + size_t max_size = 1024; + VIR_AUTOFREE(char *) namebuf = NULL; + const char *name; + + while (true) { + if (VIR_ALLOC_N(namebuf, max_size) < 0) + goto error; + + ret = rbd_list(ptr->ioctx, namebuf, &max_size); + if (ret >= 0) + break; + if (ret != -ERANGE) { + virReportSystemError(-ret, "%s", _("Unable to list RBD images")); + goto error; + } + VIR_FREE(namebuf); + } + + for (name = namebuf; name < namebuf + max_size;) { + VIR_AUTOFREE(char *) namedup = NULL; + + if (STREQ(name, "")) + break; + + if (VIR_STRDUP(namedup, name) < 0) + goto error; + + if (VIR_APPEND_ELEMENT(names, nnames, namedup) < 0) + goto error; + + name += strlen(name) + 1; + } + + if (VIR_EXPAND_N(names, nnames, 1) < 0) + goto error; + + return names; + + error: + virStringListFreeCount(names, nnames); + return NULL; +} + + static int virStorageBackendRBDRefreshPool(virStoragePoolObjPtr pool) { - size_t max_size = 1024; int ret = -1; - int len = -1; int r = 0; - char *name; virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); virStorageBackendRBDStatePtr ptr = NULL; struct rados_cluster_stat_t clusterstat; struct rados_pool_stat_t poolstat; - VIR_AUTOFREE(char *) names = NULL; + char **names = NULL; + size_t i; if (!(ptr = virStorageBackendRBDNewState(pool))) goto cleanup; @@ -602,33 +651,17 @@ virStorageBackendRBDRefreshPool(virStoragePoolObjPtr pool) def->source.name, clusterstat.kb, clusterstat.kb_avail, poolstat.num_bytes); - while (true) { - if (VIR_ALLOC_N(names, max_size) < 0) - goto cleanup; - - len = rbd_list(ptr->ioctx, names, &max_size); - if (len >= 0) - break; - if (len != -ERANGE) { - VIR_WARN("%s", "A problem occurred while listing RBD images"); - goto cleanup; - } - VIR_FREE(names); - } + if (!(names = virStorageBackendRBDGetVolNames(ptr))) + goto cleanup; - for (name = names; name < names + max_size;) { + for (i = 0; names[i] != NULL; i++) { VIR_AUTOPTR(virStorageVolDef) vol = NULL; - if (STREQ(name, "")) - break; - if (VIR_ALLOC(vol) < 0) goto cleanup; - if (VIR_STRDUP(vol->name, name) < 0) - goto cleanup; - - name += strlen(name) + 1; + vol->name = names[i]; + names[i] = NULL; r = volStorageBackendRBDRefreshVolInfo(vol, pool, ptr); @@ -659,6 +692,7 @@ virStorageBackendRBDRefreshPool(virStoragePoolObjPtr pool) ret = 0; cleanup: + virStringListFree(names); virStorageBackendRBDFreeState(&ptr); return ret; } -- 2.20.1

On Mon, Mar 18, 2019 at 12:20:17PM +0000, Daniel P. Berrangé wrote:
The rbd_list method has a quite unpleasant signature returning an array of strings in a single buffer instead of an array. It is being deprecated in favour of rbd_list2. To maintain clarity of code when supporting both APIs in parallel, split the rbd_list code out into a separate method.
Also change the VIR_WARN to report a proper error.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/storage/storage_backend_rbd.c | 84 ++++++++++++++++++++++--------- 1 file changed, 59 insertions(+), 25 deletions(-)
diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 3eae780c44..24ddbc8f69 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -565,19 +565,68 @@ volStorageBackendRBDRefreshVolInfo(virStorageVolDefPtr vol, return ret; }
+ +static char ** +virStorageBackendRBDGetVolNames(virStorageBackendRBDStatePtr ptr) +{ + char **names = NULL; + size_t nnames = 0; + int ret;
We usually use 'ret' for the return value of the current function and 'rc' for the values returned by callees.
+ size_t max_size = 1024; + VIR_AUTOFREE(char *) namebuf = NULL; + const char *name; + + while (true) { + if (VIR_ALLOC_N(namebuf, max_size) < 0) + goto error; + + ret = rbd_list(ptr->ioctx, namebuf, &max_size); + if (ret >= 0) + break; + if (ret != -ERANGE) { + virReportSystemError(-ret, "%s", _("Unable to list RBD images")); + goto error; + } + VIR_FREE(namebuf); + } + + for (name = namebuf; name < namebuf + max_size;) {
Strange, this line is winking at me.
+ VIR_AUTOFREE(char *) namedup = NULL; + + if (STREQ(name, "")) + break; + + if (VIR_STRDUP(namedup, name) < 0) + goto error; + + if (VIR_APPEND_ELEMENT(names, nnames, namedup) < 0) + goto error; + + name += strlen(name) + 1; + } +
[...]
- for (name = names; name < names + max_size;) { + for (i = 0; names[i] != NULL; i++) { VIR_AUTOPTR(virStorageVolDef) vol = NULL;
- if (STREQ(name, "")) - break; - if (VIR_ALLOC(vol) < 0) goto cleanup;
- if (VIR_STRDUP(vol->name, name) < 0) - goto cleanup; - - name += strlen(name) + 1;
+ vol->name = names[i]; + names[i] = NULL;
This can be: VIR_STEAL_PTR(vol->name, names[i]);
r = volStorageBackendRBDRefreshVolInfo(vol, pool, ptr);
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

The rbd_list method has been deprecated in Ceph >= 14.0.0 in favour of the new rbd_list2 method which populates an array of structs. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- m4/virt-storage-rbd.m4 | 1 + src/storage/storage_backend_rbd.c | 32 +++++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/m4/virt-storage-rbd.m4 b/m4/virt-storage-rbd.m4 index 17e2115309..f3d9d04908 100644 --- a/m4/virt-storage-rbd.m4 +++ b/m4/virt-storage-rbd.m4 @@ -33,6 +33,7 @@ AC_DEFUN([LIBVIRT_STORAGE_CHECK_RBD], [ old_LIBS="$LIBS" LIBS="$LIBS $LIBRBD_LIBS" AC_CHECK_FUNCS([rbd_get_features],[],[LIBRBD_FOUND=no]) + AC_CHECK_FUNCS([rbd_list2]) LIBS="$old_LIBS" fi diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 24ddbc8f69..b6f2785390 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -572,6 +572,33 @@ virStorageBackendRBDGetVolNames(virStorageBackendRBDStatePtr ptr) char **names = NULL; size_t nnames = 0; int ret; +#ifdef HAVE_RBD_LIST2 + rbd_image_spec_t *images = NULL; + size_t nimages = 16; + size_t i; + + while (true) { + if (VIR_ALLOC_N(images, nimages) < 0) + goto error; + + ret = rbd_list2(ptr->ioctx, images, &nimages); + if (ret >= 0) + break; + if (ret != -ERANGE) { + virReportSystemError(-ret, "%s", _("Unable to list RBD images")); + goto error; + } + } + + if (VIR_ALLOC_N(names, nimages + 1) < 0) + goto error; + nnames = nimages; + + for (i = 0; i < nimages; i++) { + names[i] = images->name; + images->name = NULL; + } +#else /* ! HAVE_RBD_LIST2 */ size_t max_size = 1024; VIR_AUTOFREE(char *) namebuf = NULL; const char *name; @@ -607,11 +634,16 @@ virStorageBackendRBDGetVolNames(virStorageBackendRBDStatePtr ptr) if (VIR_EXPAND_N(names, nnames, 1) < 0) goto error; +#endif /* ! HAVE_RBD_LIST2 */ return names; error: virStringListFreeCount(names, nnames); +#ifdef HAVE_RBD_LIST2 + rbd_image_spec_list_cleanup(images, nimages); + VIR_FREE(images); +#endif /* ! HAVE_RBD_LIST2 */ return NULL; } -- 2.20.1

On Mon, Mar 18, 2019 at 12:20:18PM +0000, Daniel P. Berrangé wrote:
The rbd_list method has been deprecated in Ceph >= 14.0.0 in favour of the new rbd_list2 method which populates an array of structs.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- m4/virt-storage-rbd.m4 | 1 + src/storage/storage_backend_rbd.c | 32 +++++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+)
diff --git a/m4/virt-storage-rbd.m4 b/m4/virt-storage-rbd.m4 index 17e2115309..f3d9d04908 100644 --- a/m4/virt-storage-rbd.m4 +++ b/m4/virt-storage-rbd.m4 @@ -33,6 +33,7 @@ AC_DEFUN([LIBVIRT_STORAGE_CHECK_RBD], [ old_LIBS="$LIBS" LIBS="$LIBS $LIBRBD_LIBS" AC_CHECK_FUNCS([rbd_get_features],[],[LIBRBD_FOUND=no]) + AC_CHECK_FUNCS([rbd_list2]) LIBS="$old_LIBS" fi
diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 24ddbc8f69..b6f2785390 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -572,6 +572,33 @@ virStorageBackendRBDGetVolNames(virStorageBackendRBDStatePtr ptr) char **names = NULL; size_t nnames = 0; int ret; +#ifdef HAVE_RBD_LIST2
Please, create two separate implementations: #ifdef HAVE_RBD_LIST2 static char ** virStorageBackendRBDGetVolNames(virStorageBackendRBDStatePtr ptr) { ... } #else /* ! HAVE_RBD_LIST2 */ virStorageBackendRBDGetVolNames(virStorageBackendRBDStatePtr ptr) { ... } #endif A bit more verbose, but easier to read.
+ rbd_image_spec_t *images = NULL; + size_t nimages = 16; + size_t i; + + while (true) { + if (VIR_ALLOC_N(images, nimages) < 0) + goto error; + + ret = rbd_list2(ptr->ioctx, images, &nimages); + if (ret >= 0) + break; + if (ret != -ERANGE) { + virReportSystemError(-ret, "%s", _("Unable to list RBD images")); + goto error; + } + } + + if (VIR_ALLOC_N(names, nimages + 1) < 0) + goto error; + nnames = nimages; + + for (i = 0; i < nimages; i++) { + names[i] = images->name; + images->name = NULL;
This can also use VIR_STEAL_PTR
+ } +#else /* ! HAVE_RBD_LIST2 */ size_t max_size = 1024; VIR_AUTOFREE(char *) namebuf = NULL;
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Daniel P. Berrangé
-
Ján Tomko