| Message ID | 20191127190840.15773-1-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v6] nbd: well form nbd_iter_channel_error errp handler | expand |
[adding Markus] On 11/27/19 1:08 PM, Vladimir Sementsov-Ogievskiy wrote: > Make nbd_iter_channel_error errp handler well formed: > rename local_err to errp_in, as it is IN-parameter here (which is > unusual for Error**). > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > --- > > v6: fix commit message > add Eric's r-b I'm surprised that you aren't including Markus on a lot of these patches - even though you've posted a lot of them as separate threads to make them easier for individual maintainers to pick up, it would also be possible for Markus to pick up a bunch of them at once through his error tree. At any rate, I'll queue this one through my NBD tree for 5.0 if it does not make it through Markus' error tree or the trivial tree sooner.
27.11.2019 22:49, Eric Blake wrote: > [adding Markus] > > On 11/27/19 1:08 PM, Vladimir Sementsov-Ogievskiy wrote: >> Make nbd_iter_channel_error errp handler well formed: >> rename local_err to errp_in, as it is IN-parameter here (which is >> unusual for Error**). >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> --- >> >> v6: fix commit message >> add Eric's r-b > > I'm surprised that you aren't including Markus on a lot of these patches - even though you've posted a lot of them as separate threads to make them easier for individual maintainers to pick up, it would also be possible for Markus to pick up a bunch of them at once through his error tree. Oops, you are right, I'm sorry :( If it helps, all patches Eric saying about are 21 patches from myself, with v6 tag, sent during last two hours. Sorry that I didn't answer to https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg03105.html before sending them, and I don't want do it now, to not move v5 above v6. Week passed since my proposal at that link, so having one against (Eric) and two for (Kevin and Greg), I decided to follow my plan now. > > At any rate, I'll queue this one through my NBD tree for 5.0 if it does not make it through Markus' error tree or the trivial tree sooner. > > Thanks!
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: > Make nbd_iter_channel_error errp handler well formed: > rename local_err to errp_in, as it is IN-parameter here (which is > unusual for Error**). > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > --- > > v6: fix commit message > add Eric's r-b > > block/nbd.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/block/nbd.c b/block/nbd.c > index 5f18f78a94..345bf902e3 100644 > --- a/block/nbd.c > +++ b/block/nbd.c > @@ -864,18 +864,18 @@ typedef struct NBDReplyChunkIter { > } NBDReplyChunkIter; > > static void nbd_iter_channel_error(NBDReplyChunkIter *iter, > - int ret, Error **local_err) > + int ret, Error **errp_in) > { > - assert(ret < 0); > + assert(ret < 0 && errp_in && *errp_in); > > if (!iter->ret) { > iter->ret = ret; > - error_propagate(&iter->err, *local_err); > + error_propagate(&iter->err, *errp_in); > } else { > - error_free(*local_err); > + error_free(*errp_in); > } > > - *local_err = NULL; > + *errp_in = NULL; This one is actually in/out. If we use the convention Any Error ** parameter meant for passing an error to the caller must be named @errp. No other Error ** parameter may be named @errp. then the old name is as good as the new one. But the new one's "in" suggestion is misleading. > } > > static void nbd_iter_request_error(NBDReplyChunkIter *iter, int ret)
29.11.2019 16:25, Markus Armbruster wrote: > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: > >> Make nbd_iter_channel_error errp handler well formed: >> rename local_err to errp_in, as it is IN-parameter here (which is >> unusual for Error**). >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> --- >> >> v6: fix commit message >> add Eric's r-b >> >> block/nbd.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/block/nbd.c b/block/nbd.c >> index 5f18f78a94..345bf902e3 100644 >> --- a/block/nbd.c >> +++ b/block/nbd.c >> @@ -864,18 +864,18 @@ typedef struct NBDReplyChunkIter { >> } NBDReplyChunkIter; >> >> static void nbd_iter_channel_error(NBDReplyChunkIter *iter, >> - int ret, Error **local_err) >> + int ret, Error **errp_in) >> { >> - assert(ret < 0); >> + assert(ret < 0 && errp_in && *errp_in); >> >> if (!iter->ret) { >> iter->ret = ret; >> - error_propagate(&iter->err, *local_err); >> + error_propagate(&iter->err, *errp_in); >> } else { >> - error_free(*local_err); >> + error_free(*errp_in); >> } >> >> - *local_err = NULL; >> + *errp_in = NULL; > > This one is actually in/out. > > If we use the convention > > Any Error ** parameter meant for passing an error to the caller must > be named @errp. No other Error ** parameter may be named @errp. > > then the old name is as good as the new one. But the new one's "in" > suggestion is misleading. > Agreed. Do you have a suggestion how to rename errp in such cases (using local_err in general will be misleading too).. Maybe, "filled_errp" ? Seems too long.. "set_errp" is shorter, but no one will guess that this is the third form of the verb.. >> } >> >> static void nbd_iter_request_error(NBDReplyChunkIter *iter, int ret) >
diff --git a/block/nbd.c b/block/nbd.c index 5f18f78a94..345bf902e3 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -864,18 +864,18 @@ typedef struct NBDReplyChunkIter { } NBDReplyChunkIter; static void nbd_iter_channel_error(NBDReplyChunkIter *iter, - int ret, Error **local_err) + int ret, Error **errp_in) { - assert(ret < 0); + assert(ret < 0 && errp_in && *errp_in); if (!iter->ret) { iter->ret = ret; - error_propagate(&iter->err, *local_err); + error_propagate(&iter->err, *errp_in); } else { - error_free(*local_err); + error_free(*errp_in); } - *local_err = NULL; + *errp_in = NULL; } static void nbd_iter_request_error(NBDReplyChunkIter *iter, int ret)