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

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

[Alon Levy]

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

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
(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

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

[Alon Levy]

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

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

[Anthony Liguori]

On 08/03/2011 06:57 AM, 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
>
> 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
> (1+2)+3      | 3, 3              | 3, 3

I vote for just removing the expression parsing entirely.  It's 
incredibly non-intuitive and I don't think anyone really uses it.

Does anyone strongly object?

Regards,

Anthony Liguori

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

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> On 08/03/2011 06:57 AM, 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
>>
>> 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
>> (1+2)+3      | 3, 3              | 3, 3
>
> I vote for just removing the expression parsing entirely.  It's
> incredibly non-intuitive and I don't think anyone really uses it.

Yes, please.

> Does anyone strongly object?

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

[Blue Swirl]

On Fri, Aug 5, 2011 at 4:51 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 08/03/2011 06:57 AM, 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
>>
>> 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
>> (1+2)+3      | 3, 3              | 3, 3
>
> I vote for just removing the expression parsing entirely.  It's incredibly
> non-intuitive and I don't think anyone really uses it.
>
> Does anyone strongly object?

I think the expressions would be useful with memory addresses, like
"xp/i $pc-4", but I usually start GDB in these cases. Can we disable
the expressions only for ports?

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

[Anthony Liguori]

On 08/05/2011 03:39 PM, Blue Swirl wrote:
> On Fri, Aug 5, 2011 at 4:51 PM, Anthony Liguori<anthony@codemonkey.ws>  wrote:
>> On 08/03/2011 06:57 AM, 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
>>>
>>> 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
>>> (1+2)+3      | 3, 3              | 3, 3
>>
>> I vote for just removing the expression parsing entirely.  It's incredibly
>> non-intuitive and I don't think anyone really uses it.
>>
>> Does anyone strongly object?
>
> I think the expressions would be useful with memory addresses, like
> "xp/i $pc-4", but I usually start GDB in these cases. Can we disable
> the expressions only for ports?

Not sure what you mean by ports.  You mean for anything but vc?  My goal 
in disabling the expressions would be to simplify the parsing by 
removing all that messy code.

Regards,

Anthony Liguori

>

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

[Blue Swirl]

On Fri, Aug 5, 2011 at 9:08 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 08/05/2011 03:39 PM, Blue Swirl wrote:
>>
>> On Fri, Aug 5, 2011 at 4:51 PM, Anthony Liguori<anthony@codemonkey.ws>
>>  wrote:
>>>
>>> On 08/03/2011 06:57 AM, 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
>>>>
>>>> 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
>>>> (1+2)+3      | 3, 3              | 3, 3
>>>
>>> I vote for just removing the expression parsing entirely.  It's
>>> incredibly
>>> non-intuitive and I don't think anyone really uses it.
>>>
>>> Does anyone strongly object?
>>
>> I think the expressions would be useful with memory addresses, like
>> "xp/i $pc-4", but I usually start GDB in these cases. Can we disable
>> the expressions only for ports?
>
> Not sure what you mean by ports.  You mean for anything but vc?  My goal in
> disabling the expressions would be to simplify the parsing by removing all
> that messy code.

Retain the parsing for only memory addresses, remove from other areas.
Another way would be to require any expressions to be enclosed in
parentheses for all cases.

But I don't object to removing the code very much, as I said I use
GDB. Also the setjmp stuff is buggy.

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

[Markus Armbruster]

Blue Swirl <blauwirbel@gmail.com> writes:

> On Fri, Aug 5, 2011 at 9:08 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
>> On 08/05/2011 03:39 PM, Blue Swirl wrote:
>>>
>>> On Fri, Aug 5, 2011 at 4:51 PM, Anthony Liguori<anthony@codemonkey.ws>
>>>  wrote:
>>>>
>>>> On 08/03/2011 06:57 AM, 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
>>>>>
>>>>> 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
>>>>> (1+2)+3      | 3, 3              | 3, 3
>>>>
>>>> I vote for just removing the expression parsing entirely.  It's
>>>> incredibly
>>>> non-intuitive and I don't think anyone really uses it.
>>>>
>>>> Does anyone strongly object?
>>>
>>> I think the expressions would be useful with memory addresses, like
>>> "xp/i $pc-4", but I usually start GDB in these cases. Can we disable
>>> the expressions only for ports?
>>
>> Not sure what you mean by ports.  You mean for anything but vc?  My goal in
>> disabling the expressions would be to simplify the parsing by removing all
>> that messy code.
>
> Retain the parsing for only memory addresses, remove from other areas.

Feasible, but we'd still be open to ambiguities around addresses, and
we'd still be maintaining all that messy code.

> Another way would be to require any expressions to be enclosed in
> parentheses for all cases.

Reduces the ambiguities, but some remain.

Is (1 + 2) one argument (which can evaluate into the integer 3), or
three arguments (which can evaluate into the strings/filenames/whatever
"(1", "+" and "2)")?  Depends on argument types, just like it does
without parenthesis.

> But I don't object to removing the code very much, as I said I use
> GDB. Also the setjmp stuff is buggy.

We have more important problems to solve than providing our users with
yet another pocket calculator.