[PATCH] storage: volStorageBackendRBDRefreshVolInfo: refactor

Yi Li posted 1 patch 2 weeks, 3 days ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20210108064431.3142788-1-yili@winhong.com
src/storage/storage_backend_rbd.c | 22 ++++++----------------
1 file changed, 6 insertions(+), 16 deletions(-)

[PATCH] storage: volStorageBackendRBDRefreshVolInfo: refactor

Posted by Yi Li 2 weeks, 3 days ago
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;
+    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




Re: [PATCH] storage: volStorageBackendRBDRefreshVolInfo: refactor

Posted by Ján Tomko 2 weeks, 3 days ago
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
* 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
>
>
>
>

Re: [PATCH] storage: volStorageBackendRBDRefreshVolInfo: refactor

Posted by Laine Stump 2 weeks, 1 day ago
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
>>
>>
>>
>>

Re: [PATCH] storage: volStorageBackendRBDRefreshVolInfo: refactor

Posted by Michal Privoznik 2 weeks, 3 days ago
On 1/8/21 7:44 AM, 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(-)

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

and pushed.

Michal