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 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 <vsementsov@virtuozzo.com>
> ---
>   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.

>       assert(ret < 0);
>   
>       if (!iter->ret) {
> 

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