| Message ID | 20191205174635.18758-22-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | error: prepare for auto propagated local_err | expand |
On 12/5/19 11:46 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(+) Our timing resulted in crossed mails - I was replying to v7 when this landed, and my reply there is still relevant (namely, a better commit message is needed, but the code gets my R-b with that change). > > 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) { >
Eric Blake <eblake@redhat.com> writes: > On 12/5/19 11:46 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(+) > > Our timing resulted in crossed mails - I was replying to v7 when this > landed, and my reply there is still relevant (namely, a better commit > message is needed, but the code gets my R-b with that change). If v8 turns out to be fine except for commit message tweaks, I'll gladly to these in my tree. Just tell me what to do in a reply to this message.
06.12.2019 18:58, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> On 12/5/19 11:46 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(+) >> >> Our timing resulted in crossed mails - I was replying to v7 when this >> landed, and my reply there is still relevant (namely, a better commit >> message is needed, but the code gets my R-b with that change). > > If v8 turns out to be fine except for commit message tweaks, I'll gladly > to these in my tree. Just tell me what to do in a reply to this > message. > Yes, this would be great, thanks! The only thing is your suggestion on patch 21, but it may be applied in separate (and it's actually a separate thing)
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: > 06.12.2019 18:58, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> On 12/5/19 11:46 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(+) >>> >>> Our timing resulted in crossed mails - I was replying to v7 when this >>> landed, and my reply there is still relevant (namely, a better commit >>> message is needed, but the code gets my R-b with that change). >> >> If v8 turns out to be fine except for commit message tweaks, I'll gladly >> to these in my tree. Just tell me what to do in a reply to this >> message. >> > > Yes, this would be great, thanks! > > The only thing is your suggestion on patch 21, but it may be applied in separate (and it's actually a separate thing) It's closer to idea than to suggestion, and it's separate. Commit message in my tree: nbd: assert that Error** is not NULL in nbd_iter_channel_error 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 propagate 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 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. Also: Reviewed-by: Markus Armbruster <armbru@redhat.com>
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) {
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(+)