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

[Eric Blake]

On 09/18/2015 06:00 AM, 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.
> 

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

(intentionally) Ugly because it is a workaround, but then again, giving
the attribute an ugly name will help call attention to the specific
device owners to fix the mess.  I can live with that.

> 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-* -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'

Not ideal, but much better than a crash, so it gets my vote for
inclusion as an incremental improvement.


> +++ b/include/hw/qdev-core.h
> @@ -114,6 +114,19 @@ typedef struct DeviceClass {
>       * TODO remove once we're there
>       */
>      bool cannot_instantiate_with_device_add_yet;
> +    /*
> +     * Does this device model survive object_unref(object_new(TNAME))?
> +     * All device models should, and this flag shouldn't exist.  Some
> +     * devices crash in object_new(), some crash or hang in
> +     * object_unref().  Makes introspecting properties with
> +     * qmp_device_list_properties() dangerous.  Bad, because it's used
> +     * by -device FOO,help.  This flag serves to protect that code.
> +     * It should never be set without a comment explaining why it is
> +     * set.
> +     * TODO remove once we're there
> +     */
> +    bool cannot_even_create_with_object_new_yet;

And a sufficiently verbose explanation for why the code is so ugly.

> @@ -123,9 +97,6 @@ static void test_device_intro_concrete(void)
>          type = qdict_get_try_str(qobject_to_qdict(qlist_entry_obj(entry)),
>                                  "name");
>          g_assert(type);
> -        if (blacklisted(type)) {
> -            continue;           /* FIXME broken device, skip */
> -        }

The devices are still broken, but the testsuite no longer flags them as
broken because it no longer crashes or hangs, and you intentionally
wrote the test to treat any output (including a graceful error message)
as successful use of the probe.  The ugly attribute is now our only
documentation of the problems, but that is still something sufficient to
hopefully get it fixed.

Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature