Re: [Qemu-devel] [PATCH 10/31] vl: allow full-blown QemuOpts syntax for -global

[Markus Armbruster]

Paolo Bonzini <address@hidden> writes:

> -global does not work for drivers that have a dot in their name, such as
> cfi.pflash01.  This is just a parsing limitation, because such globals
> can be declared easily inside a -readconfig file.
>
> To allow this usage, support the full QemuOpts key/value syntax for -global
> too, for example "-global driver=cfi.pflash01,property=secure,value=on".
> The two formats do not conflict, because the key/value syntax does not have
> a period before the first equal sign.

Overloading syntax isn't exactly pretty, but I guess it's the best we
can do.

> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  qdev-monitor.c  | 18 +++++++++++-------
>  qemu-options.hx |  7 ++++++-
>  2 files changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 1d87f57..9f17c81 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -822,15 +822,19 @@ int qemu_global_option(const char *str)
>      QemuOpts *opts;
>      int rc, offset;
>  
> -    rc = sscanf(str, "%63[^.].%63[^=]%n", driver, property, &offset);
> -    if (rc < 2 || str[offset] != '=') {
> -        error_report("can't parse: \"%s\"", str);
> +    rc = sscanf(str, "%63[^.=].%63[^=]%n", driver, property, &offset);
> +    if (rc == 2 && str[offset] == '=') {
> +        opts = qemu_opts_create(&qemu_global_opts, NULL, 0, &error_abort);
> +        qemu_opt_set(opts, "driver", driver, &error_abort);
> +        qemu_opt_set(opts, "property", property, &error_abort);
> +        qemu_opt_set(opts, "value", str + offset + 1, &error_abort);
> +        return 0;
> +    }
> +
> +    opts = qemu_opts_parse(&qemu_global_opts, str, false);
> +    if (!opts) {
>          return -1;
>      }
>  
> -    opts = qemu_opts_create(&qemu_global_opts, NULL, 0, &error_abort);
> -    qemu_opt_set(opts, "driver", driver, &error_abort);
> -    qemu_opt_set(opts, "property", property, &error_abort);
> -    qemu_opt_set(opts, "value", str + offset + 1, &error_abort);
>      return 0;
>  }

Restrictions on driver and property names in the shorthand syntax
driver.property=value before the patch:

* Both are at most 63 characters long, no whitespace

* Driver name cannot contain '.'

* Property name cannot contain '='

After the patch, we additionally have:

* Driver name cannot contain '='.

Okay as long as no such driver name exists.  Have you checked?

Please spell out the new restriction in the commit message.

In the new longhand syntax, driver name, property name and value all
appear in QemuOpts value position, and as such are restricted to at most
1023 characters long (see opts_do_parse()).  Okay.

I feel we should reject anti-social names for the same reasons we reject
anti-social IDs.  But that's out of scope for this patch.

> diff --git a/qemu-options.hx b/qemu-options.hx
> index ec356f6..43c9ee0 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -171,11 +171,13 @@ Set parameter @var{arg} for item @var{id} of type 
> @var{group}\n"
>  ETEXI
>  
>  DEF("global", HAS_ARG, QEMU_OPTION_global,
> -    "-global driver.prop=value\n"
> +    "-global driver.property=value\n"
> +    "-global driver=driver,property=property,value=value\n"
>      "                set a global default for a driver property\n",
>      QEMU_ARCH_ALL)
>  STEXI
>  @item -global @address@hidden@var{value}
> address@hidden -global address@hidden,address@hidden,address@hidden
>  @findex -global
>  Set default value of @var{driver}'s property @var{prop} to @var{value}, e.g.:
>  

Aside: I had to look up @itemx.  There are a few @item in this file that
should really be @itemx.

> @@ -186,6 +188,9 @@ qemu-system-i386 -global 
> ide-drive.physical_block_size=4096 -drive file=file,if=
>  In particular, you can use this to set driver properties for devices which 
> are 
>  created automatically by the machine model. To create a device which is not 
>  created automatically and set properties on it, use address@hidden
> +
> +The two syntaxes are equivalent.  The longer one works for drivers whose name
> +contains a dot.
>  ETEXI
>  
>  DEF("boot", HAS_ARG, QEMU_OPTION_boot,

I'd explain this as follows:

    -global @address@hidden@var{value} is shorthand for -global
    address@hidden,address@hidden,address@hidden  The
    longhand syntax works even when @var{driver} contains a dot.

But I can accept your version.