Re: [Qemu-arm] [Qemu-devel] [PATCH] hw/arm/bcm283x: Fix crash with device_add bcm2837 on unsupported machines

[Thomas Huth]

On 12.07.2018 18:22, Peter Maydell wrote:
> On 12 July 2018 at 17:16, Markus Armbruster <address@hidden> wrote:
>> Thomas Huth <address@hidden> writes:
>>
>>> On 12.07.2018 14:06, Markus Armbruster wrote:
>>>> Peter Maydell <address@hidden> writes:
>>>>
>>>>> On 11 July 2018 at 17:12, Eduardo Habkost <address@hidden> wrote:
>>>>>> On Wed, Jul 11, 2018 at 09:21:48AM +0200, Thomas Huth wrote:
>>>>>>> Hm, ok, so how to continue here now? Shall we at least mark the
>>>>>>> bcm2836/7 devices with user_creatable=false, so that users can not crash
>>>>>>> their QEMU so easily with device_add? The problem with introspection via
>>>>>>> device-list-properties would still continue to exist, but I think that's
>>>>>>> less likely used in practice... otherwise we could still move the
>>>>>>> qdev_set_parent_bus() calls to the realize() function instead, and just
>>>>>>> add a big fat FIXME comment in front of the code block, so that we
>>>>>>> remember to clean that up one day...
>>>>>>
>>>>>> Crashing device-list-properties should be a blocker bug, IMO.
>>>>
>>>> Seconded.
>>>
>>> Well, maybe I should then not suggest to add a hmp("info qtree") below
>>> the hmp("info qom-tree") in test_one_device() of
>>> tests/device-introspect-test.c ... otherwise we'll be quite busy in the
>>> next weeks...
>>
>> If we can't fix these bugs in time, we can bring back
>> cannot_destroy_with_object_finalize_yet, as Eduardo mentioned upthread.
>> Would be sad, but sad beats crash.
> 
> ...but are they actually interesting crashes? Nobody is ever
> going to actually start emulation of an integratorcp machine and
> then try to add a bcm2837 device via the QMP interface, except
> if they're deliberately doing exhaustive testing.

It's not "device_add" that is a real problem here (otherwise we could
simply use user_creatable=false which we likely should do for this
device anyway), but rather the "device-list-properties" QMP command,
which also works for devices that are marked as user_creatable=false.

I think it's valid that an upper layer tool scans all devices for their
properties. But since nobody complained about crashes in the past here
already, it seems like no upper layer tool is currently doing this. So I
agree with you that this should not be a blocker for the 3.0 release.

 Thomas