| Message ID | 1395705336-22528-3-git-send-email-dslutz@verizon.com |
|---|---|
| State | New |
| Headers | show |
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@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
2014-04-18 17:21 GMT+02:00 Andreas Färber <afaerber@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 <dslutz@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-devel@lists.xen.org > http://lists.xen.org/xen-devel >
Am 18.04.2014 17:36, schrieb Fabio Fantoni: > 2014-04-18 17:21 GMT+02:00 Andreas Färber <afaerber@suse.de > <mailto:afaerber@suse.de>>: > > Hi Don, > > Am 25.03.2014 00 <tel:25.03.2014%2000>: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? There are multiple graphics cards to choose from. When using -vga std or -device vga, then -global vga.foo=bar gets used; if -vga cirrus or -device cirrus-vga then it needs to be -global cirrus-vga.foo=bar and any -global vga.foo=bar gets ignored - unless you manage to add it as secondary (PCI) graphics card. Regards, Andreas P.S. Please remember to use text format mails.
Il 18/04/2014 17:59, Andreas Färber ha scritto: > Am 18.04.2014 17:36, schrieb Fabio Fantoni: >> 2014-04-18 17:21 GMT+02:00 Andreas Färber <afaerber@suse.de >> <mailto:afaerber@suse.de>>: >> >> Hi Don, >> >> Am 25.03.2014 00 <tel:25.03.2014%2000>: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? > There are multiple graphics cards to choose from. When using -vga std or > -device vga, then -global vga.foo=bar gets used; if -vga cirrus or > -device cirrus-vga then it needs to be -global cirrus-vga.foo=bar and > any -global vga.foo=bar gets ignored - unless you manage to add it as > secondary (PCI) graphics card. Thanks for your reply. Can you tell me if also -device cirrus-vga,vram_size_mb=N is correct and working? Thanks for any reply. > > Regards, > Andreas > > P.S. Please remember to use text format mails. >
Il 18/04/2014 18:54, Fabio Fantoni ha scritto: > > Il 18/04/2014 17:59, Andreas Färber ha scritto: >> Am 18.04.2014 17:36, schrieb Fabio Fantoni: >>> 2014-04-18 17:21 GMT+02:00 Andreas Färber <afaerber@suse.de >>> <mailto:afaerber@suse.de>>: >>> >>> Hi Don, >>> >>> Am 25.03.2014 00 <tel:25.03.2014%2000>: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? >> There are multiple graphics cards to choose from. When using -vga std or >> -device vga, then -global vga.foo=bar gets used; if -vga cirrus or >> -device cirrus-vga then it needs to be -global cirrus-vga.foo=bar and >> any -global vga.foo=bar gets ignored - unless you manage to add it as >> secondary (PCI) graphics card. > > Thanks for your reply. > Can you tell me if also -device cirrus-vga,vram_size_mb=N is correct > and working? I probably found the correct values settable: in in hw/display/cirrus_vga.c: > 2988 static Property pci_vga_cirrus_properties[] = { > 2989 DEFINE_PROP_UINT32("vgamem_mb", struct PCICirrusVGAState, > 2990 cirrus_vga.vga.vram_size_mb, 8), > 2991 DEFINE_PROP_END_OF_LIST(), > 2992 }; Than I "-device cirrus-vga,vgamem_mb=N", not show errors and should be correct, right? in hw/display/vga-pci.c: > 182 static Property vga_pci_properties[] = { > 183 DEFINE_PROP_UINT32("vgamem_mb", PCIVGAState, vga.vram_size_mb, > 16), > 184 DEFINE_PROP_BIT("mmio", PCIVGAState, flags, > PCI_VGA_FLAG_ENABLE_MMIO, true), > 185 DEFINE_PROP_END_OF_LIST(), > 186 }; I tried time ago the videoram setting of stdvga on xen but seems was not worked (no error but performance remain bad on medium/large resolution, trying kvm with same parameters is better), I not know if is vgabios problem or low level xen changes are needed. The mmio option seems new to me, what it is in practice, may need to disable it in xen? Thanks for any reply and sorry for my bad english. > > Thanks for any reply. >> >> Regards, >> Andreas >> >> P.S. Please remember to use text format mails. >> >
Il 18/04/2014 11:21, Andreas Färber ha scritto: > 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. Even then, I suspect sooner or later machines other than PC and Q35 will be versioned. At that point we'll probably want QEMU-global compat_props that automatically apply to all machines, even if a type is not missing. I think we should already approximate this behavior by allowing machine .compat_props where the type doesn't exist. Paolo > 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.
On 04/19/14 06:56, Fabio Fantoni wrote: > > Il 18/04/2014 18:54, Fabio Fantoni ha scritto: >> >> Il 18/04/2014 17:59, Andreas Färber ha scritto: >>> Am 18.04.2014 17:36, schrieb Fabio Fantoni: >>>> 2014-04-18 17:21 GMT+02:00 Andreas Färber <afaerber@suse.de >>>> <mailto:afaerber@suse.de>>: >>>> >>>> Hi Don, >>>> >>>> Am 25.03.2014 00 <tel:25.03.2014%2000>: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? >>> There are multiple graphics cards to choose from. When using -vga std or >>> -device vga, then -global vga.foo=bar gets used; if -vga cirrus or >>> -device cirrus-vga then it needs to be -global cirrus-vga.foo=bar and >>> any -global vga.foo=bar gets ignored - unless you manage to add it as >>> secondary (PCI) graphics card. >> >> Thanks for your reply. >> Can you tell me if also -device cirrus-vga,vram_size_mb=N is correct and working? > > I probably found the correct values settable: > > in > > in hw/display/cirrus_vga.c: >> 2988 static Property pci_vga_cirrus_properties[] = { >> 2989 DEFINE_PROP_UINT32("vgamem_mb", struct PCICirrusVGAState, >> 2990 cirrus_vga.vga.vram_size_mb, 8), >> 2991 DEFINE_PROP_END_OF_LIST(), 36d20cb >> 2992 }; > Than I "-device cirrus-vga,vgamem_mb=N", not show errors and should be correct, right? > > in hw/display/vga-pci.c: >> 182 static Property vga_pci_properties[] = { >> 183 DEFINE_PROP_UINT32("vgamem_mb", PCIVGAState, vga.vram_size_mb, 16), >> 184 DEFINE_PROP_BIT("mmio", PCIVGAState, flags, PCI_VGA_FLAG_ENABLE_MMIO, true), >> 185 DEFINE_PROP_END_OF_LIST(), >> 186 }; > I tried time ago the videoram setting of stdvga on xen but seems was not worked (no error but performance remain bad on medium/large resolution, trying kvm with same parameters is better), I not know if is vgabios problem or low level xen changes are needed. > The mmio option seems new to me, what it is in practice, may need to disable it in xen? > > Thanks for any reply and sorry for my bad english. > Using qemu 1.7.0, I verified that -vga std -global "VGA.vgamem_mb=N and -vga cirrus -global cirrus-vga.vgamem_mb=N Correctly set the size of videoram. Have not checked that 2.0 is still the same. I do see: -device cirrus-vga -global vga.vram_size_mb=N In Xen master, which is wrong. -Don Slutz > >> >> Thanks for any reply. >>> >>> Regards, >>> Andreas >>> >>> P.S. Please remember to use text format mails. >>> >> >
On 04/18/14 11:21, Andreas Färber wrote: > 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@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 assume you are talking about qdev_prop_register_global() and qdev_prop_register_global_list(). In my testing I did not see QOM types being registered at that point. I may have not been looking at the right thing. What I am sure on is that the new object pc-memory-layout (added in 2/4) is not there just like TYPE_ICC_BRIDGE at the calls to qdev_prop_register_global*(). Currently I have issues running tests: dcs-xen-50:~/qemu/out>make test make -C tests/tcg test make[1]: Entering directory `/home/don/qemu/out/tests/tcg' cc -m32 -I/home/don/qemu/tcg -I/home/don/qemu/tcg/i386 -I/home/don/qemu/linux-headers -I/home/don/qemu/out/linux-headers -I. -I/home/don/qemu -I/home/don/qemu/include -I/home/don/qemu/libcacard -I/home/don/qemu/tests/tcg -I. -I../.. -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -Wall -O2 -g -fno-strict-aliasing -c -o test_path.o /home/don/qemu/tests/tcg/test_path.c In file included from /home/don/qemu/util/cutils.c:25:0, from /home/don/qemu/tests/tcg/test_path.c:4: /home/don/qemu/include/qemu/host-utils.h: In function 'mulu64': /home/don/qemu/include/qemu/host-utils.h:35:5: error: unknown type name '__uint128_t' /home/don/qemu/include/qemu/host-utils.h:35:22: error: '__uint128_t' undeclared (first use in this function) /home/don/qemu/include/qemu/host-utils.h:35:22: note: each undeclared identifier is reported only once for each function it appears in ... > > 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. This looks correct to me. I do not know enough in the area, but at a quick look, type_register_static() could look at .hotpluggable and .props. and maybe do some checking. >> --- >> 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. > Ok, Will change. >> 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. > Yes, a separate linked list could be used. Did not look at doing this. Mostly since struct init will always set not_used to false. >> 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. Yes, This was the best place I found for it. I am happy to move if that makes sense. -Don Slutz >> + >> if (incoming) { >> Error *local_err = NULL; >> qemu_start_incoming_migration(incoming, &local_err); > Regards, > Andreas >
On 04/19/14 16:54, Paolo Bonzini wrote: > Il 18/04/2014 11:21, Andreas Färber ha scritto: >> 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. > > Even then, I suspect sooner or later machines other than PC and Q35 will be versioned. At that point we'll probably want QEMU-global compat_props that automatically apply to all machines, even if a type is not missing. I think we should already approximate this behavior by allowing machine .compat_props where the type doesn't exist. > If I am reading this correctly, this is asking that .compat_props to not be reported. This currently happens because of not_used being false in all C99 struct initializations. And that Andreas Färber would like me to extend qom-test to check that at least 1 instance of each machine does use each .compat_props so that a typo there gets "checked". -Don Slutz > Paolo > >> 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. >
Il 22/04/2014 19:13, Don Slutz ha scritto: >> >> Even then, I suspect sooner or later machines other than PC and Q35 >> will be versioned. At that point we'll probably want QEMU-global >> compat_props that automatically apply to all machines, even if a type >> is not missing. I think we should already approximate this behavior >> by allowing machine .compat_props where the type doesn't exist. > > If I am reading this correctly, this is asking that .compat_props to not > be reported. This currently happens because of not_used being false > in all C99 struct initializations. Oh, I see. That's obvious in retrospect, but perhaps a comment can be useful around the definition of the not_used field. Paolo
Il 22/04/2014 16:23, Don Slutz ha scritto: > > Currently I have issues running tests: > > dcs-xen-50:~/qemu/out>make test Use "make check", not "make test". make test is old and suffered some bitrot. Paolo
On 04/22/14 20:28, Paolo Bonzini wrote: > Il 22/04/2014 19:13, Don Slutz ha scritto: >>> >>> Even then, I suspect sooner or later machines other than PC and Q35 >>> will be versioned. At that point we'll probably want QEMU-global >>> compat_props that automatically apply to all machines, even if a type >>> is not missing. I think we should already approximate this behavior >>> by allowing machine .compat_props where the type doesn't exist. >> >> If I am reading this correctly, this is asking that .compat_props to not >> be reported. This currently happens because of not_used being false >> in all C99 struct initializations. > > Oh, I see. That's obvious in retrospect, but perhaps a comment can be > useful around the definition of the not_used field. > Will add a comment: /** * GlobalProperty: * @not_used: Track use of a global property. Defaults to false in all C99 struct initializations. * * This prevents reports of .compat_props when they are not used. */ (Not sure if this is the correct formatting.) -Don Slutz > Paolo
On 04/22/14 20:28, Paolo Bonzini wrote: > Il 22/04/2014 16:23, Don Slutz ha scritto: >> >> Currently I have issues running tests: >> >> dcs-xen-50:~/qemu/out>make test > > Use "make check", not "make test". make test is old and suffered some > bitrot. > > Paolo Thanks, "make check" is working. -Don Slutz
Am 23.04.2014 01:13, schrieb Don Slutz: > And that Andreas Färber would like me to extend qom-test to check > that at least 1 instance of each machine does use each .compat_props > so that a typo there gets "checked". No, the machines are already being checked. I was saying, if we are going to check correctness of types for .compat_props, we can easily test for typos using make check (or make check-qtest). Cheers, Andreas
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; } 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(); + if (incoming) { Error *local_err = NULL; qemu_start_incoming_migration(incoming, &local_err);
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@verizon.com> --- 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(+)