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 <arm...@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 <ehabk...@redhat.com> [1] Ha, ha. Just kidding! -- Eduardo
