[v8] error: prepare for auto propagated local_err

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

Posted by Vladimir Sementsov-Ogievskiy 5 years, 11 months ago

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);
     assert(ret < 0);
 
     if (!iter->ret) {
-- 
2.21.0

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

Posted by Eric Blake 5 years, 11 months ago

On 12/5/19 11:46 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(+)

Our timing resulted in crossed mails - I was replying to v7 when this 
landed, and my reply there is still relevant (namely, a better commit 
message is needed, but the code gets my R-b with that change).

> 
> 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);
>       assert(ret < 0);
>   
>       if (!iter->ret) {
> 

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

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

Posted by Markus Armbruster 5 years, 11 months ago

Eric Blake <eblake@redhat.com> writes:

> On 12/5/19 11:46 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(+)
>
> Our timing resulted in crossed mails - I was replying to v7 when this
> landed, and my reply there is still relevant (namely, a better commit
> message is needed, but the code gets my R-b with that change).

If v8 turns out to be fine except for commit message tweaks, I'll gladly
to these in my tree.  Just tell me what to do in a reply to this
message.

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

Posted by Vladimir Sementsov-Ogievskiy 5 years, 11 months ago

06.12.2019 18:58, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 12/5/19 11:46 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(+)
>>
>> Our timing resulted in crossed mails - I was replying to v7 when this
>> landed, and my reply there is still relevant (namely, a better commit
>> message is needed, but the code gets my R-b with that change).
> 
> If v8 turns out to be fine except for commit message tweaks, I'll gladly
> to these in my tree.  Just tell me what to do in a reply to this
> message.
> 

Yes, this would be great, thanks!

The only thing is your suggestion on patch 21, but it may be applied in separate (and it's actually a separate thing)

-- 
Best regards,
Vladimir

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

Posted by Markus Armbruster 5 years, 11 months ago

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 06.12.2019 18:58, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> On 12/5/19 11:46 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(+)
>>>
>>> Our timing resulted in crossed mails - I was replying to v7 when this
>>> landed, and my reply there is still relevant (namely, a better commit
>>> message is needed, but the code gets my R-b with that change).
>> 
>> If v8 turns out to be fine except for commit message tweaks, I'll gladly
>> to these in my tree.  Just tell me what to do in a reply to this
>> message.
>> 
>
> Yes, this would be great, thanks!
>
> The only thing is your suggestion on patch 21, but it may be applied in separate (and it's actually a separate thing)

It's closer to idea than to suggestion, and it's separate.

Commit message in my tree:

    nbd: assert that Error** is not NULL in nbd_iter_channel_error
    
    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 propagate 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 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.

Also:
Reviewed-by: Markus Armbruster <armbru@redhat.com>