From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= Subject: Re: [PATCH v3 2/4] GlobalProperty: Display warning about unused -global Date: Fri, 18 Apr 2014 17:21:54 +0200 Message-ID: <53514312.5060705@suse.de> References: <1395705336-22528-1-git-send-email-dslutz@verizon.com> <1395705336-22528-3-git-send-email-dslutz@verizon.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <1395705336-22528-3-git-send-email-dslutz@verizon.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org Sender: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org To: Don Slutz Cc: xen-devel@lists.xensource.com, "Michael S. Tsirkin" , qemu-devel@nongnu.org, Anthony Liguori , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org Hi Don, Am 25.03.2014 00:55, schrieb Don Slutz: > This can help a user understand why -global was ignored. >=20 > For example: with "-vga cirrus"; "-global vga.vgamem_mb=3D16" is just > ignored when "-global cirrus-vga.vgamem_mb=3D16" is not. >=20 > This is currently clear when the wrong property is provided: >=20 > out/x86_64-softmmu/qemu-system-x86_64 -global cirrus-vga.vram_size_mb=3D= 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) >=20 > vs >=20 > out/x86_64-softmmu/qemu-system-x86_64 -global vga.vram_size_mb=3D16 -mo= nitor pty -vga cirrus > char device redirected to /dev/pts/20 (label compat_monitor0) > VNC server running on `::1:5900' > ^Cqemu: terminating on signal 2 >=20 > Signed-off-by: Don Slutz 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 machi= ne. 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 =3D qemu_opt_get(opts, "driver"); > g->property =3D qemu_opt_get(opts, "property"); > g->value =3D qemu_opt_get(opts, "value"); > + g->not_used =3D 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) > } > } > =20 > +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=3D%s\" not used\n", > + prop->driver, prop->property, prop->value); > + > + } > +} > + > void qdev_prop_set_globals_for_type(DeviceState *dev, const char *type= name, > Error **errp) > { > @@ -962,6 +976,7 @@ void qdev_prop_set_globals_for_type(DeviceState *de= v, const char *typename, > if (strcmp(typename, prop->driver) !=3D 0) { > continue; > } > + prop->not_used =3D false; > object_property_parse(OBJECT(dev), prop->value, prop->property= , &err); > if (err !=3D 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; > =20 > 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); > =20 > 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 *type= name, > 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) > } > } > =20 > + 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 =3D NULL; > qemu_start_incoming_migration(incoming, &local_err); Regards, Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg