| Message ID | 1421744647-26844-1-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
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> 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!
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!
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
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);
-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(-)