[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] qdev: Don't exit when running into bad -global
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-devel] [PATCH] qdev: Don't exit when running into bad -global |
Date: |
Mon, 26 Jan 2015 16:51:15 -0200 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
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