Re: [PATCH] storage: volStorageBackendRBDRefreshVolInfo: refactor

[Ján Tomko <jtomko@xxxxxxxxxx>]

On a Friday in 2021, Yi Li wrote:
> use the ret variable for return value
>
> Signed-off-by: Yi Li <yili@xxxxxxxxxxx>
> ---
> src/storage/storage_backend_rbd.c | 22 ++++++----------------
> 1 file changed, 6 insertions(+), 16 deletions(-)
>
> diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c
> index 1630d6eede..22f5c78591 100644
> --- a/src/storage/storage_backend_rbd.c
> +++ b/src/storage/storage_backend_rbd.c
> @@ -507,36 +507,30 @@ volStorageBackendRBDRefreshVolInfo(virStorageVolDefPtr vol,
>                                    virStoragePoolObjPtr pool,
>                                    virStorageBackendRBDStatePtr ptr)
> {
> -    int rc, ret = -1;
> +    int ret = -1;
>     virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
>     rbd_image_t image = NULL;
>     rbd_image_info_t info;
>     uint64_t features;
>     uint64_t flags;
>
> -    if ((rc = rbd_open_read_only(ptr->ioctx, vol->name, &image, NULL)) < 0) {
> -        ret = rc;

The old version was repetitive, but it made it obvious that we want to
propagate the error value only on error.

More importantly, most of libvirt code only touches 'ret' twice:
* it is initialized to zero
* it is assigned to exactly once

That way there is no leftover success value that could be mistakenly
used if someone forgets to reset 'ret' to -1.

(The opposite case that could happen with the old approach if someone
were to forget to propagate the error is that a harmless error would not
be ignored, which is IMO less severe than ignoring a legitimate error)

Jano

> +    if ((ret = rbd_open_read_only(ptr->ioctx, vol->name, &image, NULL)) < 0) {
>         virReportSystemError(errno, _("failed to open the RBD image '%s'"),
>                              vol->name);
>         goto cleanup;
>     }
>
> -    if ((rc = rbd_stat(image, &info, sizeof(info))) < 0) {
> -        ret = rc;
> +    if ((ret = rbd_stat(image, &info, sizeof(info))) < 0) {
>         virReportSystemError(errno, _("failed to stat the RBD image '%s'"),
>                              vol->name);
>         goto cleanup;
>     }
>
> -    if ((rc = volStorageBackendRBDGetFeatures(image, vol->name, &features)) < 0) {
> -        ret = rc;
> +    if ((ret = volStorageBackendRBDGetFeatures(image, vol->name, &features)) < 0)
>         goto cleanup;
> -    }
>
> -    if ((rc = volStorageBackendRBDGetFlags(image, vol->name, &flags)) < 0) {
> -        ret = rc;
> +    if ((ret = volStorageBackendRBDGetFlags(image, vol->name, &flags)) < 0)
>         goto cleanup;
> -    }
>
>     vol->target.capacity = info.size;
>     vol->type = VIR_STORAGE_VOL_NETWORK;
> @@ -549,10 +543,8 @@ volStorageBackendRBDRefreshVolInfo(virStorageVolDefPtr vol,
>                   "Querying for actual allocation",
>                   def->source.name, vol->name);
>
> -        if ((rc = virStorageBackendRBDSetAllocation(vol, image, &info)) < 0) {
> -            ret = rc;
> +        if ((ret = virStorageBackendRBDSetAllocation(vol, image, &info)) < 0)
>             goto cleanup;
> -        }
>     } else {
>         vol->target.allocation = info.obj_size * info.num_objs;
>     }
> @@ -568,8 +560,6 @@ 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);
> --
> 2.25.3

Attachment: signature.asc
Description: PGP signature