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

Message ID 20191205152019.8454-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, 3:20 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:14 p.m. UTC | #1
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) {
>
Vladimir Sementsov-Ogievskiy Dec. 5, 2019, 5:39 p.m. UTC | #2
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.
>>
>> 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.

We don't have any grandparents, this function is always called on local_err. And it's argument named local_err to stress it.
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.

---

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);
              }
              received = true;


is ugly..


So, to support original errp=NULL the whole thing should be refactored.. Not worth it, I think.


> 
>>       assert(ret < 0);
>>       if (!iter->ret) {
>>
>
Eric Blake Dec. 5, 2019, 5:49 p.m. UTC | #3
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.
Vladimir Sementsov-Ogievskiy Dec. 5, 2019, 6:09 p.m. UTC | #4
05.12.2019 20:49, Eric Blake wrote:
> 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. 

Hmm, not to append details but to report the error to the magical receiving loop, which doesn't yet know about the 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.


So, how about simply s/to append details to that error/to report that error/ ?

> 
>> 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 Dec. 5, 2019, 7:56 p.m. UTC | #5
On 12/5/19 12:09 PM, Vladimir Sementsov-Ogievskiy wrote:

>>
>> 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.
> 
> Hmm, not to append details but to report the error to the magical receiving loop, which doesn't yet know about the 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.
> 
> 
> So, how about simply s/to append details to that error/to report that error/ ?

That should be fine.
Markus Armbruster Dec. 6, 2019, 8:54 a.m. UTC | #6
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 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.
>>>
>>> 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.
>
> We don't have any grandparents, this function is always called on local_err. And it's argument named local_err to stress it.
> 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.

Yes.

We already assert ret < 0 explicitly, and we rely on !local_err
implicitly.  Making that explicit is obviously safe.

The code doesn't actually rely on !*local_err.  But when ret < 0, and
!local_err, surely local_err should point to an Error object.  Asserting
that makes sense to me.

> I just want to place an assertion at start of functions like this,
> which will be easily recognizable by coccinelle.

That's not a convincing argument.  Doesn't matter as long as we have
convincing ones :)

>
> ---
>
> 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);
>               }
>               received = true;
>
>
> is ugly..
>
>
> So, to support original errp=NULL the whole thing should be refactored.. Not worth it, I think.

The only change I'd consider in addition to the assertion is this
simplification:

diff --git a/block/nbd.c b/block/nbd.c
index 5f18f78a94..7bbac1e5b8 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -870,11 +870,9 @@ static void nbd_iter_channel_error(NBDReplyChunkIter *iter,
 
     if (!iter->ret) {
         iter->ret = ret;
-        error_propagate(&iter->err, *local_err);
-    } else {
-        error_free(*local_err);
     }
 
+    error_propagate(&iter->err, *local_err);
     *local_err = NULL;
 }
Vladimir Sementsov-Ogievskiy Dec. 6, 2019, 10:26 a.m. UTC | #7
06.12.2019 11:54, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> 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.
>>>>
>>>> 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.
>>
>> We don't have any grandparents, this function is always called on local_err. And it's argument named local_err to stress it.
>> 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.
> 
> Yes.
> 
> We already assert ret < 0 explicitly, and we rely on !local_err
> implicitly.  Making that explicit is obviously safe.
> 
> The code doesn't actually rely on !*local_err.  But when ret < 0, and
> !local_err, surely local_err should point to an Error object.  Asserting
> that makes sense to me.
> 
>> I just want to place an assertion at start of functions like this,
>> which will be easily recognizable by coccinelle.
> 
> That's not a convincing argument. 

That's why it is absent in commit message)

> Doesn't matter as long as we have
> convincing ones :)
> 
>>
>> ---
>>
>> 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);
>>                }
>>                received = true;
>>
>>
>> is ugly..
>>
>>
>> So, to support original errp=NULL the whole thing should be refactored.. Not worth it, I think.
> 
> The only change I'd consider in addition to the assertion is this
> simplification:
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 5f18f78a94..7bbac1e5b8 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -870,11 +870,9 @@ static void nbd_iter_channel_error(NBDReplyChunkIter *iter,
>   
>       if (!iter->ret) {
>           iter->ret = ret;
> -        error_propagate(&iter->err, *local_err);
> -    } else {
> -        error_free(*local_err);
>       }
>   
> +    error_propagate(&iter->err, *local_err);

because it will just free the second argument if the first one already set..
OK, will add this.

>       *local_err = NULL;
>   }
>   
>

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