[Qemu-devel] [PATCH 0/3] qdev: Validate class name for -device <class>, help

[Qemu-devel] [PATCH 0/3] qdev: Validate class name for -device <class>, help

[Eduardo Habkost]

This 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: /home/ehabkost/rh/proj/virt/qemu/target-i386/cpu.c:1329: host_x86_cpu_initfn: Assertion `(kvm_allowed)' failed.
   Aborted (core dumped)

Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Gonglei <arei.gonglei@huawei.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Andreas Färber <afaerber@suse.de>
Cc: Paolo Bonzini <pbonzini@redhat.com>

Eduardo Habkost (3):
  qdev: Create qdev_get_device_class() function
  qdev: Move error printing to the end of qdev_device_help()
  qdev: Use qdev_device_add_get_class() for -device <type>,help

 qdev-monitor.c | 88 ++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 51 insertions(+), 37 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/3] qdev: Create qdev_get_device_class() function

[Eduardo Habkost]

Extract the DeviceClass lookup from qdev_device_add() to a separate
function.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 qdev-monitor.c | 70 +++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 42 insertions(+), 28 deletions(-)

diff --git a/qdev-monitor.c b/qdev-monitor.c
index fac7d17..982f3f4 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -180,6 +180,44 @@ static const char *find_typename_by_alias(const char *alias)
     return NULL;
 }
 
+static DeviceClass *qdev_get_device_class(const char **driver, Error **errp)
+{
+    ObjectClass *oc;
+    DeviceClass *dc;
+
+    oc = object_class_by_name(*driver);
+    if (!oc) {
+        const char *typename = find_typename_by_alias(*driver);
+
+        if (typename) {
+            *driver = typename;
+            oc = object_class_by_name(*driver);
+        }
+    }
+
+    if (!object_class_dynamic_cast(oc, TYPE_DEVICE)) {
+        error_setg(errp, "'%s' is not a valid device model name", *driver);
+        return NULL;
+    }
+
+    if (object_class_is_abstract(oc)) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "driver",
+                  "non-abstract device type");
+        return NULL;
+    }
+
+    dc = DEVICE_CLASS(oc);
+    if (dc->cannot_instantiate_with_device_add_yet ||
+        (qdev_hotplug && !dc->hotpluggable)) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "driver",
+                  "pluggable device type");
+        return NULL;
+    }
+
+    return dc;
+}
+
+
 int qdev_device_help(QemuOpts *opts)
 {
     Error *local_err = NULL;
@@ -455,7 +493,6 @@ static BusState *qbus_find(const char *path)
 
 DeviceState *qdev_device_add(QemuOpts *opts)
 {
-    ObjectClass *oc;
     DeviceClass *dc;
     const char *driver, *path, *id;
     DeviceState *dev;
@@ -469,33 +506,10 @@ DeviceState *qdev_device_add(QemuOpts *opts)
     }
 
     /* find driver */
-    oc = object_class_by_name(driver);
-    if (!oc) {
-        const char *typename = find_typename_by_alias(driver);
-
-        if (typename) {
-            driver = typename;
-            oc = object_class_by_name(driver);
-        }
-    }
-
-    if (!object_class_dynamic_cast(oc, TYPE_DEVICE)) {
-        qerror_report(ERROR_CLASS_GENERIC_ERROR,
-                      "'%s' is not a valid device model name", driver);
-        return NULL;
-    }
-
-    if (object_class_is_abstract(oc)) {
-        qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver",
-                      "non-abstract device type");
-        return NULL;
-    }
-
-    dc = DEVICE_CLASS(oc);
-    if (dc->cannot_instantiate_with_device_add_yet ||
-        (qdev_hotplug && !dc->hotpluggable)) {
-        qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver",
-                      "pluggable device type");
+    dc = qdev_get_device_class(&driver, &err);
+    if (err) {
+        qerror_report_err(err);
+        error_free(err);
         return NULL;
     }
 
-- 
1.9.3

[Qemu-devel] [PATCH 2/3] qdev: Move error printing to the end of qdev_device_help()

[Eduardo Habkost]

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 qdev-monitor.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/qdev-monitor.c b/qdev-monitor.c
index 982f3f4..a9702d8 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -245,9 +245,7 @@ int qdev_device_help(QemuOpts *opts)
 
     prop_list = qmp_device_list_properties(driver, &local_err);
     if (local_err) {
-        error_printf("%s\n", error_get_pretty(local_err));
-        error_free(local_err);
-        return 1;
+        goto error;
     }
 
     for (prop = prop_list; prop; prop = prop->next) {
@@ -263,6 +261,11 @@ int qdev_device_help(QemuOpts *opts)
 
     qapi_free_DevicePropertyInfoList(prop_list);
     return 1;
+
+error:
+    error_printf("%s\n", error_get_pretty(local_err));
+    error_free(local_err);
+    return 1;
 }
 
 static Object *qdev_get_peripheral(void)
-- 
1.9.3

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

[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 <ehabkost@redhat.com>
---
 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);
-- 
1.9.3

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

[Andreas Färber]

Hi Eduardo,

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 <ehabkost@redhat.com>
> ---
>  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?

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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:40:20PM +0100, Andreas Färber wrote:
> Hi Eduardo,
> 
> 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 <ehabkost@redhat.com>
> > ---
> >  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()).

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/3] qdev: Create qdev_get_device_class() function

[Andreas Färber]

Hi,

Am 01.11.2014 um 16:56 schrieb Eduardo Habkost:
> Extract the DeviceClass lookup from qdev_device_add() to a separate
> function.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  qdev-monitor.c | 70 +++++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 42 insertions(+), 28 deletions(-)
> 
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index fac7d17..982f3f4 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -180,6 +180,44 @@ static const char *find_typename_by_alias(const char *alias)
>      return NULL;
>  }
>  
> +static DeviceClass *qdev_get_device_class(const char **driver, Error **errp)

Since this does nothing qdev-specific, what about
device_get_class_by_name()? The only that's not generically suitable for
hw/core/ is the "driver" naming in the error handling; otherwise it
looks very similar to the CPUClass hooks I added a while back.

> +{
> +    ObjectClass *oc;
> +    DeviceClass *dc;
> +
> +    oc = object_class_by_name(*driver);
> +    if (!oc) {
> +        const char *typename = find_typename_by_alias(*driver);
> +
> +        if (typename) {
> +            *driver = typename;
> +            oc = object_class_by_name(*driver);
> +        }
> +    }
> +
> +    if (!object_class_dynamic_cast(oc, TYPE_DEVICE)) {
> +        error_setg(errp, "'%s' is not a valid device model name", *driver);
> +        return NULL;
> +    }
> +
> +    if (object_class_is_abstract(oc)) {
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "driver",
> +                  "non-abstract device type");
> +        return NULL;
> +    }
> +

See 3/3 for whether we may want to return here.

> +    dc = DEVICE_CLASS(oc);
> +    if (dc->cannot_instantiate_with_device_add_yet ||
> +        (qdev_hotplug && !dc->hotpluggable)) {
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "driver",
> +                  "pluggable device type");
> +        return NULL;
> +    }
> +
> +    return dc;
> +}
> +
> +
>  int qdev_device_help(QemuOpts *opts)
>  {
>      Error *local_err = NULL;
> @@ -455,7 +493,6 @@ static BusState *qbus_find(const char *path)
>  
>  DeviceState *qdev_device_add(QemuOpts *opts)
>  {
> -    ObjectClass *oc;
>      DeviceClass *dc;
>      const char *driver, *path, *id;
>      DeviceState *dev;
> @@ -469,33 +506,10 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>      }
>  
>      /* find driver */
> -    oc = object_class_by_name(driver);
> -    if (!oc) {
> -        const char *typename = find_typename_by_alias(driver);
> -
> -        if (typename) {
> -            driver = typename;
> -            oc = object_class_by_name(driver);
> -        }
> -    }
> -
> -    if (!object_class_dynamic_cast(oc, TYPE_DEVICE)) {
> -        qerror_report(ERROR_CLASS_GENERIC_ERROR,
> -                      "'%s' is not a valid device model name", driver);
> -        return NULL;
> -    }
> -
> -    if (object_class_is_abstract(oc)) {
> -        qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver",
> -                      "non-abstract device type");
> -        return NULL;
> -    }
> -
> -    dc = DEVICE_CLASS(oc);
> -    if (dc->cannot_instantiate_with_device_add_yet ||
> -        (qdev_hotplug && !dc->hotpluggable)) {
> -        qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver",
> -                      "pluggable device type");
> +    dc = qdev_get_device_class(&driver, &err);
> +    if (err) {
> +        qerror_report_err(err);
> +        error_free(err);
>          return NULL;
>      }
>  

Otherwise looks good.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[Andreas Färber]

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 <ehabkost@redhat.com>
>>> ---
>>>  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?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1/3] qdev: Create qdev_get_device_class() function

[Eduardo Habkost]

On Sat, Nov 01, 2014 at 05:46:14PM +0100, Andreas Färber wrote:
> Hi,
> 
> Am 01.11.2014 um 16:56 schrieb Eduardo Habkost:
> > Extract the DeviceClass lookup from qdev_device_add() to a separate
> > function.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  qdev-monitor.c | 70 +++++++++++++++++++++++++++++++++++-----------------------
> >  1 file changed, 42 insertions(+), 28 deletions(-)
> > 
> > diff --git a/qdev-monitor.c b/qdev-monitor.c
> > index fac7d17..982f3f4 100644
> > --- a/qdev-monitor.c
> > +++ b/qdev-monitor.c
> > @@ -180,6 +180,44 @@ static const char *find_typename_by_alias(const char *alias)
> >      return NULL;
> >  }
> >  
> > +static DeviceClass *qdev_get_device_class(const char **driver, Error **errp)
> 
> Since this does nothing qdev-specific, what about
> device_get_class_by_name()? The only that's not generically suitable for
> hw/core/ is the "driver" naming in the error handling; otherwise it
> looks very similar to the CPUClass hooks I added a while back.

Well, I don't know where's the line that divides "qdev-specific" from
"DeviceState-specific". The distinction seems arbitrary to me, and I
won't see any problem if somebody moves all qdev-*.c files inside
hw/core.  :)

I assume that at least qdev_alias_table (which is used by both
qdev_device_add() and qdev_device_help()) can be considered
qdev-specific (but just because the table is currently inside
qdev-monitor.c).

Anyway, the only point of this function is to reduce code duplication
between qdev_device_add() and qdev_device_help() while fixing the
"-device ,help" crashes. If somebody wants to move any of the existing
qdev-*.c logic to hw/core later, I won't complain (but I wouldn't want
to do that after hard freeze).

> 
> > +{
> > +    ObjectClass *oc;
> > +    DeviceClass *dc;
> > +
> > +    oc = object_class_by_name(*driver);
> > +    if (!oc) {
> > +        const char *typename = find_typename_by_alias(*driver);
> > +
> > +        if (typename) {
> > +            *driver = typename;
> > +            oc = object_class_by_name(*driver);
> > +        }
> > +    }
> > +
> > +    if (!object_class_dynamic_cast(oc, TYPE_DEVICE)) {
> > +        error_setg(errp, "'%s' is not a valid device model name", *driver);
> > +        return NULL;
> > +    }
> > +
> > +    if (object_class_is_abstract(oc)) {
> > +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "driver",
> > +                  "non-abstract device type");
> > +        return NULL;
> > +    }
> > +
> 
> See 3/3 for whether we may want to return here.

The point of this function is to allow reuse of checks that are
necessary on both qdev_device_add() and qdev_device_help(), and both
functions need to reject abstract and
cannot_instantiate_with_device_add_yet classes.

-- 
Eduardo

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 <ehabkost@redhat.com>
> >>> ---
> >>>  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

Re: [Qemu-devel] [PATCH 0/3] qdev: Validate class name for -device <class>, help

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 626 bytes --]

On Sat, Nov 01, 2014 at 01:56:08PM -0200, Eduardo Habkost wrote:
> This 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: /home/ehabkost/rh/proj/virt/qemu/target-i386/cpu.c:1329: host_x86_cpu_initfn: Assertion `(kvm_allowed)' failed.
>    Aborted (core dumped)
> 
> Cc: Stefan Hajnoczi <stefanha@redhat.com>

Haven't reviewed in detail but thanks for fixing this.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 0/3] qdev: Validate class name for -device <class>, help

[Andreas Färber]

[-- Attachment #1: Type: text/plain, Size: 933 bytes --]

Am 03.11.2014 um 16:14 schrieb Stefan Hajnoczi:
> On Sat, Nov 01, 2014 at 01:56:08PM -0200, Eduardo Habkost wrote:
>> This 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: /home/ehabkost/rh/proj/virt/qemu/target-i386/cpu.c:1329: host_x86_cpu_initfn: Assertion `(kvm_allowed)' failed.
>>    Aborted (core dumped)
>>
>> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Haven't reviewed in detail but thanks for fixing this.

Thanks, applied to qom-next as is:
https://github.com/afaerber/qemu-cpu/commits/qom-next

Andreas

-- 
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 21284 AG Nürnberg


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]