Re: [PATCH 03/55] qdev: New qdev_new(), qdev_realize(), etc.

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> On 20/05/20 10:11, Markus Armbruster wrote:
>>>> On 19/05/20 16:54, Markus Armbruster wrote:
>>>>> +
>>>>> +    object_ref(OBJECT(dev));
>>>>> +    object_property_set_bool(OBJECT(dev), true, "realized", &err);
>>>>> +    if (err) {
>>>>> +        error_propagate_prepend(errp, err,
>>>>> +                                "Initialization of device %s failed: ",
>>>>> +                                object_get_typename(OBJECT(dev)));
>>>>> +    }
>>>>> +    object_unref(OBJECT(dev));
>>>> Why is the ref/unref pair needed?  Should it be done in the realized
>>>> setter instead?
>>> Copied from qdev_init_nofail(), where it is necessary (I figured out why
>>> the hard way).  It doesn't seem to be necessary here, though.  Thanks!
>>
>> Why is it necessary there?  It seems a bit iffy.
>
> My exact thoughts a few days back.  One debugging session later, I
> understood, and put them right back.  Glad we have tests :)
>
> When object_property_set_bool() fails in qdev_init_nofail(), the
> reference count can drop to zero.  Certainly surprised me.  Have a look:
>
>         dev = qdev_create(bus, type_name);
>         // @dev is a weak reference, and @bus holds the only strong one
>         ...
>         qdev_init_nofail(dev);
>
> In qdev_init_nofail():
>
>         // object_ref(OBJECT(dev));
>         object_property_set_bool(OBJECT(dev), true, "realized", &err);
>
> This is a fancy way to call device_set_realized().  If something goes
> wrong there, we execute
>
>     fail:
>         error_propagate(errp, local_err);
>         if (unattached_parent) {
>             /*
>              * Beware, this doesn't just revert
>              * object_property_add_child(), it also runs bus_remove()!
>              */
>             object_unparent(OBJECT(dev));
>             unattached_count--;
>         }
>
> and bus_remove() drops the reference count to zero.
>
> Back in qdev_init_nofail(), we then use after free:
>     
>     if (err) {
>         error_reportf_err(err, "Initialization of device %s failed: ",
> --->                      object_get_typename(OBJECT(dev)));
>         exit(1);
>     }
>     // object_unref(OBJECT(dev));
>
> The ref/unref keeps around @dev long enough for adding @dev's type name
> to the error message.
>
> The equivalent new pattern doesn't have this issue:
>
>         dev = qdev_new(type_name);
>         // @dev is the only reference
>         ...
>         qdev_realize_and_unref(dev, bus, errp);
>
> In qdev_realize(), called via qdev_realize_and_unref():
>
>         qdev_set_parent_bus(dev, bus);
>         // @bus now holds the second reference
>
>         // object_ref(OBJECT(dev));
>         object_property_set_bool(OBJECT(dev), true, "realized", &err);
>
> In device_set_realized(), the reference count drops to one, namely
> @dev's reference.  That one goes away only in qdev_realize_and_unref(),
> after we added @dev's type name to the error message.
>
> However, a boring drive to the supermarket gave me this scenario:
>
>         dev = qdev_new(type_name);
>         // @dev is the only reference
>         ...
>         object_property_add_child(parent, name, OBJECT(dev));
>         // @parent holds the second reference
>         object_unref(dev);
>         // unusual, but not wrong; @parent holds the only reference now
>         ...
>         qdev_realize(dev, bus, errp);
>
> Here, the reference count can drop to zero when device_set_realized()
> fails, and qdev_realize()'s object_get_typename() is a use after free.
>
> Best to keep the ref/unref, I think.

Actually, best to get rid of the "Initialization of device FOO failed: "
prefix, because:

    $ qemu-system-x86_64 -device virtio-blk
    qemu-system-x86_64: -device virtio-blk: Initialization of device 
virtio-blk-pci failed: Initialization of device virtio-blk-device failed: drive 
property not set

Ugly as sin.

The prefix exists for cases like this:

    $ qemu-system-x86_64 -vga cirrus -global cirrus-vga.vgamem_mb=99
    qemu-system-x86_64: Initialization of device cirrus-vga failed: Invalid 
cirrus_vga ram size '99'

Ideally, we'd point to the user configuration that caused the failure,
in this case -global cirrus-vga.vgamem_mb=99.  But that would be work,
so we made do with mentioning the device type.

Prefix pileup is not possible with qdev_init_nofail(), because the error
is immediately fatal there.

With qdev_realize(), realize failure commonly ripples through QOM
composition tree parents all the way to board initialization, and the
prefix gets added at every step.

If we want to keep the prefix, we could keep qdev_init_nofail(), then
figure out when to use it instead of qdev_realize().  That's a lot of
work.  I doubt it's worthwhile now.

I'll drop it.  Speak up if you want me to reconsider.