Re: [Qemu-devel] [PATCH 5/7] qdev: Protect device-list-properties against broken devices

[Markus Armbruster]

Eduardo Habkost <address@hidden> writes:

> On Mon, Sep 21, 2015 at 10:30:50AM +0200, David Hildenbrand wrote:
>> > Am 18.09.2015 um 14:00 schrieb Markus Armbruster:
>> > > Several devices don't survive object_unref(object_new(T)): they crash
>> > > or hang during cleanup, or they leave dangling pointers behind.
>> > > 
>> > > This breaks at least device-list-properties, because
>> > > qmp_device_list_properties() needs to create a device to find its
>> > > properties.  Broken in commit f4eb32b "qmp: show QOM properties in
>> > > device-list-properties", v2.1.  Example reproducer:
>> > > 
>> > >     $ qemu-system-aarch64 -nodefaults -display none -machine none -S 
>> > > -qmp stdio
>> > >     {"QMP": {"version": {"qemu": {"micro": 50, "minor": 4, "major": 2}, 
>> > > "package": ""}, "capabilities": []}}
>> > >     { "execute": "qmp_capabilities" }
>> > >     {"return": {}}
>> > >     { "execute": "device-list-properties", "arguments": { "typename": 
>> > > "pxa2xx-pcmcia" } }
>> > >     qemu-system-aarch64: /home/armbru/work/qemu/memory.c:1307: 
>> > > memory_region_finalize: Assertion `((&mr->subregions)->tqh_first == 
>> > > ((void *)0))' failed.
>> > >     Aborted (core dumped)
>> > >     [Exit 134 (SIGABRT)]
>> > > 
>> > > Unfortunately, I can't fix the problems in these devices right now.
>> > > Instead, add DeviceClass member cannot_even_create_with_object_new_yet
>> > > to mark them:
>> > > 
>> > > * Crash or hang during cleanup (didn't debug them, so I can't say
>> > >   why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
>> > >   "s390-sclp-event-facility", "sclp"
>> > 
>> > Ack for the sclp things. Theses devices are created by the machine and
>> > sclp creates the event-facility, so not having a way to query properties
>> > for these devices is better than a hang.
>> > 
>> > David, can you have a look on why these devices fail as outlined?
>> > 
>> 
>> The problem was that the quiesce and cpu hotplug sclp event devices had no
>> parent (onoly a parent bus). So when the bus (inside the event facility) was
>> removed, it looped forever trying remove/unparent it's "bus childs" (which 
>> had
>> no parent).
>> 
>> sclp (parent=machine)
>>     -> even-facility (parent=sclp)
>>                     -> bus (parent=event-facility)
>>                           -> quiesce (parent=null)
>>                           -> cpu hotplug (parent=null)
>> 
>> Maybe that helps others struggling with the same symptoms.
>> 
>> 
>> Just a quick comment on the introspection:
>> 
>> I don't think it is a good idea to expose properties that way. Temporarily
>> creating devices for the sake of querying property names sounds very wrong to
>> me, because certain devices require certain knowledge about how and when they
>> can be created.

I sympathize.  However, QOM is what it is.  Its design assumption
include:

  Properties are dynamically added.  To introspect them, you need to
  create the object.

and:

> No knowledge should be required for object_new(). Classes' instance_init
> functions should have no side-effects outside the object itself.

I'd tighten this to "object_unref(object_new(...)) works and has no
visible side effect".

See also review of [PATCH RFC 1/7] qom: allow properties to be
registered against classes
Message-ID: <address@hidden>
http://lists.gnu.org/archive/html/qemu-devel/2015-09/msg00517.html

>> Feels a little like hacking an interface to a java program, which allows to
>> create any object of a special kind dynamically (constructor arguments?),
>> reading some member variable (names) via reflections and then throwing that
>> object away. Which sounds very wrong to me.
>
> I wouldn't call it wrong. Confusing, maybe. We use a mix of class-based
> and prototype-based approaches to inheritance and property
> introspection.

I'm no friend of this aspect of QOM.  But QOM experts assure us it is
necessary (see thread quoted above).

>> 
>> Wonder if it wouldn't make more sense to query only the static properties of 
>> a
>> device. I mean if the dynamic properties are dynamic by definition (and can
>> change during runtime). So looking up the static properties via the typename
>> feels more KIS-style to me. Of course, the relevant properties would
>> have to be
>> defined statically then, which shouldn't be a problem.
>
> It depends on your definition of "shouldn't be a problem". :)
>
> The static property system is more limited than the QOM dynamic property
> system, today. We already depend on introspection of dynamic properties
> registered by instance_init functions, so we would need to move
> everything to a better static property system if we want to stop doing
> object_new() during class introspection without regressions.

If Dan's patch "qom: allow properties to be registered against classes"
goes in, we can certainly consider the idea to limit
device-list-properties to properties registered against classes.

Even then, the assumption "object_unref(object_new(...)) works and has
no visible side effect" remains until proven unnecessary, because
fundamental design assumptions should not be discarded lightly.

Until then, side effects in instance_init() methods are simply bugs.
This patch protects device-list-properties from known ones.

>> Dynamic properties that are actually created could depend on almost
>> everything in the system - why fake something that we don't know.
>
> Properties registered on instance_init shouldn't depend on anything else
> on the system. Properties registered later in the object lifetime (e.g.
> on realize) can.