Re: [PATCH v3 2/4] GlobalProperty: Display warning about unused -global

["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
> 
> 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