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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 6/24/20 11:43 AM, 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);
>
> Loses an assertion that blockdev_init() doesn't mis-use errp, but I
> think the goal of your cleanup work has been to make it easier to
> prove any function follows the rules, so the assertion doesn't add
> much at this point.

This is an early use of Error in the block layer.  Back then, we were
concerned about functions that are supposed to either return a value or
set an error doing both or none.  Since then, we've tacitly decided such
programming mistakes are too unlikely to be worth littering the code
with assertions.  Error handling is tediously verbose even without them.

Since this series is about making it somewhat less tediously verbose...

>> +++ 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);
>
> Another such case.
>
>>           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",
>
> Should we s/can not/cannot/ while touching this?

The longer and the more mechanical the patch, the less willing I am to
include "while we're there" improvements.  This one may still be okay.
But you pointed out a few more error message improvements in later
reviews.  Perhaps collecting them all into an omnibus error message
patch would be better.

> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!