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

[Markus Armbruster]

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);

> 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!

>>     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.

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.

> With or without the extra changes:
>
> Reviewed-by: Greg Kurz <groug@kaod.org>

Thanks!