Re: [Qemu-devel] [PATCH] rewamp/simplify option parsing

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2808 bytes --]

On 05/12/2014 01:55 AM, Michael Tokarev wrote:

s/rewamp/revamp/ in the subject

> Main change is to allow get_opt_name() to accept
> a set of delimiters (string) instead of a single
> delimiter (char).  This way it is easier to search
> for the next (sub)option in an option string, so
> other code using get_opt_name() can be simplified.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
> This is an old patch (from Jun 2013) which was
> sitting in my local git repository since that.
> I rebased it to current master (with 2 trivial
> conflicts resolved).
> 
> FWIW.
> 
>  include/qemu/option.h |    3 +-
>  util/qemu-option.c    |  136 +++++++++++++++++++++++--------------------------
>  vl.c                  |    4 +-
>  3 files changed, 67 insertions(+), 76 deletions(-)
> 

> @@ -97,24 +95,27 @@ int get_next_param_value(char *buf, int buf_size,
>  
>      p = *pstr;
>      for(;;) {
> -        p = get_opt_name(option, sizeof(option), p, '=');
> -        if (*p != '=')
> -            break;
> -        p++;
> -        if (!strcmp(tag, option)) {
> -            *pstr = get_opt_value(buf, buf_size, p);
> -            if (**pstr == ',') {
> -                (*pstr)++;
> +        if (*p == '\0') {
> +            return 0;
> +        }
> +        p = get_opt_name(option, sizeof(option), p, "=,");
> +        if (strcmp(tag, option) == 0) {
> +            if (*p == '=') {
> +                p = get_opt_value(buf, buf_size, p + 1);
> +            } else {
> +                buf[0] = '\0';
>              }
> -            return strlen(buf);
> +            *pstr = *p == ',' ? p + 1 : p;
> +            return 1;

Does this botch handling of ',,' escaping that allows literal commas as
part of an option value?  I suspect there may be some corner cases you
are altering.  Based just on a quick read of 'qemu-kvm -help', I see:

-name string1[,process=string2][,debug-threads=on|off]

Which, if I'm interpreting correctly, means that pre-patch I can do:

qemu-kvm -name foo,,bar /dev/null

as a way to start a (pretty pointless) machine with the window title of
"QEMU (foo,bar)".  Reading the source code, I also found the implicit
name of string1:

qemu-kvm -name guest=debug-threads= /dev/null

creates the window title "QEMU (debug-threads=)", and:

qemu-kvm -name debug-threads,, /dev/null

creates the window title "QEMU (debug-threads,)".  That is, I have a way
of specifying ANY name, including trailing '=' or ',', for my guest.

I did not apply your patch, but off-hand, I'm guessing that your patch
breaks at least one, if not multiple, of these cases.  We need unit
tests in place first.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]