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

[Eric Blake <eblake@redhat.com>]

On 12/5/19 11:39 AM, Vladimir Sementsov-Ogievskiy wrote:
> 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.
>>>

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

Then the commit message should state that. How about:

All callers of nbd_iter_channel_error() pass the address of a local_err 
variable, and only call this function if an error has already occurred, 
using this function to append details to that error.  This is already 
implied by its name (local_err instead of the classic errp), but it is 
worth additionally stressing this by adding an assertion to make it part 
of the function contract.

> 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.
> 
> I just want to place an assertion at start of functions like this,
> which will be easily recognizable by coccinelle.

With an improved commit message, the assertion makes sense, so

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> ---
> 
> 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);

No, that's not worth it.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org