[PATCH-for-5.2 v4] hw/core/qdev: Increase qdev_realize() kindness

[PATCH-for-5.2 v4] hw/core/qdev: Increase qdev_realize() kindness

[Philippe Mathieu-Daudé]

Since commit 510ef98dca5, qdev_realize() aborts if bus-less
device is realized on a bus. While commits 514db7710b..007d1dbf72
took care of converting all mainstream uses, QEMU forks weren't
converted.

These forks are usually maintained by hobbyist with interest in
following mainstream development, but with limited time, so usually
rebase from time to time. To avoid them to spend time on debugging
and reading git-log history, display a kind hint about what is wrong.

Before:

  qemu-system-mipsel: hw/core/qdev.c:376: qdev_realize: Assertion `!DEVICE_GET_CLASS(dev)->bus_type' failed.
  Aborted (core dumped)

After:

  Unexpected error in qdev_realize() at hw/core/qdev.c:376:
  qemu-system-mipsel: Unexpected bus 'System' for bus-less device 'unimplemented-device'
  Aborted (core dumped)

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..a16f1270f1 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, "Unexpected bus '%s' for bus-less device '%s'",
+                   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-for-5.2 v4] hw/core/qdev: Increase qdev_realize() kindness

[Markus Armbruster]

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

> Since commit 510ef98dca5, qdev_realize() aborts if bus-less
> device is realized on a bus. While commits 514db7710b..007d1dbf72
> took care of converting all mainstream uses, QEMU forks weren't
> converted.
>
> These forks are usually maintained by hobbyist with interest in
> following mainstream development, but with limited time, so usually
> rebase from time to time. To avoid them to spend time on debugging
> and reading git-log history, display a kind hint about what is wrong.
>
> Before:
>
>   qemu-system-mipsel: hw/core/qdev.c:376: qdev_realize: Assertion `!DEVICE_GET_CLASS(dev)->bus_type' failed.
>   Aborted (core dumped)
>
> After:
>
>   Unexpected error in qdev_realize() at hw/core/qdev.c:376:
>   qemu-system-mipsel: Unexpected bus 'System' for bus-less device 'unimplemented-device'
>   Aborted (core dumped)
>
> 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..a16f1270f1 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, "Unexpected bus '%s' for bus-less device '%s'",
> +                   DEVICE_GET_CLASS(dev)->bus_type,
> +                   object_get_typename(OBJECT(dev)));
> +        return false;
>      }
>  
>      object_property_set_bool(OBJECT(dev), true, "realized", &err);

Objection.  This turns an abort into something else unless the caller
passes &error_abort.  The caller in your commit message's example does,
others don't.

Keep the unconditional abort, please.  Feel free to print something kind
right before.  I doubt it's all that useful, as I believe whoever gets
to fix the bug will have to figure out the code anyway, but I could be
wrong.

Re: [PATCH-for-5.2 v4] hw/core/qdev: Increase qdev_realize() kindness

[Paolo Bonzini]

On 28/07/20 09:44, Markus Armbruster wrote:
>> -        assert(!DEVICE_GET_CLASS(dev)->bus_type);
>> +    } else if (DEVICE_GET_CLASS(dev)->bus_type) {
>> +        error_setg(errp, "Unexpected bus '%s' for bus-less device '%s'",
>> +                   DEVICE_GET_CLASS(dev)->bus_type,
>> +                   object_get_typename(OBJECT(dev)));
>> +        return false;
>>      }
>>  
>>      object_property_set_bool(OBJECT(dev), true, "realized", &err);
> Objection.  This turns an abort into something else unless the caller
> passes &error_abort.  The caller in your commit message's example does,
> others don't.
> 
> Keep the unconditional abort, please.  Feel free to print something kind
> right before.  I doubt it's all that useful, as I believe whoever gets
> to fix the bug will have to figure out the code anyway, but I could be
> wrong.
> 

This was my request, actually.  We have an Error**, we should use it in
case this code is reached via device_add.

Paolo

Re: [PATCH-for-5.2 v4] hw/core/qdev: Increase qdev_realize() kindness

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 28/07/20 09:44, Markus Armbruster wrote:
>>> -        assert(!DEVICE_GET_CLASS(dev)->bus_type);
>>> +    } else if (DEVICE_GET_CLASS(dev)->bus_type) {
>>> +        error_setg(errp, "Unexpected bus '%s' for bus-less device '%s'",
>>> +                   DEVICE_GET_CLASS(dev)->bus_type,
>>> +                   object_get_typename(OBJECT(dev)));
>>> +        return false;
>>>      }
>>>  
>>>      object_property_set_bool(OBJECT(dev), true, "realized", &err);
>> Objection.  This turns an abort into something else unless the caller
>> passes &error_abort.  The caller in your commit message's example does,
>> others don't.
>> 
>> Keep the unconditional abort, please.  Feel free to print something kind
>> right before.  I doubt it's all that useful, as I believe whoever gets
>> to fix the bug will have to figure out the code anyway, but I could be
>> wrong.
>> 
>
> This was my request, actually.  We have an Error**, we should use it in
> case this code is reached via device_add.

That's not actually possible.  qdev_device_add():

    path = qemu_opt_get(opts, "bus");
    if (path != NULL) {

If user passed bus=...,

        bus = qbus_find(path, errp);
        if (!bus) {
            return NULL;
        }
        if (!object_dynamic_cast(OBJECT(bus), dc->bus_type)) {
            error_setg(errp, "Device '%s' can't go on %s bus",
                       driver, object_get_typename(OBJECT(bus)));

but the device is bus-less, error out.

            return NULL;
        }
    } else if (dc->bus_type != NULL) {


If user did not pass bus=..., but the device needs one,

        bus = qbus_find_recursive(sysbus_get_default(), NULL, dc->bus_type);

pick a default bus, or else ...

        if (!bus || qbus_is_full(bus)) {
            error_setg(errp, "No '%s' bus found for device '%s'",
                       dc->bus_type, driver);
            return NULL;

error out.

        }
    }

Taking a step back, I disagree with the notion that assertions should be
avoided just because we have an Error **.  A programming error doesn't
become less wrong, and continuing when the program is in disorder
doesn't become any safer when you add an Error ** parameter to a
function.

If you're calling for recovering from programming errors where that can
be done safely, we can talk about creating the necessary infrastructure.
Handling them as if they were errors the user can do something about can
only lead to confusion.

Re: [PATCH-for-5.2 v4] hw/core/qdev: Increase qdev_realize() kindness

[Philippe Mathieu-Daudé]

On 7/29/20 9:39 AM, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> On 28/07/20 09:44, Markus Armbruster wrote:
>>>> -        assert(!DEVICE_GET_CLASS(dev)->bus_type);
>>>> +    } else if (DEVICE_GET_CLASS(dev)->bus_type) {
>>>> +        error_setg(errp, "Unexpected bus '%s' for bus-less device '%s'",
>>>> +                   DEVICE_GET_CLASS(dev)->bus_type,
>>>> +                   object_get_typename(OBJECT(dev)));
>>>> +        return false;
>>>>      }
>>>>  
>>>>      object_property_set_bool(OBJECT(dev), true, "realized", &err);
>>> Objection.  This turns an abort into something else unless the caller
>>> passes &error_abort.  The caller in your commit message's example does,
>>> others don't.
>>>
>>> Keep the unconditional abort, please.  Feel free to print something kind
>>> right before.  I doubt it's all that useful, as I believe whoever gets
>>> to fix the bug will have to figure out the code anyway, but I could be
>>> wrong.
>>>
>>
>> This was my request, actually.  We have an Error**, we should use it in
>> case this code is reached via device_add.
> 
> That's not actually possible.

I agree this condition is not possible in current mainstream.

What I'm working on is:

qmp command that:
- create a SDCard or FloppyDisk medium
- eventually link a block driver to it
- insert the medium into a slot

then another qmp command that
- eject the medium
- unlink the block driver
- destroy the medium

second step is a command that takes as argument
(block driver, bus endpoint) and automatically
creates the envelope media and insert it to the bus.

> qdev_device_add():
> 
>     path = qemu_opt_get(opts, "bus");
>     if (path != NULL) {
> 
> If user passed bus=...,
> 
>         bus = qbus_find(path, errp);
>         if (!bus) {
>             return NULL;
>         }
>         if (!object_dynamic_cast(OBJECT(bus), dc->bus_type)) {
>             error_setg(errp, "Device '%s' can't go on %s bus",
>                        driver, object_get_typename(OBJECT(bus)));
> 
> but the device is bus-less, error out.
> 
>             return NULL;
>         }
>     } else if (dc->bus_type != NULL) {
> 
> 
> If user did not pass bus=..., but the device needs one,
> 
>         bus = qbus_find_recursive(sysbus_get_default(), NULL, dc->bus_type);
> 
> pick a default bus, or else ...
> 
>         if (!bus || qbus_is_full(bus)) {
>             error_setg(errp, "No '%s' bus found for device '%s'",
>                        dc->bus_type, driver);
>             return NULL;
> 
> error out.
> 
>         }
>     }
> 
> Taking a step back, I disagree with the notion that assertions should be
> avoided just because we have an Error **.  A programming error doesn't
> become less wrong, and continuing when the program is in disorder
> doesn't become any safer when you add an Error ** parameter to a
> function.
> 
> If you're calling for recovering from programming errors where that can
> be done safely, we can talk about creating the necessary infrastructure.
> Handling them as if they were errors the user can do something about can
> only lead to confusion.
> 
> 

Re: [PATCH-for-5.2 v4] hw/core/qdev: Increase qdev_realize() kindness

[Markus Armbruster]

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

> On 7/29/20 9:39 AM, Markus Armbruster wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> On 28/07/20 09:44, Markus Armbruster wrote:
>>>>> -        assert(!DEVICE_GET_CLASS(dev)->bus_type);
>>>>> +    } else if (DEVICE_GET_CLASS(dev)->bus_type) {
>>>>> +        error_setg(errp, "Unexpected bus '%s' for bus-less device '%s'",
>>>>> +                   DEVICE_GET_CLASS(dev)->bus_type,
>>>>> +                   object_get_typename(OBJECT(dev)));
>>>>> +        return false;
>>>>>      }
>>>>>  
>>>>>      object_property_set_bool(OBJECT(dev), true, "realized", &err);
>>>> Objection.  This turns an abort into something else unless the caller
>>>> passes &error_abort.  The caller in your commit message's example does,
>>>> others don't.
>>>>
>>>> Keep the unconditional abort, please.  Feel free to print something kind
>>>> right before.  I doubt it's all that useful, as I believe whoever gets
>>>> to fix the bug will have to figure out the code anyway, but I could be
>>>> wrong.
>>>>
>>>
>>> This was my request, actually.  We have an Error**, we should use it in
>>> case this code is reached via device_add.
>> 
>> That's not actually possible.
>
> I agree this condition is not possible in current mainstream.
>
> What I'm working on is:
>
> qmp command that:
> - create a SDCard or FloppyDisk medium
> - eventually link a block driver to it
> - insert the medium into a slot
>
> then another qmp command that
> - eject the medium
> - unlink the block driver
> - destroy the medium
>
> second step is a command that takes as argument
> (block driver, bus endpoint) and automatically
> creates the envelope media and insert it to the bus.

If this makes the error possible, then your code fails to establish
qdev_realize()'s precondition, and therefore needs fixing.

Could a combination of existing commands get the job done?

Re: [PATCH-for-5.2 v4] hw/core/qdev: Increase qdev_realize() kindness

[Philippe Mathieu-Daudé]

On 7/29/20 2:32 PM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> 
>> On 7/29/20 9:39 AM, Markus Armbruster wrote:
>>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>>
>>>> On 28/07/20 09:44, Markus Armbruster wrote:
>>>>>> -        assert(!DEVICE_GET_CLASS(dev)->bus_type);
>>>>>> +    } else if (DEVICE_GET_CLASS(dev)->bus_type) {
>>>>>> +        error_setg(errp, "Unexpected bus '%s' for bus-less device '%s'",
>>>>>> +                   DEVICE_GET_CLASS(dev)->bus_type,
>>>>>> +                   object_get_typename(OBJECT(dev)));
>>>>>> +        return false;
>>>>>>      }
>>>>>>  
>>>>>>      object_property_set_bool(OBJECT(dev), true, "realized", &err);
>>>>> Objection.  This turns an abort into something else unless the caller
>>>>> passes &error_abort.  The caller in your commit message's example does,
>>>>> others don't.
>>>>>
>>>>> Keep the unconditional abort, please.  Feel free to print something kind
>>>>> right before.  I doubt it's all that useful, as I believe whoever gets
>>>>> to fix the bug will have to figure out the code anyway, but I could be
>>>>> wrong.
>>>>>
>>>>
>>>> This was my request, actually.  We have an Error**, we should use it in
>>>> case this code is reached via device_add.
>>>
>>> That's not actually possible.
>>
>> I agree this condition is not possible in current mainstream.
>>
>> What I'm working on is:
>>
>> qmp command that:
>> - create a SDCard or FloppyDisk medium
>> - eventually link a block driver to it
>> - insert the medium into a slot
>>
>> then another qmp command that
>> - eject the medium
>> - unlink the block driver
>> - destroy the medium
>>
>> second step is a command that takes as argument
>> (block driver, bus endpoint) and automatically
>> creates the envelope media and insert it to the bus.
> 
> If this makes the error possible, then your code fails to establish
> qdev_realize()'s precondition, and therefore needs fixing.

Yes, because I try to create a bus-ful device without plugging it on
the bus (I want to set the block driver link before plugging it).

> 
> Could a combination of existing commands get the job done?

Maybe. I'm trying a clean design. With the current state it is hard
to figure if my design is broken or the current QEMU design [*] is
broken and unfixable.

[*] keep re-using existing media device, add blockdrv, reset media
    device internals register depending of the blockdrv properties,
    eject blockdrv/insert different one and keep adapting the device.

Re: [PATCH-for-5.2 v4] hw/core/qdev: Increase qdev_realize() kindness

[Markus Armbruster]

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

> On 7/29/20 2:32 PM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>> 
>>> On 7/29/20 9:39 AM, Markus Armbruster wrote:
>>>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>>>
>>>>> On 28/07/20 09:44, Markus Armbruster wrote:
>>>>>>> -        assert(!DEVICE_GET_CLASS(dev)->bus_type);
>>>>>>> +    } else if (DEVICE_GET_CLASS(dev)->bus_type) {
>>>>>>> +        error_setg(errp, "Unexpected bus '%s' for bus-less device '%s'",
>>>>>>> +                   DEVICE_GET_CLASS(dev)->bus_type,
>>>>>>> +                   object_get_typename(OBJECT(dev)));
>>>>>>> +        return false;
>>>>>>>      }
>>>>>>>  
>>>>>>>      object_property_set_bool(OBJECT(dev), true, "realized", &err);
>>>>>> Objection.  This turns an abort into something else unless the caller
>>>>>> passes &error_abort.  The caller in your commit message's example does,
>>>>>> others don't.
>>>>>>
>>>>>> Keep the unconditional abort, please.  Feel free to print something kind
>>>>>> right before.  I doubt it's all that useful, as I believe whoever gets
>>>>>> to fix the bug will have to figure out the code anyway, but I could be
>>>>>> wrong.
>>>>>>
>>>>>
>>>>> This was my request, actually.  We have an Error**, we should use it in
>>>>> case this code is reached via device_add.
>>>>
>>>> That's not actually possible.
>>>
>>> I agree this condition is not possible in current mainstream.
>>>
>>> What I'm working on is:
>>>
>>> qmp command that:
>>> - create a SDCard or FloppyDisk medium
>>> - eventually link a block driver to it
>>> - insert the medium into a slot
>>>
>>> then another qmp command that
>>> - eject the medium
>>> - unlink the block driver
>>> - destroy the medium
>>>
>>> second step is a command that takes as argument
>>> (block driver, bus endpoint) and automatically
>>> creates the envelope media and insert it to the bus.
>> 
>> If this makes the error possible, then your code fails to establish
>> qdev_realize()'s precondition, and therefore needs fixing.
>
> Yes, because I try to create a bus-ful device without plugging it on
> the bus (I want to set the block driver link before plugging it).

You need to set properties *before* you realize!

[...]

Re: [PATCH-for-5.2 v4] hw/core/qdev: Increase qdev_realize() kindness

[Paolo Bonzini]

On 29/07/20 09:39, Markus Armbruster wrote:
> Taking a step back, I disagree with the notion that assertions should be
> avoided just because we have an Error **.  A programming error doesn't
> become less wrong, and continuing when the program is in disorder
> doesn't become any safer when you add an Error ** parameter to a
> function.

I don't think it is actually unsafe to continue after passing a bus-less
device with a bus_type to qdev_realize.  It will fail, but orderly.

So even though it's a programming error, it should not be a big deal to
avoid the assertion here: either the caller will pass &error_abort, or
it will print a nice error message and let the user go on with their
business.

I'm not particularly attached to the change, but it seemed inconsistent
to use error_setg(&error_abort).

Paolo

Re: [PATCH-for-5.2 v4] hw/core/qdev: Increase qdev_realize() kindness

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 29/07/20 09:39, Markus Armbruster wrote:
>> Taking a step back, I disagree with the notion that assertions should be
>> avoided just because we have an Error **.  A programming error doesn't
>> become less wrong, and continuing when the program is in disorder
>> doesn't become any safer when you add an Error ** parameter to a
>> function.
>
> I don't think it is actually unsafe to continue after passing a bus-less
> device with a bus_type to qdev_realize.  It will fail, but orderly.
>
> So even though it's a programming error, it should not be a big deal to
> avoid the assertion here: either the caller will pass &error_abort, or
> it will print a nice error message and let the user go on with their
> business.

An error message the user can do nothing about other than report as a
bug is never nice.

We can try to phrase the message in a way that makes "this is a bug,
please report" clear, but doing that case-by-case can only result in
inconsistency and confusion.  Having a common way to do it gets us to:

>> If you're calling for recovering from programming errors where that can
>> be done safely, we can talk about creating the necessary infrastructure.
>> Handling them as if they were errors the user can do something about can
>> only lead to confusion.

Moreover, we want programming errors reported even when we recover, so
they get fixed.  If we treat them like any other error, that is not
assured: errors may be handled silently.

> I'm not particularly attached to the change, but it seemed inconsistent
> to use error_setg(&error_abort).

From error.h:

 * Please don't error_setg(&error_fatal, ...), use error_report() and
 * exit(), because that's more obvious.
 * Likewise, don't error_setg(&error_abort, ...), use assert().