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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

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

Named in Anthony's memory (see commit efec3dd) ;-P

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

Thanks!