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.