Re: [PATCH v7 21/21] nbd: assert that Error** is not NULL in nbd_iter_channel_error

[Vladimir Sementsov-Ogievskiy]

06.12.2019 11:54, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <address@hidden> writes:
> 
>> 05.12.2019 20:14, Eric Blake wrote:
>>> On 12/5/19 9:20 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> The local_err parameter is not here to return information about
>>>> nbd_iter_channel_error failure. Instead it's assumed to be filled when
>>>> passed to the function. This is already stressed by its name
>>>> (local_err, instead of classic errp). Stress it additionally by
>>>> assertion.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>>> ---
>>>>    block/nbd.c | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/block/nbd.c b/block/nbd.c
>>>> index 5f18f78a94..d085554f21 100644
>>>> --- a/block/nbd.c
>>>> +++ b/block/nbd.c
>>>> @@ -866,6 +866,7 @@ typedef struct NBDReplyChunkIter {
>>>>    static void nbd_iter_channel_error(NBDReplyChunkIter *iter,
>>>>                                       int ret, Error **local_err)
>>>>    {
>>>> +    assert(local_err && *local_err);
>>>
>>> Why are we forbidding grandparent callers from passing NULL when they want 
>>> to ignore an error?  We are called by several parent functions that get an 
>>> errp from the grandparent, and use this function to do some common grunt 
>>> work.  Let's look at the possibilities:
>>>
>>> 1. grandparent passes address of a local error: we want to append to the 
>>> error message, parent will then deal with the error how it wants (report 
>>> it, ignore it, propagate it, whatever)
>>>
>>> 2. grandparent passes &error_fatal: we want to append a hint, but before 
>>> ERRP_AUTO_PROPAGATE, the parent has already exited.  After 
>>> ERRP_AUTO_PROPAGATE, this looks like case 1.
>>>
>>> 3. grandparent passes &error_abort: we should never be reached (failure 
>>> earlier in the parent should have already aborted) - true whether or not 
>>> ERRP_AUTO_PROPAGATE is applied
>>>
>>> 4. grandparent passes NULL to ignore the error. Does not currently happen 
>>> in any of the grandparent callers, because if it did, your assertion in 
>>> this patch would now fire.  After ERRP_AUTO_PROPAGATE, this would look like 
>>> case 1.
>>>
>>> Would it be better to assert(!local_err || *local_err)?  The assertion as 
>>> written is too strict without ERRP_AUTO_PROPAGATE, but you get away with it 
>>> because none of the grandparents pass NULL; but is appropriate as written 
>>> for after after the macro conversion so then we wonder if churn on the 
>>> macro is worth it.
>>
>> We don't have any grandparents, this function is always called on local_err. 
>> And it's argument named local_err to stress it.
>> The function is an API to report error, and it wants filled local_err object.
>>
>> It will crash anyway if local_err is NULL, as it dereferences it.
> 
> Yes.
> 
> We already assert ret < 0 explicitly, and we rely on !local_err
> implicitly.  Making that explicit is obviously safe.
> 
> The code doesn't actually rely on !*local_err.  But when ret < 0, and
> !local_err, surely local_err should point to an Error object.  Asserting
> that makes sense to me.
> 
>> I just want to place an assertion at start of functions like this,
>> which will be easily recognizable by coccinelle.
> 
> That's not a convincing argument. 

That's why it is absent in commit message)

> Doesn't matter as long as we have
> convincing ones :)
> 
>>
>> ---
>>
>> We can improve the API, to support local_err==NULL, for the case when 
>> original request was called with
>> errp==NULL, but for this we'll need more changes, like, pass errp to 
>> NBD_FOREACH_REPLY_CHUNK and save
>> it into iter object...
>>
>> But how to detect it in code? Something like
>>
>>
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -1059,8 +1059,10 @@ static int 
>> nbd_co_receive_blockstatus_reply(BDRVNBDState *s,
>>            case NBD_REPLY_TYPE_BLOCK_STATUS:
>>                if (received) {
>>                    nbd_channel_error(s, -EINVAL);
>> -                error_setg(&local_err, "Several BLOCK_STATUS chunks in 
>> reply");
>> -                nbd_iter_channel_error(&iter, -EINVAL, &local_err);
>> +                if (errp) {
>> +                    error_setg(&local_err, "Several BLOCK_STATUS chunks in 
>> reply");
>> +                }
>> +                nbd_iter_channel_error(&iter, -EINVAL, errp ? &local_err : 
>> NULL);
>>                }
>>                received = true;
>>
>>
>> is ugly..
>>
>>
>> So, to support original errp=NULL the whole thing should be refactored.. Not 
>> worth it, I think.
> 
> The only change I'd consider in addition to the assertion is this
> simplification:
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 5f18f78a94..7bbac1e5b8 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -870,11 +870,9 @@ static void nbd_iter_channel_error(NBDReplyChunkIter 
> *iter,
>   
>       if (!iter->ret) {
>           iter->ret = ret;
> -        error_propagate(&iter->err, *local_err);
> -    } else {
> -        error_free(*local_err);
>       }
>   
> +    error_propagate(&iter->err, *local_err);

because it will just free the second argument if the first one already set..
OK, will add this.

>       *local_err = NULL;
>   }
>   
> 


-- 
Best regards,
Vladimir