On Mon, 06 Jul 2020 22:01:38 +0200 Markus Armbruster <arm...@redhat.com> wrote:
> Greg Kurz <gr...@kaod.org> writes: > > > On Mon, 6 Jul 2020 10:09:12 +0200 > > Markus Armbruster <arm...@redhat.com> wrote: > > > >> Convert uses like > >> > >> opts = qemu_opts_create(..., &err); > >> if (err) { > >> ... > >> } > >> > >> to > >> > >> opts = qemu_opts_create(..., &err); > > > > The patch doesn't strictly do that since it also converts &err to errp. > > Yes, and that's actually why I do it. I'll change the commit message to > say so: > > to > > opts = qemu_opts_create(..., errp); > Ok. > > This is okay because most of the changes also drop the associated > > error_propagate(), with the exception of block/parallels.c for which > > I had to check how local_err is used. As already noted by Vladimir > > earlier this generates an harmless "no-op error_propagate", but it > > could be worth mentioning that in the changelog for future reviews :) > > Yes, error_propagate() becomes a no-op for one out of three error paths > through it. There's similar "partial no-opification" elsewhere in this > series, notably in PATCH 36. > > Concrete suggestions for improving the commit message further are > welcome! > What about this ? The change in block/parallels.c doesn't provide any clue on the usage of local_err. As usual it is expected to be equal to NULL at the time qemu_opts_create() is called, and the goto on the error path jumps here: fail_options: error_propagate(errp, local_err); So, if qemu_opts_create() fails, we end up doing error_propagate(errp, NULL) which is a harmles no-op. > >> if (!opts) { > >> ... > >> } > >> > >> Eliminate error_propagate() that are now unnecessary. Delete @err > >> that are now unused. > >> > >> Signed-off-by: Markus Armbruster <arm...@redhat.com> > >> Reviewed-by: Eric Blake <ebl...@redhat.com> > >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> > >> --- > >> block/parallels.c | 4 ++-- > >> blockdev.c | 5 ++--- > >> qdev-monitor.c | 5 ++--- > >> util/qemu-config.c | 10 ++++------ > >> util/qemu-option.c | 12 ++++-------- > > > > Maybe some other potential candidates ? > > > > chardev/char.c: > > > > opts = qemu_opts_create(qemu_find_opts("chardev"), label, 1, &local_err); > > if (local_err) { > > error_report_err(local_err); > > return NULL; > > } > > > > monitor/hmp-cmds.c: > > > > opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &err); > > if (err) { > > goto out; > > } > > > > > > opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err); > > if (err) { > > goto end; > > } > > Don't fit my clarified commit message, because I can't replace &err by > errp there. > Sure. > I found these: > > diff --git a/block/blkdebug.c b/block/blkdebug.c > index 7194bc7f06..471b597dfe 100644 > --- a/block/blkdebug.c > +++ b/block/blkdebug.c > @@ -294,17 +294,13 @@ static int read_config(BDRVBlkdebugState *s, const > char *filename, > > d.s = s; > d.action = ACTION_INJECT_ERROR; > - qemu_opts_foreach(&inject_error_opts, add_rule, &d, &local_err); > - if (local_err) { > - error_propagate(errp, local_err); > + if (qemu_opts_foreach(&inject_error_opts, add_rule, &d, errp)) { > ret = -EINVAL; > goto fail; > } > > d.action = ACTION_SET_STATE; > - qemu_opts_foreach(&set_state_opts, add_rule, &d, &local_err); > - if (local_err) { > - error_propagate(errp, local_err); > + if (qemu_opts_foreach(&set_state_opts, add_rule, &d, errp)) { > ret = -EINVAL; > goto fail; > } > > However, I really need to get a pull request out... Can patch them > later. > Yeah and we might probably find a few more, but certainly not much after this colossal effort of yours. > > With or without the extra changes: > > > > Reviewed-by: Greg Kurz <gr...@kaod.org> > > Thanks! > My pleasure.