Re: [Qemu-devel] [PATCH] monitor: HMP: fix consecutive integer expression parsing

[Alon Levy <alevy@redhat.com>]

On Wed, Aug 03, 2011 at 02:57:27PM +0300, Alon Levy wrote:
> Currently a command that takes two consecutive integer operations, like
> client_migrate_info, will be incorrectly parsed by the human monitor if
> the second expression begins with a minus ('-') or plus ('+') sign:
> 
> client_migrate_info <protocol> <hostname> <port> <tls-port>
> client_migrate_info spice localhost 5900 -1
> => port = 5899 = 5900 - 1
>    tls-port = -1
> But expected by the user to be:
>    port = 5900
>    tls-port = -1

Actually since the current code doesn't accept negative numbers after this patch
that command is illegal, which is perfectly fine.

> 
> The fix is that for any required integer (ilM) expression followed by another
> integer expression (ilM) the first expression will be parsed by expr_unary
> instead of expr_sum. So you can still use arithmetic, but you have to enclose
> it in parenthesis:
> 
> Command line | Old parsed result | With patch result
> (1+1) 2      | 2, 2              | 2, 2
> 1 -1         | 0, -1             | 1, -1
> The rest are bizarre but not any worse then before
> 1+2+3        | 6, 5              | 1, 5
The old is actually: 6, -1 (the first get_expr consumes the whole command line, the
second gets a zero length string and so returns the default -1)
> (1+2)+3      | 3, 3              | 3, 3
> 
> Signed-off-by: Alon Levy <alevy@redhat.com>
> ---
>  monitor.c |   27 ++++++++++++++++++++++++---
>  1 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 1b8ba2c..45e2d6c 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3889,7 +3889,7 @@ static int64_t expr_sum(Monitor *mon)
>      return val;
>  }
>  
> -static int get_expr(Monitor *mon, int64_t *pval, const char **pp)
> +static int get_expr(Monitor *mon, int64_t *pval, const char **pp, int unary)
>  {
>      pch = *pp;
>      if (setjmp(expr_env)) {
> @@ -3898,7 +3898,11 @@ static int get_expr(Monitor *mon, int64_t *pval, const char **pp)
>      }
>      while (qemu_isspace(*pch))
>          pch++;
> -    *pval = expr_sum(mon);
> +    if (unary) {
> +        *pval = expr_unary(mon);
> +    } else {
> +        *pval = expr_sum(mon);
> +    }
>      *pp = pch;
>      return 0;
>  }
> @@ -4267,6 +4271,9 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>          case 'M':
>              {
>                  int64_t val;
> +                int unary = 0;
> +                char *next_key;
> +                char *next;
>  
>                  while (qemu_isspace(*p))
>                      p++;
> @@ -4288,7 +4295,21 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>                      }
>                      typestr++;
>                  }
> -                if (get_expr(mon, &val, &p))
> +                next = key_get_info(typestr, &next_key);
> +                qemu_free(next_key);
> +                if (*next == 'i' || *next == 'l' || *next == 'M') {
> +                    /* If a command has two consecutive ii parameters the first
> +                     * get_expr will also parse the second parameter if it
> +                     * starts with a - or +. To avoid this only parse unary in
> +                     * this case, i.e.:
> +                     * client_migrate_info spice localhost 1 -1
> +                     *  => 1, -1
> +                     * client_migrate_info spice localhost (1+3) -1
> +                     *  => 4, -1
> +                     */
> +                    unary = 1;
> +                }
> +                if (get_expr(mon, &val, &p, unary))
>                      goto fail;
>                  /* Check if 'i' is greater than 32-bit */
>                  if ((c == 'i') && ((val >> 32) & 0xffffffff)) {
> -- 
> 1.7.6
> 
>