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

[Thomas Huth <thuth@redhat.com>]

On 24/09/15 20:57, Markus Armbruster wrote:
> 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 during init (didn't debug, so I can't say why): "spapr-rng"
> 
> * Crash or hang during cleanup (didn't debug, so I can't say why):
>   "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
>   "s390-sclp-event-facility", "sclp"
> 
> * Dangling pointers: most CPUs, plus "allwinner-a10", "digic",
>   "fsl,imx25", "fsl,imx31", "xlnx,zynqmp", because they create such
>   CPUs
> 
> * Assert kvm_enabled(): "host-x86_64-cpu", host-i386-cpu",
>   "host-powerpc64-cpu", "host-embedded-powerpc-cpu",
>   "host-powerpc-cpu" (the powerpc ones can't currently reach the
>   assertion, because the CPUs are only registered when KVM is enabled,
>   but the assertion is arguably in the wrong place all the same)
> 
> Make qmp_device_list_properties() fail cleanly when the device is so
> marked.  This improves device-list-properties from "crashes or hangs"
> to "fails".  Not a complete fix, just a better-than-nothing
> work-around.  In the above reproducer, device-list-properties now
> fails with "Can't list properties of device 'pxa2xx-pcmcia'".
> 
> This also protects -device FOO,help, which uses the same machinery
> since commit ef52358 "qdev-monitor: include QOM properties in -device
> FOO, help output", v2.2.  Example reproducer:
> 
>     $ qemu-system-aarch64 -machine none -device pxa2xx-pcmcia,help
> 
> Before:
> 
>     qemu-system-aarch64: .../memory.c:1307: memory_region_finalize: Assertion `((&mr->subregions)->tqh_first == ((void *)0))' failed.
> 
> After:
> 
>     Can't list properties of device 'pxa2xx-pcmcia'
> 
> Cc: "Andreas Färber" <afaerber@suse.de>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Alistair Francis <alistair.francis@xilinx.com>
> Cc: Antony Pavlov <antonynpavlov@gmail.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Li Guang <lig.fnst@cn.fujitsu.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: qemu-ppc@nongnu.org
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
...
>  static void pxa2xx_pcmcia_register_types(void)
> diff --git a/hw/ppc/spapr_rng.c b/hw/ppc/spapr_rng.c
> index ed43d5e..e1b115d 100644
> --- a/hw/ppc/spapr_rng.c
> +++ b/hw/ppc/spapr_rng.c
> @@ -169,6 +169,11 @@ static void spapr_rng_class_init(ObjectClass *oc, void *data)
>      dc->realize = spapr_rng_realize;
>      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>      dc->props = spapr_rng_properties;
> +
> +    /*
> +     * Reason: crashes device-introspect-test for unknown reason.
> +     */
> +    dc->cannot_even_create_with_object_new_yet = true;
>  }

Please don't do that! That breaks the help output from
"-device spapr-rng,?" which should help the user to see how to use this
device!

I tried to debug why this device breaks the test, but the test
environment is giving me a hard time ... how do you best hook a gdb into
that framework, so you can trace such problems?
Anyway, with some trial and error, I found out that it seems like the

  object_resolve_path_type("", TYPE_SPAPR_RNG, NULL)

in spapr_rng_instance_init() is causing the problems. Could it be that
object_resolve_path_type is not working with the test environment?

 Thomas