Re: [PATCH 07/46] error: Avoid more error_propagate() when error is not used here

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>
>> 24.06.2020 19:43, Markus Armbruster wrote:
>>> When all we do with an Error we receive into a local variable is
>>> propagating to somewhere else, we can just as well receive it there
>>> right away.  The previous commit did that for simple cases with
>>> Coccinelle.  Do it for a few more manually.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>   blockdev.c     |  5 +----
>>>   hw/core/numa.c | 44 ++++++++++++++------------------------------
>>>   qdev-monitor.c | 11 ++++-------
>>>   3 files changed, 19 insertions(+), 41 deletions(-)
>>>
>>> diff --git a/blockdev.c b/blockdev.c
>>> index b66863c42a..73736a4eaf 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -1009,13 +1009,10 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
>>> BlockInterfaceType block_default_type,
>>>       }
>>>         /* Actual block device init: Functionality shared with
>>> blockdev-add */
>>> -    blk = blockdev_init(filename, bs_opts, &local_err);
>>> +    blk = blockdev_init(filename, bs_opts, errp);
>>>       bs_opts = NULL;
>>>       if (!blk) {
>>> -        error_propagate(errp, local_err);
>>>           goto fail;
>>> -    } else {
>>> -        assert(!local_err);
>>>       }
>>>         /* Create legacy DriveInfo */
>>> diff --git a/hw/core/numa.c b/hw/core/numa.c
>>> index 5f81900f88..aa8c6be210 100644
>>> --- a/hw/core/numa.c
>>> +++ b/hw/core/numa.c
>>> @@ -449,40 +449,33 @@ void parse_numa_hmat_cache(MachineState *ms, 
>>> NumaHmatCacheOptions *node,
>>>     void set_numa_options(MachineState *ms, NumaOptions *object,
>>> Error **errp)
>>>   {
>>> -    Error *err = NULL;
>>> -
>>>       if (!ms->numa_state) {
>>>           error_setg(errp, "NUMA is not supported by this machine-type");
>>> -        goto end;
>>> +        return;
>>>       }
>>>         switch (object->type) {
>>>       case NUMA_OPTIONS_TYPE_NODE:
>>> -        parse_numa_node(ms, &object->u.node, &err);
>>> -        if (err) {
>>> -            goto end;
>>> -        }
>>> +        parse_numa_node(ms, &object->u.node, errp);
>>>           break;
>>
>> Could we use return here and and for other "break" operators here, to 
>> stress, that we
>> are not going to do something more in case of failure (as well as in case of
>> success)? To prevent the future addition of some code after the switch 
>> without
>> handling the error carefully here.
>
> Can do.

Second thoughts: I'd prefer not to mess with it now.

The sane way to add code after the switch is to make the
parse_numa_FOO() return bool, then bail out like this:

             if (!parse_numa_node(ms, &object->u.node, errp)) {
                 return;
             }

Too much for me right now.  I'm having a hard time getting this ready in
time of the freeze.  We can always improve on top.

[...]