[v8,21/21] nbd: assert that Error** is not NULL in nbd_iter_channel_error
diff mbox series

Message ID 20191205174635.18758-22-vsementsov@virtuozzo.com
State New
Headers show
Series
  • error: prepare for auto propagated local_err
Related show

Commit Message

Vladimir Sementsov-Ogievskiy Dec. 5, 2019, 5:46 p.m. UTC
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(+)

Comments

Eric Blake Dec. 5, 2019, 5:51 p.m. UTC | #1
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) {
>
Markus Armbruster Dec. 6, 2019, 3:58 p.m. UTC | #2
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.
Vladimir Sementsov-Ogievskiy Dec. 6, 2019, 6:19 p.m. UTC | #3
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)
Markus Armbruster Dec. 10, 2019, 9:05 a.m. UTC | #4
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>

Patch
diff mbox series

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