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

[Markus Armbruster]

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.

>>       case NUMA_OPTIONS_TYPE_DIST:
>> -        parse_numa_distance(ms, &object->u.dist, &err);
>> -        if (err) {
>> -            goto end;
>> -        }
>> +        parse_numa_distance(ms, &object->u.dist, errp);
>>           break;
>>       case NUMA_OPTIONS_TYPE_CPU:
>>           if (!object->u.cpu.has_node_id) {
>> -            error_setg(&err, "Missing mandatory node-id property");
>> -            goto end;
>> +            error_setg(errp, "Missing mandatory node-id property");
>> +            return;
>>           }
>>           if (!ms->numa_state->nodes[object->u.cpu.node_id].present) {
>> -            error_setg(&err, "Invalid node-id=%" PRId64 ", NUMA node must 
>> be "
>> -                "defined with -numa node,nodeid=ID before it's used with "
>> -                "-numa cpu,node-id=ID", object->u.cpu.node_id);
>> -            goto end;
>> +            error_setg(errp, "Invalid node-id=%" PRId64 ", NUMA node must 
>> be "
>> +                       "defined with -numa node,nodeid=ID before it's used 
>> with "
>> +                       "-numa cpu,node-id=ID", object->u.cpu.node_id);
>> +            return;
>>           }
>>   -        machine_set_cpu_numa_node(ms,
>> qapi_NumaCpuOptions_base(&object->u.cpu),
>> -                                  &err);
>> +        machine_set_cpu_numa_node(ms,
>> +                                  qapi_NumaCpuOptions_base(&object->u.cpu),
>> +                                  errp);
>>           break;
>>       case NUMA_OPTIONS_TYPE_HMAT_LB:
>>           if (!ms->numa_state->hmat_enabled) {
>> @@ -492,10 +485,7 @@ void set_numa_options(MachineState *ms, NumaOptions 
>> *object, Error **errp)
>>               return;
>>           }
>>   -        parse_numa_hmat_lb(ms->numa_state, &object->u.hmat_lb,
>> &err);
>> -        if (err) {
>> -            goto end;
>> -        }
>> +        parse_numa_hmat_lb(ms->numa_state, &object->u.hmat_lb, errp);
>>           break;
>>       case NUMA_OPTIONS_TYPE_HMAT_CACHE:
>>           if (!ms->numa_state->hmat_enabled) {
>> @@ -505,17 +495,11 @@ void set_numa_options(MachineState *ms, NumaOptions 
>> *object, Error **errp)
>>               return;
>>           }
>>   -        parse_numa_hmat_cache(ms, &object->u.hmat_cache, &err);
>> -        if (err) {
>> -            goto end;
>> -        }
>> +        parse_numa_hmat_cache(ms, &object->u.hmat_cache, errp);
>>           break;
>>       default:
>>           abort();
>>       }
>> -
>> -end:
>> -    error_propagate(errp, err);
>>   }
>>     static int parse_numa(void *opaque, QemuOpts *opts, Error
>> **errp)
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index e38030429b..40c34bb9cf 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -600,7 +600,6 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error 
>> **errp)
>>       const char *driver, *path;
>>       DeviceState *dev = NULL;
>>       BusState *bus = NULL;
>> -    Error *err = NULL;
>>       bool hide;
>>         driver = qemu_opt_get(opts, "driver");
>> @@ -655,15 +654,14 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error 
>> **errp)
>>       dev = qdev_new(driver);
>>         /* Check whether the hotplug is allowed by the machine */
>> -    if (qdev_hotplug && !qdev_hotplug_allowed(dev, &err)) {
>> +    if (qdev_hotplug && !qdev_hotplug_allowed(dev, errp)) {
>>           /* Error must be set in the machine hook */
>> -        assert(err);
>>           goto err_del_dev;
>>       }
>>         if (!bus && qdev_hotplug &&
>> !qdev_get_machine_hotplug_handler(dev)) {
>>           /* No bus, no machine hotplug handler --> device is not 
>> hotpluggable */
>> -        error_setg(&err, "Device '%s' can not be hotplugged on this 
>> machine",
>> +        error_setg(errp, "Device '%s' can not be hotplugged on this 
>> machine",
>>                      driver);
>>           goto err_del_dev;
>>       }
>> @@ -671,19 +669,18 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error 
>> **errp)
>>       qdev_set_id(dev, qemu_opts_id(opts));
>>         /* set properties */
>> -    if (qemu_opt_foreach(opts, set_property, dev, &err)) {
>> +    if (qemu_opt_foreach(opts, set_property, dev, errp)) {
>
> Here is an example, what I was afraid, when we discussed introducing a lot 
> more
> bool functions (true is success).
>
> Here are two functions with different semantics, and it looks a bit weird,
> one if (func()) and one if (!func()). Still "goto err" makes it obvious
> that it's all about error checking.
>
> I don't remember, did we considered a convention to avoid if (func()) to check
> errors, and use instead if (func() < 0) for such case? So here, update it to 
> be
>
> if (qemu_opt_foreach(opts, set_property, dev, errp) < 0)

qemu_opt_foreach()'s contract:

  /**
   * For each member of @opts, call @func(@opaque, name, value, @errp).
   * @func() may store an Error through @errp, but must return non-zero then.
   * When @func() returns non-zero, break the loop and return that value.
   * Return zero when the loop completes.
   */

Thus, < 0 would be wrong.

Contracts can be changed, of course.  We can make qemu_opt_foreach()
return true on success, and false on failure.  Confusing unless we
change the callback's contract as well, and that requires updating all
the callbacks.  Feasible, just work.

> (I don't insist to do it exactly in this patch, as its aim is another, I just
>  want to remind about this problem)

Appreciated.

>>           goto err_del_dev;
>>       }
>>         dev->opts = opts;
>> -    if (!qdev_realize(DEVICE(dev), bus, &err)) {
>> +    if (!qdev_realize(DEVICE(dev), bus, errp)) {
>>           dev->opts = NULL;
>>           goto err_del_dev;
>>       }
>>       return dev;
>>     err_del_dev:
>> -    error_propagate(errp, err);
>>       if (dev) {
>>           object_unparent(OBJECT(dev));
>>           object_unref(OBJECT(dev));
>>
>
>
> Also, suggest a squash-in, I've noted during previous patch review:
>
> diff --git a/backends/cryptodev-vhost-user.c b/backends/cryptodev-vhost-user.c
> index dbe5a8aae6..3cdc406b0d 100644
> --- a/backends/cryptodev-vhost-user.c
> +++ b/backends/cryptodev-vhost-user.c
> @@ -184,15 +184,13 @@ static void cryptodev_vhost_user_init(
>  {
>      int queues = backend->conf.peers.queues;
>      size_t i;
> -    Error *local_err = NULL;
>      Chardev *chr;
>      CryptoDevBackendClient *cc;
>      CryptoDevBackendVhostUser *s =
>                        CRYPTODEV_BACKEND_VHOST_USER(backend);
>  -    chr = cryptodev_vhost_claim_chardev(s, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> +    chr = cryptodev_vhost_claim_chardev(s, errp);
> +    if (!chr) {
>          return;
>      }

I like this change, but I'd rather not squash it into this patch,
because this patch is about doing exactly the things the previous patch
does with Coccinelle for cases where the Coccinelle script doesn't
match.

In particular, the patch does not switch from "use @local_err to check
for failure" to "use return value to check for failure".

The closest match is probably "[PATCH 10/46] qemu-option: Check return
value instead of @err where convenient", but that one's strictly about
qemu-options.

Perhaps a future series could transform more calls of functions that
return non-null on succes, null on failure.  How can we identify them?
False negatives would be tolerable, false positives absolutely not.