Re: [PATCH v3 06/44] qemu-option: Check return value instead of @err where convenient

[Greg Kurz]

On Mon, 06 Jul 2020 22:01:38 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Greg Kurz <groug@kaod.org> writes:
> 
> > On Mon,  6 Jul 2020 10:09:12 +0200
> > Markus Armbruster <armbru@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 <armbru@redhat.com>
> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@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 <groug@kaod.org>
> 
> Thanks!
> 

My pleasure.