Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 18/03/20 16:06, Markus Armbruster wrote:
>>> - object initialization should cause no side effects outside the subtree
>>> of the object
>> 
>> object_initialize_child() and its users like sysbus_init_child_obj()
>> violate this rule: they add a child property to the subtree's parent.
>> Correct?
>
> At least object_initialize_child() adds the property to the object
> itself, so it's fine.

It seems to

1. Initialize @childobj
2. Set a bunch of properties
3. Add the child property to @parentobj
4. Call .complete() if it's a TYPE_USER_CREATABLE
5. Adjust reference count

Step 3. modifies outside the sub-tree rooted at @childobj.  What am I
missing?

Hmm, maybe this: using object_initialize_child() when initializing
@parentobj is fine; while object_initialize_child() leaves the sub-tree
rooted at @childobj, it still stays within the sub-tree rooted at
@parentobj.

If this is how the function must be used, its contract should spell it
out!

A review of existing uses for misuses may be in order.

> sysbus_init_child_obj() adds a link to the object to the sysbus object
> (via qdev_set_parent_bus/bus_add_child).  This does violate the rule.
> However, currently we have:
>
> - create link on qdev_set_parent_bus, before realizing
>
> - remove link on device_unparent, after unrealizing

By "create link", do you mean bus_add_child() in qdev_set_parent_bus()?

By "remove link", do you mean bus_remove_child() in device_unparent()?

> which makes the device setup even more complicated than necessary.  In
> other words I don't think the bug is in the function, instead it
> probably makes sense to do something else:
>
> - create link in device_set_realized, before calling ->pre_plug (undo on
> failure)
>
> - remove link in device_set_realized, after calling ->unrealize (if it
> succeeds)
>
> and leave sysbus_init_child_obj() as is.

I'm barely following you.  Me reviewing an actual patch could help.

>>> - setting properties can cause side effects outside the subtree of the
>>> object (e.g. marking an object as "in use"), but they must be undone by
>>> finalization.
>> 
>> Textbook example is the 1:1 connection between device frontend and
>> backend: block frontend's property "drive", char frontend's property
>> "chardev", NIC frontend property "netdev", ...
>> 
>> Can we come up with some guardrails for what property setting may do?
>> Affecting guest-visible state is off limits.  What else is?
>
> Yes, guest-visible state is off limits until realization.
>
>>> - realization can also cause side effects outside the subtree of the
>>> object, but if unrealization is possible, they must be undone by
>>> unrealization. if an object is realized and unrealization is not
>>> possible, finalization will never run (and in that case, side effects of
>>> realization need not be undone at all).
>> 
>> One possible answer the question what should be done in realize() is
>> whatever must not be done earlier.  Is that the best we can do?
>
> That's the lower bound of descriptivity.  The upper bound is anything
> that is visible from the guest.  The truth could be in the middle.

Can we set aside a bit of time to write docs/devel/qom.rst together?  I
know object.h is lovingly commented, but unless you already know how QOM
works, you're drowning in detail there.  You'd have to provide most of
the contents, I'm afraid, because you know so much more about it.  Could
save you time in the long run, though: fewer questions to answer, fewer
mistakes to correct.