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

[Eric Blake]

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 <address@hidden>

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