Re: [PATCH] storage: volStorageBackendRBDRefreshVolInfo: refactor
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(a)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
>
>
>
>