Re: [PATCH v6] hw/core/qdev: cleanup Error ** variables

[Vladimir Sementsov-Ogievskiy]

29.11.2019 9:07, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <address@hidden> writes:
> 
>> Rename Error ** parameter in check_only_migratable to common errp.
>>
>> In device_set_realized:
>>
>>   - Move "if (local_err != NULL)" closer to error setters.
>>
>>   - Drop 'Error **local_errp': it doesn't save any LoCs, but it's very
>>     unusual.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> Reviewed-by: Eric Blake <address@hidden>
>> Reviewed-by: Marc-André Lureau <address@hidden>
>> ---
>>
>> v6: enhance grammar in comment [Eric]
>>      add r-b by Eric and Marc-André
>>
>>   hw/core/qdev.c | 28 +++++++++++++---------------
>>   1 file changed, 13 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index cf1ba28fe3..82d3ee590a 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -820,12 +820,12 @@ static bool device_get_realized(Object *obj, Error 
>> **errp)
>>       return dev->realized;
>>   }
>>   
>> -static bool check_only_migratable(Object *obj, Error **err)
>> +static bool check_only_migratable(Object *obj, Error **errp)
>>   {
>>       DeviceClass *dc = DEVICE_GET_CLASS(obj);
>>   
>>       if (!vmstate_check_only_migratable(dc->vmsd)) {
>> -        error_setg(err, "Device %s is not migratable, but "
>> +        error_setg(errp, "Device %s is not migratable, but "
>>                      "--only-migratable was specified",
>>                      object_get_typename(obj));
>>           return false;
>> @@ -874,10 +874,9 @@ static void device_set_realized(Object *obj, bool 
>> value, Error **errp)
>>   
>>           if (dc->realize) {
>>               dc->realize(dev, &local_err);
>> -        }
>> -
>> -        if (local_err != NULL) {
>> -            goto fail;
>> +            if (local_err != NULL) {
>> +                goto fail;
>> +            }
>>           }
> 
> Yes, this is the more conventional usage.
> 
>>   
>>           DEVICE_LISTENER_CALL(realize, Forward, dev);
>> @@ -918,27 +917,26 @@ static void device_set_realized(Object *obj, bool 
>> value, Error **errp)
>>          }
>>   
>>       } else if (!value && dev->realized) {
>> -        Error **local_errp = NULL;
>> +        /* We want local_err to track only the first error */
>>           QLIST_FOREACH(bus, &dev->child_bus, sibling) {
>> -            local_errp = local_err ? NULL : &local_err;
>>               object_property_set_bool(OBJECT(bus), false, "realized",
>> -                                     local_errp);
>> +                                     local_err ? NULL : &local_err);
>>           }
> 
> This is a rather unusual way to keep the first error of several.
> qapi/error.h advises:
> 
>   * Receive and accumulate multiple errors (first one wins):
>   *     Error *err = NULL, *local_err = NULL;
>   *     foo(arg, &err);
>   *     bar(arg, &local_err);
>   *     error_propagate(&err, local_err);
>   *     if (err) {
>   *         handle the error...
>   *     }

Hmm, honestly, I like more what I've written:

1. less code
2. logic is more clean: we store first error to local_err, and after first error
    pass NULL as a parameter. No propagation or extra error variables.
3. more efficient (no propagation, no extra allocation for errors which we'll 
drop
    anyway) (I understand that efficiency of error path is not thing to care 
about,
    so it's at third place)

Also, propagation which you propose is also unusual thing (it proposed in 
comment,
but who reads it :). I've never seen it before, and I've to go and check that
error_propagate works correctly when first argument is already set.

So, I'd prefer to keep now this patch as is, and to convert later if we really 
need it.

> 
> If replacing this by the usual way is too troublesome now, we can do it
> after the ERRP_AUTO_PROPAGATE() conversion.  Your choice.
> 
>>           if (qdev_get_vmsd(dev)) {
>>               vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
>>           }
>>           if (dc->unrealize) {
>> -            local_errp = local_err ? NULL : &local_err;
>> -            dc->unrealize(dev, local_errp);
>> +            dc->unrealize(dev, local_err ? NULL : &local_err);
>>           }
>>           dev->pending_deleted_event = true;
>>           DEVICE_LISTENER_CALL(unrealize, Reverse, dev);
>> -    }
>>   
>> -    if (local_err != NULL) {
>> -        goto fail;
>> +        if (local_err != NULL) {
>> +            goto fail;
>> +        }
>>       }
>>   
>> +    assert(local_err == NULL);
> 
> Not sure this one's worth it's keep with the usage of local_err cleaned
> up.
> 
>>       dev->realized = value;
>>       return;
>>   
>> @@ -976,7 +974,7 @@ static bool device_get_hotpluggable(Object *obj, Error 
>> **errp)
>>                                   qbus_is_hotpluggable(dev->parent_bus));
>>   }
>>   
>> -static bool device_get_hotplugged(Object *obj, Error **err)
>> +static bool device_get_hotplugged(Object *obj, Error **errp)
>>   {
>>       DeviceState *dev = DEVICE(obj);
> 
> In case you want to clean up afterwards rather than now:
> Reviewed-by: Markus Armbruster <address@hidden>
> 

Thanks!

-- 
Best regards,
Vladimir