|
| From: | Fabio Fantoni |
| Subject: | Re: [Qemu-devel] [Xen-devel] [PATCH v3 2/4] GlobalProperty: Display warning about unused -global |
| Date: | Fri, 18 Apr 2014 17:36:44 +0200 |
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
>
> Signed-off-by: Don Slutz <address@hidden>
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
address@hidden
http://lists.xen.org/xen-devel
| [Prev in Thread] | Current Thread | [Next in Thread] |