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 <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
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;
+ }
}
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);
}
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);
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);
--
2.21.0
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> 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 <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>
> 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...
* }
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 <armbru@redhat.com>
29.11.2019 9:07, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> 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 <vsementsov@virtuozzo.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>
>> 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 <armbru@redhat.com>
>
Thanks!
--
Best regards,
Vladimir
On 12/5/19 8:48 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> @@ -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.
It may be unusual, but has the benefit of avoiding error_propagate...
>> 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.
...and after conversion to use ERRP_AUTO_PROPATATE(), the use of
error_propagate() should NOT occur in any code _except_ for the macro
definition (any other use of the function points out a place where we
failed to use the macro to get rid of boilerplate).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Eric Blake <eblake@redhat.com> writes:
> On 12/5/19 8:48 AM, Vladimir Sementsov-Ogievskiy wrote:
>
>>>> @@ -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.
>
> It may be unusual, but has the benefit of avoiding error_propagate...
Non-issue if the error_propagate() gets replaced by
ERRP_AUTO_PROPAGATE(), isn't it?
>>> 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.
When you think you can improve on the common, documented pattern, you're
invited to update the documentation and the existing uses of the
pattern.
If everybody "improved" on common, documented patterns locally, the code
would become needlessly hard to read for developers with experience in
the pattern's area.
>> So, I'd prefer to keep now this patch as is, and to convert later if we really need it.
I want this to match the common, documented pattern. Whether we make it
match before or after your ERRP_AUTO_PROPAGATE() work doesn't matter to
me.
>>> If replacing this by the usual way is too troublesome now, we can do it
>>> after the ERRP_AUTO_PROPAGATE() conversion. Your choice.
>
> ...and after conversion to use ERRP_AUTO_PROPATATE(), the use of
> error_propagate() should NOT occur in any code _except_ for the macro
> definition (any other use of the function points out a place where we
> failed to use the macro to get rid of boilerplate).
I figure we still need it in the (rare) cases where we want to ignore
some of a function's errors, as we do in fit_load_fdt(). If that
bothers us, we can try to find a solution that avoids the boilerplate.
06.12.2019 11:26, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> On 12/5/19 8:48 AM, Vladimir Sementsov-Ogievskiy wrote:
>>
>>>>> @@ -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.
>>
>> It may be unusual, but has the benefit of avoiding error_propagate...
>
> Non-issue if the error_propagate() gets replaced by
> ERRP_AUTO_PROPAGATE(), isn't it?
>
>>>> 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.
>
> When you think you can improve on the common, documented pattern, you're
> invited to update the documentation and the existing uses of the
> pattern.
>
> If everybody "improved" on common, documented patterns locally, the code
> would become needlessly hard to read for developers with experience in
> the pattern's area.
That's reasonable, I understand.
>
>>> So, I'd prefer to keep now this patch as is, and to convert later if we really need it.
>
> I want this to match the common, documented pattern. Whether we make it
> match before or after your ERRP_AUTO_PROPAGATE() work doesn't matter to
> me.
Then, let's after.
>
>>>> If replacing this by the usual way is too troublesome now, we can do it
>>>> after the ERRP_AUTO_PROPAGATE() conversion. Your choice.
>>
>> ...and after conversion to use ERRP_AUTO_PROPATATE(), the use of
>> error_propagate() should NOT occur in any code _except_ for the macro
>> definition (any other use of the function points out a place where we
>> failed to use the macro to get rid of boilerplate).
>
> I figure we still need it in the (rare) cases where we want to ignore
> some of a function's errors, as we do in fit_load_fdt(). If that
> bothers us, we can try to find a solution that avoids the boilerplate.
>
--
Best regards,
Vladimir
© 2016 - 2024 Red Hat, Inc.