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

[Markus Armbruster]

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.  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);
     *local_err = NULL;
 }