[v2] hw/core/qdev: Increase qdev_realize() kindness

[PATCH v2] hw/core/qdev: Increase qdev_realize() kindness

Posted by Philippe Mathieu-Daudé 3 years, 9 months ago

Since commit 510ef98dca5, qdev_realize() aborts if bus-less
device is realized on a bus. Be kind with the developer by
displaying a hint about what is wrong.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
v2: Use errp (bonzini suggestion)

Paolo, I was tempted to check errp is really &error_abort else
it is an error! :P

    } else if (DEVICE_GET_CLASS(dev)->bus_type) {
        error_setg(errp, "%s: Unexpected bus '%s' for bus-less device '%s'",
                   __func__, DEVICE_GET_CLASS(dev)->bus_type,
                   object_get_typename(OBJECT(dev)));
        assert(errp == &error_abort); // <--------------
        return false;
    }
---
 hw/core/qdev.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 2131c7f951..9d1530c39d 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -392,8 +392,11 @@ bool qdev_realize(DeviceState *dev, BusState *bus, Error **errp)
 
     if (bus) {
         qdev_set_parent_bus(dev, bus);
-    } else {
-        assert(!DEVICE_GET_CLASS(dev)->bus_type);
+    } else if (DEVICE_GET_CLASS(dev)->bus_type) {
+        error_setg(errp, "%s: Unexpected bus '%s' for bus-less device '%s'",
+                   __func__, DEVICE_GET_CLASS(dev)->bus_type,
+                   object_get_typename(OBJECT(dev)));
+        return false;
     }
 
     object_property_set_bool(OBJECT(dev), true, "realized", &err);
-- 
2.21.3

Re: [PATCH v2] hw/core/qdev: Increase qdev_realize() kindness

Posted by Markus Armbruster 3 years, 9 months ago

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> Since commit 510ef98dca5, qdev_realize() aborts if bus-less
> device is realized on a bus. Be kind with the developer by
> displaying a hint about what is wrong.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> v2: Use errp (bonzini suggestion)
>
> Paolo, I was tempted to check errp is really &error_abort else
> it is an error! :P
>
>     } else if (DEVICE_GET_CLASS(dev)->bus_type) {
>         error_setg(errp, "%s: Unexpected bus '%s' for bus-less device '%s'",
>                    __func__, DEVICE_GET_CLASS(dev)->bus_type,
>                    object_get_typename(OBJECT(dev)));
>         assert(errp == &error_abort); // <--------------

Don't!

Functions taking an Error **errp parameter to report errors should never
examine the argument.

The Error API is for separating concerns.  The callee's concern is
detecting errors and failing cleanly.  *Handling* the errors is the
caller's concern.

To simplify common handling patterns, we provide convenience arguments
&error_abort, &error_fatal, and NULL.  Their use is exclusively the
caller's concern.

Examining the argument undermines the separation of concerns.

>         return false;
>     }
> ---
>  hw/core/qdev.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 2131c7f951..9d1530c39d 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -392,8 +392,11 @@ bool qdev_realize(DeviceState *dev, BusState *bus, Error **errp)
>  
>      if (bus) {
>          qdev_set_parent_bus(dev, bus);
> -    } else {
> -        assert(!DEVICE_GET_CLASS(dev)->bus_type);
> +    } else if (DEVICE_GET_CLASS(dev)->bus_type) {
> +        error_setg(errp, "%s: Unexpected bus '%s' for bus-less device '%s'",
> +                   __func__, DEVICE_GET_CLASS(dev)->bus_type,
> +                   object_get_typename(OBJECT(dev)));
> +        return false;
>      }
>  
>      object_property_set_bool(OBJECT(dev), true, "realized", &err);

Scratch __func__.  error_setg() records __FILE__, __LINE__ and __func__,
and error_handle_fatal() prints them.

Always, always, always test your error messages.  If they are impossible
to test, mock up the error.  If mocking up is too much trouble, then
reporting the error nicely is, too.

That said, this one matters to me only insofar as it sets a bad example.
I don't actually care how impossible error messages come out :)

Re: [PATCH v2] hw/core/qdev: Increase qdev_realize() kindness

Posted by Philippe Mathieu-Daudé 3 years, 9 months ago

On 7/6/20 9:09 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> 
>> Since commit 510ef98dca5, qdev_realize() aborts if bus-less
>> device is realized on a bus. Be kind with the developer by
>> displaying a hint about what is wrong.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> v2: Use errp (bonzini suggestion)
>>
>> Paolo, I was tempted to check errp is really &error_abort else
>> it is an error! :P
>>
>>     } else if (DEVICE_GET_CLASS(dev)->bus_type) {
>>         error_setg(errp, "%s: Unexpected bus '%s' for bus-less device '%s'",
>>                    __func__, DEVICE_GET_CLASS(dev)->bus_type,
>>                    object_get_typename(OBJECT(dev)));
>>         assert(errp == &error_abort); // <--------------
> 
> Don't!
> 
> Functions taking an Error **errp parameter to report errors should never
> examine the argument.
> 
> The Error API is for separating concerns.  The callee's concern is
> detecting errors and failing cleanly.  *Handling* the errors is the
> caller's concern.
> 
> To simplify common handling patterns, we provide convenience arguments
> &error_abort, &error_fatal, and NULL.  Their use is exclusively the
> caller's concern.
> 
> Examining the argument undermines the separation of concerns.
> 
>>         return false;
>>     }
>> ---
>>  hw/core/qdev.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index 2131c7f951..9d1530c39d 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -392,8 +392,11 @@ bool qdev_realize(DeviceState *dev, BusState *bus, Error **errp)
>>  
>>      if (bus) {
>>          qdev_set_parent_bus(dev, bus);
>> -    } else {
>> -        assert(!DEVICE_GET_CLASS(dev)->bus_type);
>> +    } else if (DEVICE_GET_CLASS(dev)->bus_type) {
>> +        error_setg(errp, "%s: Unexpected bus '%s' for bus-less device '%s'",
>> +                   __func__, DEVICE_GET_CLASS(dev)->bus_type,
>> +                   object_get_typename(OBJECT(dev)));
>> +        return false;
>>      }
>>  
>>      object_property_set_bool(OBJECT(dev), true, "realized", &err);
> 
> Scratch __func__.  error_setg() records __FILE__, __LINE__ and __func__,
> and error_handle_fatal() prints them.

Correct :/

> 
> Always, always, always test your error messages.  If they are impossible
> to test, mock up the error.  If mocking up is too much trouble, then
> reporting the error nicely is, too.
> 
> That said, this one matters to me only insofar as it sets a bad example.
> I don't actually care how impossible error messages come out :)

It might seems "impossible" to you because you only work with mainstream
QEMU.
There are plenty of forks trying to keep up with mainstream. Their code
is not mergeable in mainstream for various reasons (usually code not in
good shape, using deprecated APIs). These are mostly hobbyist trying
different boards / hardware / whatever.

You cleaned mainstream incorrect qdev/qom bus/parenting code before
merging your qdev/qom cleanup in commit 6675a653d2 ("Merge
remotes/armbru/tags/pull-qom-2020-06-15") but you didn't clean all the
forks. Now various forks simply abort, and it seems quite hard for a
developer not following mainstream activity on a daily basis to figure
out which change broke is fork.

As of today:
$ git log --oneline --since="3 weeks ago"|wc -l
776
And in 2 days we'll probably pass the 1k.

I hope you now understand the motivation of this patch. I failed at
properly describing it, mostly because I have a limited hobbyist time
where I try to address as much issues as I can.

I'll respin with improved commit description, and __func__ removed.

Regards,

Phil.