Eduardo Habkost <ehabk...@redhat.com> 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: <55e72154.7070...@suse.de> 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.