On 1/8/21 7:59 AM, Ján Tomko wrote:

On a Friday in 2021, Yi Li wrote:

...

use the ret variable for return value

Signed-off-by: Yi Li <yili@winhong.com> --- 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

That didn't sound right, so I looked it up - grep says that we have 112 instances of ret initialized to 0, but 1479 instances of ret initialized to -1. Was that a typo?

* 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.

... a statement which makes sense (to me) only if you actually meant to say "initialized to -1". (And yes, I agree with your reasoning.)

(The opposite case that could happen with the old approachif 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)

I'm not quite parsing everything you're saying here (maybe because it's 1AM Sunday morning :-), but I agree it's better the way it was. But even better than your proposal to revert this patch would be to carry it even further - remove the "ret =" from each rbd function call, and then add a "ret = 0;" just before the cleanup label. (I just suggested this in my response to your "revert" patch).

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