qdev unrealize error handling WTF?!?

qdev unrealize error handling WTF?!?

[Markus Armbruster]

Have a look at device_set_realized():

    static void device_set_realized(Object *obj, bool value, Error **errp)
    {
        DeviceState *dev = DEVICE(obj);
        DeviceClass *dc = DEVICE_GET_CLASS(dev);
        HotplugHandler *hotplug_ctrl;
        BusState *bus;
        Error *local_err = NULL;
        bool unattached_parent = false;
        static int unattached_count;

        if (dev->hotplugged && !dc->hotpluggable) {
            error_setg(errp, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj));
            return;
        }

        if (value && !dev->realized) {
            [code to realize snipped...]
        } else if (!value && dev->realized) {
            /* We want local_err to track only the first error */
            QLIST_FOREACH(bus, &dev->child_bus, sibling) {
[1]             object_property_set_bool(OBJECT(bus), false, "realized",
                                         local_err ? NULL : &local_err);
            }
            if (qdev_get_vmsd(dev)) {
[2]             vmstate_unregister(VMSTATE_IF(dev), qdev_get_vmsd(dev), dev);
            }
            if (dc->unrealize) {
[3]             dc->unrealize(dev, local_err ? NULL : &local_err);
            }
            dev->pending_deleted_event = true;
[4]         DEVICE_LISTENER_CALL(unrealize, Reverse, dev);

            if (local_err != NULL) {
                goto fail;
            }
        }

        assert(local_err == NULL);
        dev->realized = value;
        return;

    [realize-only error handling snipped...]
    fail:
        error_propagate(errp, local_err);
        if (unattached_parent) {
            object_unparent(OBJECT(dev));
            unattached_count--;
        }
    }

Can fail in two places: [1] a device on dev->child_bus fails to
unrealize, or [3] @dev itself fails to unrealize.  We hold on to the
first failure, and continue to unrealize.  device_set_realized() fails
when any of these fail.

Issue #1: What if some, but not all fail?  How can this possibly work?

Issue #2: Even if all fail, and therefore both the device and the ones
on ->child_bus all remain realized, there are side effects at [2] and
[4].

Any better ideas than &error_abort?

Re: qdev unrealize error handling WTF?!?

[Paolo Bonzini]

On 27/04/20 14:33, Markus Armbruster wrote:
> [1] a device on dev->child_bus fails to
> unrealize, or [3] @dev itself fails to unrealize.  We hold on to the
> first failure, and continue to unrealize.  device_set_realized() fails
> when any of these fail.
> 
> Issue #1: What if some, but not all fail?  How can this possibly work?
> 
> Issue #2: Even if all fail, and therefore both the device and the ones
> on ->child_bus all remain realized, there are side effects at [2] and
> [4].
> 
> Any better ideas than &error_abort?

I don't think we have unrealizes that fail, so removing the argument and
&error_abort is the best idea.

Paolo