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

[David Hildenbrand <dahi@linux.vnet.ibm.com>]

> 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.

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.

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.

Dynamic properties that are actually created could depend on almost
everything in the system - why fake something that we don't know.

David