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 <address@hidden>


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