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 <vsement...@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) { >> > -- Best regards, Vladimir