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

[Eduardo Habkost]

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.

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

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

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

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

-- 
Eduardo