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.
> 
>