[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [Qemu-devel] [PATCH v3 2/4] GlobalProperty: Display warning about unused -global
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 <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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |