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

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

Posted by Philippe Mathieu-Daudé 3 years, 11 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>
---
 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..dd3c90d37a 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_report("%s: Unexpected bus '%s' for device '%s'",
+                     __func__, DEVICE_GET_CLASS(dev)->bus_type,
+                     object_get_typename(OBJECT(dev)));
+        abort();
     }
 
     object_property_set_bool(OBJECT(dev), true, "realized", &err);
-- 
2.21.3

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

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

ping?

On 6/20/20 5:38 PM, Philippe Mathieu-Daudé wrote:
> 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>
> ---
>  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..dd3c90d37a 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_report("%s: Unexpected bus '%s' for device '%s'",
> +                     __func__, DEVICE_GET_CLASS(dev)->bus_type,
> +                     object_get_typename(OBJECT(dev)));
> +        abort();
>      }
>  
>      object_property_set_bool(OBJECT(dev), true, "realized", &err);
>

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

Posted by Paolo Bonzini 3 years, 10 months ago

On 20/06/20 17:38, Philippe Mathieu-Daudé wrote:
> -    } else {
> -        assert(!DEVICE_GET_CLASS(dev)->bus_type);
> +    } else if (DEVICE_GET_CLASS(dev)->bus_type) {
> +        error_report("%s: Unexpected bus '%s' for device '%s'",
> +                     __func__, DEVICE_GET_CLASS(dev)->bus_type,
> +                     object_get_typename(OBJECT(dev)));
> +        abort();
>      }
>  

Since there is an errp, should we use it and be even kinder?

Paolo

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

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

On 7/5/20 7:46 AM, Paolo Bonzini wrote:
> On 20/06/20 17:38, Philippe Mathieu-Daudé wrote:
>> -    } else {
>> -        assert(!DEVICE_GET_CLASS(dev)->bus_type);
>> +    } else if (DEVICE_GET_CLASS(dev)->bus_type) {
>> +        error_report("%s: Unexpected bus '%s' for device '%s'",
>> +                     __func__, DEVICE_GET_CLASS(dev)->bus_type,
>> +                     object_get_typename(OBJECT(dev)));
>> +        abort();
>>      }
>>  
> 
> Since there is an errp, should we use it and be even kinder?

This is a programming error, not an user triggerable condition,
so I'm not sure. IOW this must not happen, but if it does, then
the error message helps the developer to notice the problem without
having to use gdb.

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

Posted by Paolo Bonzini 3 years, 10 months ago

Are we sure that qdev_realize is never called with user-provided input? If
it's a programming error, the call chain will end up passing &error_abort
anyway, won't it?

Paolo

Il dom 5 lug 2020, 12:05 Philippe Mathieu-Daudé <f4bug@amsat.org> ha
scritto:

> On 7/5/20 7:46 AM, Paolo Bonzini wrote:
> > On 20/06/20 17:38, Philippe Mathieu-Daudé wrote:
> >> -    } else {
> >> -        assert(!DEVICE_GET_CLASS(dev)->bus_type);
> >> +    } else if (DEVICE_GET_CLASS(dev)->bus_type) {
> >> +        error_report("%s: Unexpected bus '%s' for device '%s'",
> >> +                     __func__, DEVICE_GET_CLASS(dev)->bus_type,
> >> +                     object_get_typename(OBJECT(dev)));
> >> +        abort();
> >>      }
> >>
> >
> > Since there is an errp, should we use it and be even kinder?
>
> This is a programming error, not an user triggerable condition,
> so I'm not sure. IOW this must not happen, but if it does, then
> the error message helps the developer to notice the problem without
> having to use gdb.
>
>

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

Posted by Markus Armbruster 3 years, 10 months ago

Paolo Bonzini <pbonzini@redhat.com> writes:

> Are we sure that qdev_realize is never called with user-provided input? If

The only way to call qdev_realize() with a user-provided bus is -device
/ device_add via qdev_device_add().  qdev_device_add() carefully checks
the user-provided bus before passing it to qdev_realize().

> it's a programming error, the call chain will end up passing &error_abort
> anyway, won't it?

Correct.

>
> Paolo
>
> Il dom 5 lug 2020, 12:05 Philippe Mathieu-Daudé <f4bug@amsat.org> ha
> scritto:
>
>> On 7/5/20 7:46 AM, Paolo Bonzini wrote:
>> > On 20/06/20 17:38, Philippe Mathieu-Daudé wrote:
>> >> -    } else {
>> >> -        assert(!DEVICE_GET_CLASS(dev)->bus_type);
>> >> +    } else if (DEVICE_GET_CLASS(dev)->bus_type) {
>> >> +        error_report("%s: Unexpected bus '%s' for device '%s'",
>> >> +                     __func__, DEVICE_GET_CLASS(dev)->bus_type,
>> >> +                     object_get_typename(OBJECT(dev)));
>> >> +        abort();
>> >>      }
>> >>
>> >
>> > Since there is an errp, should we use it and be even kinder?
>>
>> This is a programming error, not an user triggerable condition,
>> so I'm not sure. IOW this must not happen, but if it does, then
>> the error message helps the developer to notice the problem without
>> having to use gdb.

I don't bother with reporting impossible errors nicely.  Statement, not
objection.

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

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

On 7/5/20 1:14 PM, Paolo Bonzini wrote:
> Are we sure that qdev_realize is never called with user-provided input?

I am not sure, but ...

> If it's a programming error, the call chain will end up passing
> &error_abort anyway, won't it?

... this is a good point :)

> 
> Paolo
> 
> Il dom 5 lug 2020, 12:05 Philippe Mathieu-Daudé <f4bug@amsat.org
> <mailto:f4bug@amsat.org>> ha scritto:
> 
>     On 7/5/20 7:46 AM, Paolo Bonzini wrote:
>     > On 20/06/20 17:38, Philippe Mathieu-Daudé wrote:
>     >> -    } else {
>     >> -        assert(!DEVICE_GET_CLASS(dev)->bus_type);
>     >> +    } else if (DEVICE_GET_CLASS(dev)->bus_type) {
>     >> +        error_report("%s: Unexpected bus '%s' for device '%s'",
>     >> +                     __func__, DEVICE_GET_CLASS(dev)->bus_type,
>     >> +                     object_get_typename(OBJECT(dev)));
>     >> +        abort();
>     >>      }
>     >> 
>     >
>     > Since there is an errp, should we use it and be even kinder?
> 
>     This is a programming error, not an user triggerable condition,
>     so I'm not sure. IOW this must not happen, but if it does, then
>     the error message helps the developer to notice the problem without
>     having to use gdb.
>