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

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

[1] Ha, ha. Just kidding!

-- 
Eduardo