[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v7 21/21] nbd: assert that Error** is not NULL in nbd_iter_ch
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [PATCH v7 21/21] nbd: assert that Error** is not NULL in nbd_iter_channel_error |
Date: |
Thu, 5 Dec 2019 17:39:21 +0000 |
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 <address@hidden>
>> ---
>> 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) {
>>
>
--
Best regards,
Vladimir
Re: [PATCH v7 00/21] error: prepare for auto propagated local_err, Cornelia Huck, 2019/12/05