[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 18:09:24 +0000 |
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 <address@hidden>
>
>>
>> ---
>>
>> 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.
>
--
Best regards,
Vladimir
- [PATCH v7 11/21] hw/i386/amd_iommu: rename Error ** parameter to more common errp, (continued)
- [PATCH v7 11/21] hw/i386/amd_iommu: rename Error ** parameter to more common errp, Vladimir Sementsov-Ogievskiy, 2019/12/05
- [PATCH v7 10/21] block/snapshot: rename Error ** parameter to more common errp, Vladimir Sementsov-Ogievskiy, 2019/12/05
- [PATCH v7 14/21] hw/s390x: rename Error ** parameter to more common errp, Vladimir Sementsov-Ogievskiy, 2019/12/05
- [PATCH v7 12/21] qga: rename Error ** parameter to more common errp, Vladimir Sementsov-Ogievskiy, 2019/12/05
- [PATCH v7 18/21] include/qom/object.h: rename Error ** parameter to more common errp, Vladimir Sementsov-Ogievskiy, 2019/12/05
- [PATCH v7 17/21] hw/usb: rename Error ** parameter to more common errp, Vladimir Sementsov-Ogievskiy, 2019/12/05
- [PATCH v7 21/21] nbd: assert that Error** is not NULL in nbd_iter_channel_error, Vladimir Sementsov-Ogievskiy, 2019/12/05
- Re: [PATCH v7 21/21] nbd: assert that Error** is not NULL in nbd_iter_channel_error, Markus Armbruster, 2019/12/06
- Re: [PATCH v7 21/21] nbd: assert that Error** is not NULL in nbd_iter_channel_error, Vladimir Sementsov-Ogievskiy, 2019/12/06
[PATCH v7 07/21] ppc: well form kvmppc_hint_smt_possible error hint helper, Vladimir Sementsov-Ogievskiy, 2019/12/05
[PATCH v7 15/21] hw/sd: drop extra whitespace in sdhci_sysbus_realize() header, Vladimir Sementsov-Ogievskiy, 2019/12/05
Re: [PATCH v7 00/21] error: prepare for auto propagated local_err, Cornelia Huck, 2019/12/05