Re: [PATCH v6] nbd: well form nbd_iter_channel_error errp handler

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

29.11.2019 16:25, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> Make nbd_iter_channel_error errp handler well formed:
>> rename local_err to errp_in, as it is IN-parameter here (which is
>> unusual for Error**).
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>
>> v6: fix commit message
>>      add Eric's r-b
>>
>>   block/nbd.c | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/block/nbd.c b/block/nbd.c
>> index 5f18f78a94..345bf902e3 100644
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -864,18 +864,18 @@ typedef struct NBDReplyChunkIter {
>>   } NBDReplyChunkIter;
>>   
>>   static void nbd_iter_channel_error(NBDReplyChunkIter *iter,
>> -                                   int ret, Error **local_err)
>> +                                   int ret, Error **errp_in)
>>   {
>> -    assert(ret < 0);
>> +    assert(ret < 0 && errp_in && *errp_in);
>>   
>>       if (!iter->ret) {
>>           iter->ret = ret;
>> -        error_propagate(&iter->err, *local_err);
>> +        error_propagate(&iter->err, *errp_in);
>>       } else {
>> -        error_free(*local_err);
>> +        error_free(*errp_in);
>>       }
>>   
>> -    *local_err = NULL;
>> +    *errp_in = NULL;
> 
> This one is actually in/out.
> 
> If we use the convention
> 
>      Any Error ** parameter meant for passing an error to the caller must
>      be named @errp.  No other Error ** parameter may be named @errp.
> 
> then the old name is as good as the new one.  But the new one's "in"
> suggestion is misleading.
> 

Agreed. Do you have a suggestion how to rename errp in such cases (using
local_err in general will be misleading too)..

Maybe, "filled_errp" ? Seems too long..
"set_errp" is shorter, but no one will guess that this is the third form of the verb..

>>   }
>>   
>>   static void nbd_iter_request_error(NBDReplyChunkIter *iter, int ret)
> 


-- 
Best regards,
Vladimir