[Qemu-devel] [PATCH] qdev: Don't exit when running into bad -global

[Qemu-devel] [PATCH] qdev: Don't exit when running into bad -global

[Markus Armbruster]

-global lets you set a nice booby-trap for yourself:

    $ qemu-system-x86_64 -nodefaults -S -display none -usb -monitor stdio -global usb-mouse.usb_version=l
    QEMU 2.1.94 monitor - type 'help' for more information
    (qemu) device_add usb-mouse
    Parameter 'usb_version' expects an int64 value or range
    $ echo $?
    1

Not nice.  Until commit 3196270 we even abort()ed.

The same error triggers if you manage to screw up a machine type's
compat_props.  To demonstrate, change HW_COMPAT_2_1's entry to

            .driver   = "usb-mouse",\
            .property = "usb_version",\
            .value    = "1", \

Then run

    $ qemu-system-x86_64 -usb -M pc-i440fx-2.1 -device usb-mouse
    upstream-qemu: -device usb-mouse: Parameter 'usb_version' expects an int64 value or range
    $ echo $?
    1

One of our creatively cruel error messages.

Since this is actually a coding error, we *should* abort() here.
Replace the error by an assertion failure in this case.

But turn the fatal error into a mere warning when the faulty
GlobalProperty comes from the user.  Looks like this:

    $ qemu-system-x86_64 -nodefaults -S -display none -usb -monitor stdio -global usb-mouse.usb_version=l
    QEMU 2.1.94 monitor - type 'help' for more information
    (qemu) device_add usb-mouse
    Warning: global usb-mouse.usb_version=l ignored (Parameter 'usb_version' expects an int64 value or range)
    (qemu)

This is consistent with how we handle similarly unusable -global in
qdev_prop_check_globals().

You could argue that the error should make device_add fail.  Would be
harder, because we're running within TypeInfo's instance_post_init()
method device_post_init(), which can't fail.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/core/qdev-properties.c    | 21 +++++++++------------
 hw/core/qdev.c               |  8 +-------
 include/hw/qdev-properties.h |  4 +---
 3 files changed, 11 insertions(+), 22 deletions(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 2e47f70..5a4e4d5 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -990,8 +990,8 @@ int qdev_prop_check_globals(void)
     return ret;
 }
 
-void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
-                                    Error **errp)
+static void qdev_prop_set_globals_for_type(DeviceState *dev,
+                                const char *typename)
 {
     GlobalProperty *prop;
 
@@ -1004,25 +1004,22 @@ void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
         prop->used = true;
         object_property_parse(OBJECT(dev), prop->value, prop->property, &err);
         if (err != NULL) {
-            error_propagate(errp, err);
+            assert(prop->user_provided);
+            error_report("Warning: global %s.%s=%s ignored (%s)",
+                         prop->driver, prop->property, prop->value,
+                         error_get_pretty(err));
+            error_free(err);
             return;
         }
     }
 }
 
-void qdev_prop_set_globals(DeviceState *dev, Error **errp)
+void qdev_prop_set_globals(DeviceState *dev)
 {
     ObjectClass *class = object_get_class(OBJECT(dev));
 
     do {
-        Error *err = NULL;
-
-        qdev_prop_set_globals_for_type(dev, object_class_get_name(class),
-                                       &err);
-        if (err != NULL) {
-            error_propagate(errp, err);
-            return;
-        }
+        qdev_prop_set_globals_for_type(dev, object_class_get_name(class));
         class = object_class_get_parent(class);
     } while (class);
 }
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 901f289..827c084 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -1126,13 +1126,7 @@ static void device_initfn(Object *obj)
 
 static void device_post_init(Object *obj)
 {
-    Error *err = NULL;
-    qdev_prop_set_globals(DEVICE(obj), &err);
-    if (err) {
-        qerror_report_err(err);
-        error_free(err);
-        exit(EXIT_FAILURE);
-    }
+    qdev_prop_set_globals(DEVICE(obj));
 }
 
 /* Unlink device from bus and free the structure.  */
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 070006c..57ee363 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -180,9 +180,7 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
 void qdev_prop_register_global(GlobalProperty *prop);
 void qdev_prop_register_global_list(GlobalProperty *props);
 int qdev_prop_check_globals(void);
-void qdev_prop_set_globals(DeviceState *dev, Error **errp);
-void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
-                                    Error **errp);
+void qdev_prop_set_globals(DeviceState *dev);
 void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
                                     Property *prop, const char *value);
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH] qdev: Don't exit when running into bad -global

[Eric Blake]

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

On 01/20/2015 02:04 AM, Markus Armbruster wrote:
> -global lets you set a nice booby-trap for yourself:
> 
>     $ qemu-system-x86_64 -nodefaults -S -display none -usb -monitor stdio -global usb-mouse.usb_version=l
>     QEMU 2.1.94 monitor - type 'help' for more information
>     (qemu) device_add usb-mouse
>     Parameter 'usb_version' expects an int64 value or range
>     $ echo $?
>     1
> 
> Not nice.  Until commit 3196270 we even abort()ed.
> 

> This is consistent with how we handle similarly unusable -global in
> qdev_prop_check_globals().
> 
> You could argue that the error should make device_add fail.  Would be
> harder, because we're running within TypeInfo's instance_post_init()
> method device_post_init(), which can't fail.

I agree that outputting a warning up front then ignoring the bogus
value, is as good as we can do given we are under "can't fail"
constraints, and much better than confusingly failing down the road.

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/core/qdev-properties.c    | 21 +++++++++------------
>  hw/core/qdev.c               |  8 +-------
>  include/hw/qdev-properties.h |  4 +---
>  3 files changed, 11 insertions(+), 22 deletions(-)


>  
> -void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
> -                                    Error **errp)
> +static void qdev_prop_set_globals_for_type(DeviceState *dev,
> +                                const char *typename)

Is the indentation off here?

But that's minor, so:

Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

Re: [Qemu-devel] [PATCH] qdev: Don't exit when running into bad -global

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 01/20/2015 02:04 AM, Markus Armbruster wrote:
>> -global lets you set a nice booby-trap for yourself:
>> 
>>     $ qemu-system-x86_64 -nodefaults -S -display none -usb -monitor stdio -global usb-mouse.usb_version=l
>>     QEMU 2.1.94 monitor - type 'help' for more information
>>     (qemu) device_add usb-mouse
>>     Parameter 'usb_version' expects an int64 value or range
>>     $ echo $?
>>     1
>> 
>> Not nice.  Until commit 3196270 we even abort()ed.
>> 
>
>> This is consistent with how we handle similarly unusable -global in
>> qdev_prop_check_globals().
>> 
>> You could argue that the error should make device_add fail.  Would be
>> harder, because we're running within TypeInfo's instance_post_init()
>> method device_post_init(), which can't fail.
>
> I agree that outputting a warning up front then ignoring the bogus
> value, is as good as we can do given we are under "can't fail"
> constraints, and much better than confusingly failing down the road.
>
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/core/qdev-properties.c    | 21 +++++++++------------
>>  hw/core/qdev.c               |  8 +-------
>>  include/hw/qdev-properties.h |  4 +---
>>  3 files changed, 11 insertions(+), 22 deletions(-)
>
>
>>  
>> -void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
>> -                                    Error **errp)
>> +static void qdev_prop_set_globals_for_type(DeviceState *dev,
>> +                                const char *typename)
>
> Is the indentation off here?

Yes.  Dear maintainer, can you fix this up on commit, or would you
prefer me to respin?

> But that's minor, so:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH] qdev: Don't exit when running into bad -global

[Eduardo Habkost]

On Tue, Jan 20, 2015 at 10:04:07AM +0100, Markus Armbruster wrote:
> -global lets you set a nice booby-trap for yourself:
> 
>     $ qemu-system-x86_64 -nodefaults -S -display none -usb -monitor stdio -global usb-mouse.usb_version=l
>     QEMU 2.1.94 monitor - type 'help' for more information
>     (qemu) device_add usb-mouse
>     Parameter 'usb_version' expects an int64 value or range
>     $ echo $?
>     1
> 
> Not nice.  Until commit 3196270 we even abort()ed.
> 
> The same error triggers if you manage to screw up a machine type's
> compat_props.  To demonstrate, change HW_COMPAT_2_1's entry to
> 
>             .driver   = "usb-mouse",\
>             .property = "usb_version",\
>             .value    = "1", \
> 
> Then run
> 
>     $ qemu-system-x86_64 -usb -M pc-i440fx-2.1 -device usb-mouse
>     upstream-qemu: -device usb-mouse: Parameter 'usb_version' expects an int64 value or range
>     $ echo $?
>     1
> 
> One of our creatively cruel error messages.
> 
> Since this is actually a coding error, we *should* abort() here.
> Replace the error by an assertion failure in this case.
> 
> But turn the fatal error into a mere warning when the faulty
> GlobalProperty comes from the user.  Looks like this:
> 
>     $ qemu-system-x86_64 -nodefaults -S -display none -usb -monitor stdio -global usb-mouse.usb_version=l
>     QEMU 2.1.94 monitor - type 'help' for more information
>     (qemu) device_add usb-mouse
>     Warning: global usb-mouse.usb_version=l ignored (Parameter 'usb_version' expects an int64 value or range)
>     (qemu)
> 
> This is consistent with how we handle similarly unusable -global in
> qdev_prop_check_globals().
> 
> You could argue that the error should make device_add fail.  Would be
> harder, because we're running within TypeInfo's instance_post_init()
> method device_post_init(), which can't fail.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

So, we could try to validate the -global parameter earlier before it
breaks, but it is impossible to do that because we are moving away from
class-defined static properties to instance-defined dynamic properties.

We could check if the properties exist by actually creating temporary
instances like we do on qmp_device_list_properties(), but:
1) we still have instance_init functions that have lots of side-effects;
2) we can't do that for abstract classes.

I have an idea: let's introduce an register_properties() method that
classes should implement, and then convert all the existing code to this
new way of registering object properties[1].

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

[1] Ha, ha. Just kidding!

-- 
Eduardo

Re: [Qemu-devel] [PATCH] qdev: Don't exit when running into bad -global

[Michael S. Tsirkin]

On Tue, Jan 20, 2015 at 10:04:07AM +0100, Markus Armbruster wrote:
> -global lets you set a nice booby-trap for yourself:
> 
>     $ qemu-system-x86_64 -nodefaults -S -display none -usb -monitor stdio -global usb-mouse.usb_version=l
>     QEMU 2.1.94 monitor - type 'help' for more information
>     (qemu) device_add usb-mouse
>     Parameter 'usb_version' expects an int64 value or range
>     $ echo $?
>     1
> 
> Not nice.  Until commit 3196270 we even abort()ed.
> 
> The same error triggers if you manage to screw up a machine type's
> compat_props.  To demonstrate, change HW_COMPAT_2_1's entry to
> 
>             .driver   = "usb-mouse",\
>             .property = "usb_version",\
>             .value    = "1", \
> 
> Then run
> 
>     $ qemu-system-x86_64 -usb -M pc-i440fx-2.1 -device usb-mouse
>     upstream-qemu: -device usb-mouse: Parameter 'usb_version' expects an int64 value or range
>     $ echo $?
>     1
> 
> One of our creatively cruel error messages.
> 
> Since this is actually a coding error, we *should* abort() here.
> Replace the error by an assertion failure in this case.
> 
> But turn the fatal error into a mere warning when the faulty
> GlobalProperty comes from the user.  Looks like this:
> 
>     $ qemu-system-x86_64 -nodefaults -S -display none -usb -monitor stdio -global usb-mouse.usb_version=l
>     QEMU 2.1.94 monitor - type 'help' for more information
>     (qemu) device_add usb-mouse
>     Warning: global usb-mouse.usb_version=l ignored (Parameter 'usb_version' expects an int64 value or range)
>     (qemu)
> 
> This is consistent with how we handle similarly unusable -global in
> qdev_prop_check_globals().
> 
> You could argue that the error should make device_add fail.  Would be
> harder, because we're running within TypeInfo's instance_post_init()
> method device_post_init(), which can't fail.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>


Applied, thanks!

> ---
>  hw/core/qdev-properties.c    | 21 +++++++++------------
>  hw/core/qdev.c               |  8 +-------
>  include/hw/qdev-properties.h |  4 +---
>  3 files changed, 11 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 2e47f70..5a4e4d5 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -990,8 +990,8 @@ int qdev_prop_check_globals(void)
>      return ret;
>  }
>  
> -void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
> -                                    Error **errp)
> +static void qdev_prop_set_globals_for_type(DeviceState *dev,
> +                                const char *typename)
>  {
>      GlobalProperty *prop;
>  
> @@ -1004,25 +1004,22 @@ void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
>          prop->used = true;
>          object_property_parse(OBJECT(dev), prop->value, prop->property, &err);
>          if (err != NULL) {
> -            error_propagate(errp, err);
> +            assert(prop->user_provided);
> +            error_report("Warning: global %s.%s=%s ignored (%s)",
> +                         prop->driver, prop->property, prop->value,
> +                         error_get_pretty(err));
> +            error_free(err);
>              return;
>          }
>      }
>  }
>  
> -void qdev_prop_set_globals(DeviceState *dev, Error **errp)
> +void qdev_prop_set_globals(DeviceState *dev)
>  {
>      ObjectClass *class = object_get_class(OBJECT(dev));
>  
>      do {
> -        Error *err = NULL;
> -
> -        qdev_prop_set_globals_for_type(dev, object_class_get_name(class),
> -                                       &err);
> -        if (err != NULL) {
> -            error_propagate(errp, err);
> -            return;
> -        }
> +        qdev_prop_set_globals_for_type(dev, object_class_get_name(class));
>          class = object_class_get_parent(class);
>      } while (class);
>  }
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 901f289..827c084 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -1126,13 +1126,7 @@ static void device_initfn(Object *obj)
>  
>  static void device_post_init(Object *obj)
>  {
> -    Error *err = NULL;
> -    qdev_prop_set_globals(DEVICE(obj), &err);
> -    if (err) {
> -        qerror_report_err(err);
> -        error_free(err);
> -        exit(EXIT_FAILURE);
> -    }
> +    qdev_prop_set_globals(DEVICE(obj));
>  }
>  
>  /* Unlink device from bus and free the structure.  */
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 070006c..57ee363 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -180,9 +180,7 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
>  void qdev_prop_register_global(GlobalProperty *prop);
>  void qdev_prop_register_global_list(GlobalProperty *props);
>  int qdev_prop_check_globals(void);
> -void qdev_prop_set_globals(DeviceState *dev, Error **errp);
> -void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
> -                                    Error **errp);
> +void qdev_prop_set_globals(DeviceState *dev);
>  void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
>                                      Property *prop, const char *value);
>  
> -- 
> 1.9.3