[PATCH] storage: volStorageBackendRBDRefreshVolInfo: refactor

[Laine Stump]

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 at 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
>>
>>
>>
>>