2014-04-18 17:21 GMT+02:00 Andreas Färber <afaer...@suse.de>:
> 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 <dsl...@verizon.com>
>
> 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-de...@lists.xen.org
> http://lists.xen.org/xen-devel
>
