Eric Blake <ebl...@redhat.com> 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 <ebl...@redhat.com> Thanks!