[Top][All Lists]

[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: Eric Blake
Subject: Re: [PATCH v7 21/21] nbd: assert that Error** is not NULL in nbd_iter_channel_error
Date: Thu, 5 Dec 2019 11:49:54 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2

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

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 <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 
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 
               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 
+                }
+                nbd_iter_channel_error(&iter, -EINVAL, errp ? &local_err : 

No, that's not worth it.

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]