Re: [libvirt PATCH] Revert "storage: volStorageBackendRBDRefreshVolInfo: refactor"

[Laine Stump <laine@xxxxxxxxxx>]

On 1/8/21 8:00 AM, Ján Tomko wrote:
> Only set 'ret' once in any given execution path instead of mixing
> it with intermediate return values.
>
> This reverts commit 453bdebe5dcc3ec87d4db011e4f657fa24e42d94


I agree that we shouldn't be repeatedly setting ret as is done by the 
patch you're reverting, but maybe instead of reverting that patch, it 
would be better to just remove the "ret =" from all the function calls, 
and add a "ret = 0;" just before the cleanup label. (the caller doesn't 
know or care exactly what values could be returned from these rbd_* 
functions, it just cares it the return is < 0 (error) or not (success).


> Signed-off-by: Ján Tomko <jtomko@xxxxxxxxxx>
> ---
>   src/storage/storage_backend_rbd.c | 22 ++++++++++++++++------
>   1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c
> index 22f5c78591..1630d6eede 100644
> --- a/src/storage/storage_backend_rbd.c
> +++ b/src/storage/storage_backend_rbd.c
> @@ -507,30 +507,36 @@ volStorageBackendRBDRefreshVolInfo(virStorageVolDefPtr vol,
>                                      virStoragePoolObjPtr pool,
>                                      virStorageBackendRBDStatePtr ptr)
>   {
> -    int ret = -1;
> +    int rc, ret = -1;
>       virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
>       rbd_image_t image = NULL;
>       rbd_image_info_t info;
>       uint64_t features;
>       uint64_t flags;
>   
> -    if ((ret = rbd_open_read_only(ptr->ioctx, vol->name, &image, NULL)) < 0) {
> +    if ((rc = rbd_open_read_only(ptr->ioctx, vol->name, &image, NULL)) < 0) {
> +        ret = rc;
>           virReportSystemError(errno, _("failed to open the RBD image '%s'"),
>                                vol->name);
>           goto cleanup;
>       }
>   
> -    if ((ret = rbd_stat(image, &info, sizeof(info))) < 0) {
> +    if ((rc = rbd_stat(image, &info, sizeof(info))) < 0) {
> +        ret = rc;
>           virReportSystemError(errno, _("failed to stat the RBD image '%s'"),
>                                vol->name);
>           goto cleanup;
>       }
>   
> -    if ((ret = volStorageBackendRBDGetFeatures(image, vol->name, &features)) < 0)
> +    if ((rc = volStorageBackendRBDGetFeatures(image, vol->name, &features)) < 0) {
> +        ret = rc;
>           goto cleanup;
> +    }
>   
> -    if ((ret = volStorageBackendRBDGetFlags(image, vol->name, &flags)) < 0)
> +    if ((rc = volStorageBackendRBDGetFlags(image, vol->name, &flags)) < 0) {
> +        ret = rc;
>           goto cleanup;
> +    }
>   
>       vol->target.capacity = info.size;
>       vol->type = VIR_STORAGE_VOL_NETWORK;
> @@ -543,8 +549,10 @@ volStorageBackendRBDRefreshVolInfo(virStorageVolDefPtr vol,
>                     "Querying for actual allocation",
>                     def->source.name, vol->name);
>   
> -        if ((ret = virStorageBackendRBDSetAllocation(vol, image, &info)) < 0)
> +        if ((rc = virStorageBackendRBDSetAllocation(vol, image, &info)) < 0) {
> +            ret = rc;
>               goto cleanup;
> +        }
>       } else {
>           vol->target.allocation = info.obj_size * info.num_objs;
>       }
> @@ -560,6 +568,8 @@ volStorageBackendRBDRefreshVolInfo(virStorageVolDefPtr vol,
>       VIR_FREE(vol->key);
>       vol->key = g_strdup_printf("%s/%s", def->source.name, vol->name);
>   
> +    ret = 0;
> +
>    cleanup:
>       if (image)
>           rbd_close(image);