Xen project Mailing List

2014-04-18 17:21 GMT+02:00 Andreas Färber <afaerber@xxxxxxx>:

Hi Don,

Am 25.03.2014 00:55, schrieb Don Slutz:
> This can help a user understand why -global was ignored.
>
> For example: with "-vga cirrus"; "-global vga.vgamem_mb=16" is just
> ignored when "-global cirrus-vga.vgamem_mb=16" is not.
>
> This is currently clear when the wrong property is provided:
>
> out/x86_64-softmmu/qemu-system-x86_64 -global cirrus-vga.vram_size_mb=16 -monitor pty -vga cirrus
> char device redirected to /dev/pts/20 (label compat_monitor0)
> qemu-system-x86_64: Property '.vram_size_mb' not found
> Aborted (core dumped)
>
> vs
>
> out/x86_64-softmmu/qemu-system-x86_64 -global vga.vram_size_mb=16 -monitor pty -vga cirrus
> char device redirected to /dev/pts/20 (label compat_monitor0)
> VNC server running on `::1:5900'
> ^Cqemu: terminating on signal 2

I added the cirrus video memory setting in libxl time ago (using -global vga.vram_size_mb), testing it with qemu 1.3 and also on qemu 1.6 when I changed from -vga to -device if I remember good.
Has been changed in recent versions or something was not right even though it seemed right to me?

Thanks for any reply and sorry for my bad english.

>
> Signed-off-by: Don Slutz <dslutz@xxxxxxxxxxx>

Improving this is greatly appreciated, thanks.

Now, I can see two ways things can go wrong: a) Mistyping or later
refactoring devices, or b) user typos or thinkos.
And four ways to set globals: -global, config file (I hope?), legacy
command line options (vl.c) and machine .compat_props.

If a property does not exist on the instance of an existing type,
object_property_parse() will raise an Error and we will abort in
device_post_init().

What we are silently missing is if a type is misspelled; for that we can
probably add an Error **errp to the two qdev_prop_register_global*()
functions - assuming QOM types are already registered at that point.
qom-test would help us catch any such mistake by instantiating each machine.

I note that your proposed check is not failing, but still, with hot-add
of e.g. PCI devices we might well get a global property default for a
type that is not instantiated immediately but possibly used later on.

> ---
> hw/core/qdev-properties-system.c | 1 +
> hw/core/qdev-properties.c | 15 +++++++++++++++
> include/hw/qdev-core.h | 1 +
> include/hw/qdev-properties.h | 1 +
> vl.c | 2 ++
> 5 files changed, 20 insertions(+)

FWIW I'd prefer "qdev:" for consistency (and yes, it's ambiguous), since
there are no "GlobalProperty" files or directory.

> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index de83561..9c742ca 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -444,6 +444,7 @@ static int qdev_add_one_global(QemuOpts *opts, void *opaque)
> g->driver = qemu_opt_get(opts, "driver");
> g->property = qemu_opt_get(opts, "property");
> g->value = qemu_opt_get(opts, "value");
> + g->not_used = true;
> qdev_prop_register_global(g);
> return 0;
> }

IIUC your patch relies on not_used being false in the non-QemuOpts case
to avoid noise when using -nodefaults or pc*-x.y. Still, the C99 struct
initializations elsewhere get that field as well, hmm. An alternative
would be a separate linked list for user-supplied globals.

> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index c67acf5..437c008 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -951,6 +951,20 @@ void qdev_prop_register_global_list(GlobalProperty *props)
> }
> }
>
> +void qdev_prop_check_global(void)
> +{
> + GlobalProperty *prop;
> +
> + QTAILQ_FOREACH(prop, &global_props, next) {
> + if (!prop->not_used) {
> + continue;
> + }
> + fprintf(stderr, "Warning: \"-global %s.%s=%s\" not used\n",
> + prop->driver, prop->property, prop->value);
> +
> + }
> +}
> +
> void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
> Error **errp)
> {
> @@ -962,6 +976,7 @@ void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
> if (strcmp(typename, prop->driver) != 0) {
> continue;
> }
> + prop->not_used = false;
> object_property_parse(OBJECT(dev), prop->value, prop->property, &err);
> if (err != NULL) {
> error_propagate(errp, err);
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index dbe473c..131fb49 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -235,6 +235,7 @@ typedef struct GlobalProperty {
> const char *driver;
> const char *property;
> const char *value;
> + bool not_used;
> QTAILQ_ENTRY(GlobalProperty) next;
> } GlobalProperty;
>
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index c46e908..fbca313 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -180,6 +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);
> +void qdev_prop_check_global(void);
> void qdev_prop_set_globals(DeviceState *dev, Error **errp);
> void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
> Error **errp);
> diff --git a/vl.c b/vl.c
> index acd97a8..61fac1b 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4490,6 +4490,8 @@ int main(int argc, char **argv, char **envp)
> }
> }
>
> + qdev_prop_check_global();

I have some doubts about this placement. A machine init done notifier
might avoid touching vl.c by leaving it in qdev-properties.c. It happens
to be after that point as is, but later refactorings wrt QOM realize or
unrelated issues might change that.

> +
> if (incoming) {
> Error *local_err = NULL;
> qemu_start_incoming_migration(incoming, &local_err);

Regards,
Andreas

--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH v3 2/4] GlobalProperty: Display warning about unused -global