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

[Markus Armbruster]

Thomas Huth <address@hidden> writes:

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

Fixing the crashes by bringing back
cannot_destroy_with_object_finalize_yet would be a bit of a cop out, but
it would also be so easy that there's really no excuse for leaving them
unfixed.