Re: [Qemu-devel] [PATCH 3/3] qdev: Use qdev_device_add_get_class() for -device <type>, help

[Eduardo Habkost]

On Sat, Nov 01, 2014 at 05:48:19PM +0100, Andreas Färber wrote:
> Am 01.11.2014 um 17:45 schrieb Eduardo Habkost:
> > On Sat, Nov 01, 2014 at 05:40:20PM +0100, Andreas Färber wrote:
> >> Am 01.11.2014 um 16:56 schrieb Eduardo Habkost:
> >>> Make sure we try to list properties from classes that can be safely used 
> >>> with
> >>> "-device".
> >>>
> >>> Fixes the following crashes:
> >>>
> >>>   $ qemu-system-x86_64 -device x86_64-cpu,help
> >>>   **
> >>>   ERROR:qom/object.c:336:object_initialize_with_type: assertion failed: 
> >>> (type->abstract == false)
> >>>   Aborted (core dumped)
> >>>   $ qemu-system-x86_64 -device host-x86_64-cpu,help
> >>>   qemu-system-x86_64: [...]/target-i386/cpu.c:1329: host_x86_cpu_initfn: 
> >>> Assertion `(kvm_allowed)' failed.
> >>>   Aborted (core dumped)
> >>>
> >>> After applying this patch:
> >>>
> >>>   $ qemu-system-x86_64 -device x86_64-cpu,help
> >>>   Parameter 'driver' expects non-abstract device type
> >>>   $ qemu-system-x86_64 -device host-x86_64-cpu,help
> >>>   Parameter 'driver' expects pluggable device type
> >>>
> >>> Signed-off-by: Eduardo Habkost <address@hidden>
> >>> ---
> >>>  qdev-monitor.c | 9 +++------
> >>>  1 file changed, 3 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/qdev-monitor.c b/qdev-monitor.c
> >>> index a9702d8..ebfa701 100644
> >>> --- a/qdev-monitor.c
> >>> +++ b/qdev-monitor.c
> >>> @@ -235,12 +235,9 @@ int qdev_device_help(QemuOpts *opts)
> >>>          return 0;
> >>>      }
> >>>  
> >>> -    if (!object_class_by_name(driver)) {
> >>> -        const char *typename = find_typename_by_alias(driver);
> >>> -
> >>> -        if (typename) {
> >>> -            driver = typename;
> >>> -        }
> >>> +    qdev_get_device_class(&driver, &local_err);
> >>> +    if (local_err) {
> >>> +        goto error;
> >>>      }
> >>>  
> >>>      prop_list = qmp_device_list_properties(driver, &local_err);
> >>
> >> Is dc->cannot_instantiate_with_device_add_yet || (qdev_hotplug &&
> >> !dc->hotpluggable) really relevant here? Or should that rather remain
> >> outside the common function in 1/3?
> > 
> > cannot_instantiate_with_device_add_yet makes sure we won't try to
> > instantiate classes that are not device_add-safe yet (like X86CPU, that
> > has lots of assumptions and side-effects inside instance_init()).
> 
> Maybe I'm misunderstanding? Does this code path apply only to device_add
> (then you are right) or does it also apply to -device?

It applies to both -device and device_add, and both reject
cannot_instantiate_with_device_add_yet classes.

The question here is whether it is safe to blindly call object_new() on
the class or not. I assume that a non-hotpluggable or
cannot_instantiate_with_device_add_yet class indicates it is not safe to
do that.

I am being conservative because I don't know how many classes have
side-effects on instance_init today. I believe the next steps could be:

1) Moving the dc->hotpluggable check to qdev_device_add(). Preferably
   after ensuring all (!hotpluggable && !cannot_instantiate_with_device_add_yet)
   classes have no side-effects on instance_init.
2) Moving the cannot_instantiate_with_device_add_yet check to
   qdev_device_add(). Preferably after ensuring all classes have no
   side-effects on instance_init.

Or we could just move directly to (1) or (2), and live with the
possibility of having QEMU crashing or misbehaving when using
"-device ...,help" or "device_add ...,help" with some class names.

-- 
Eduardo