[Qemu-devel] [PATCH] monitor: report entirety of hmp command on error

[Qemu-devel] [PATCH] monitor: report entirety of hmp command on error

[Collin Walling]

When a user incorrectly provides an hmp command, an error response will be 
printed that prompts the user to try "help <command name>". However, when
the command contains multiple parts e.g. "info skeys", only the last 
whitespace delimited string will be reported (in this example "info" will 
be dropped and the message will read "Try "help skeys" for more information",
which is incorrect).

Let's correct this by capturing the full name of the command as we recurse 
through the function monitor_parse_command.

Reported-by: Mikhail Fokin <fokin@de.ibm.com>
Signed-off-by: Collin Walling <walling@linux.ibm.com>
---
 monitor.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/monitor.c b/monitor.c
index 39f8ee1..d4844b4 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2964,7 +2964,8 @@ static const mon_cmd_t *search_dispatch_table(const mon_cmd_t *disp_table,
 static const mon_cmd_t *monitor_parse_command(Monitor *mon,
                                               const char *cmdp_start,
                                               const char **cmdp,
-                                              mon_cmd_t *table)
+                                              mon_cmd_t *table,
+                                              char *fullname)
 {
     const char *p;
     const mon_cmd_t *cmd;
@@ -2987,10 +2988,14 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
         p++;
     }
 
+    strncat(fullname, cmdname, strlen(cmdname));
+
     *cmdp = p;
     /* search sub command */
     if (cmd->sub_table != NULL && *p != '\0') {
-        return monitor_parse_command(mon, cmdp_start, cmdp, cmd->sub_table);
+        strncat(fullname, " ", 1);
+        return monitor_parse_command(mon, cmdp_start, cmdp, cmd->sub_table,
+                                     fullname);
     }
 
     return cmd;
@@ -3371,10 +3376,12 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline)
 {
     QDict *qdict;
     const mon_cmd_t *cmd;
+    char fullname[256];
 
     trace_handle_hmp_command(mon, cmdline);
 
-    cmd = monitor_parse_command(mon, cmdline, &cmdline, mon->cmd_table);
+    cmd = monitor_parse_command(mon, cmdline, &cmdline, mon->cmd_table,
+                                fullname);
     if (!cmd) {
         return;
     }
@@ -3382,7 +3389,7 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline)
     qdict = monitor_parse_arguments(mon, &cmdline, cmd);
     if (!qdict) {
         monitor_printf(mon, "Try \"help %s\" for more information\n",
-                       cmd->name);
+                       fullname);
         return;
     }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH] monitor: report entirety of hmp command on error

[Eric Blake]

On 05/04/2018 09:49 AM, Collin Walling wrote:
> When a user incorrectly provides an hmp command, an error response will be
> printed that prompts the user to try "help <command name>". However, when
> the command contains multiple parts e.g. "info skeys", only the last
> whitespace delimited string will be reported (in this example "info" will
> be dropped and the message will read "Try "help skeys" for more information",
> which is incorrect).

What's the exact formula for reproducing this?  I tried:

$ ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic --monitor stdio
QEMU 2.12.50 monitor - type 'help' for more information
(qemu) info skeys
unknown command: 'info skeys'

Oh, I see now:

(qemu) info uuid blah
uuid: extraneous characters at the end of line
Try "help uuid" for more information
(qemu) help uuid
(qemu)

You'll want to update your commit message to document something that is 
reproducible (you may be adding an 'info skeys', but until that is in, 
it doesn't make a good example).

> 
> Let's correct this by capturing the full name of the command as we recurse
> through the function monitor_parse_command.
> 
> Reported-by: Mikhail Fokin <fokin@de.ibm.com>
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---
>   monitor.c | 15 +++++++++++----
>   1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 39f8ee1..d4844b4 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2964,7 +2964,8 @@ static const mon_cmd_t *search_dispatch_table(const mon_cmd_t *disp_table,
>   static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>                                                 const char *cmdp_start,
>                                                 const char **cmdp,
> -                                              mon_cmd_t *table)
> +                                              mon_cmd_t *table,
> +                                              char *fullname)

Umm, how is fullname any better than cmdp_start that we already have?

>   {
>       const char *p;
>       const mon_cmd_t *cmd;
> @@ -2987,10 +2988,14 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>           p++;
>       }
>   
> +    strncat(fullname, cmdname, strlen(cmdname));

gcc 8 is pickier about using strncat() [perhaps too picky - see 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85602], but it is generally 
NOT the function you want to be using.

> +
>       *cmdp = p;
>       /* search sub command */
>       if (cmd->sub_table != NULL && *p != '\0') {
> -        return monitor_parse_command(mon, cmdp_start, cmdp, cmd->sub_table);
> +        strncat(fullname, " ", 1);
> +        return monitor_parse_command(mon, cmdp_start, cmdp, cmd->sub_table,
> +                                     fullname);

See, you're reconstructing a command into fullname, which already 
matches the original command in cmdp_start, so I see no reason to change 
the signature.

>       }
>   
>       return cmd;
> @@ -3371,10 +3376,12 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline)
>   {
>       QDict *qdict;
>       const mon_cmd_t *cmd;
> +    char fullname[256];

EWWW. Don't do that.  You are just ASKING for a buffer overflow exploit 
that prints the wrong thing or causes a security hole, when I 
intentionally type a super-long garbage command into HMP.

>   
>       trace_handle_hmp_command(mon, cmdline);
>   
> -    cmd = monitor_parse_command(mon, cmdline, &cmdline, mon->cmd_table);
> +    cmd = monitor_parse_command(mon, cmdline, &cmdline, mon->cmd_table,
> +                                fullname);

Note that even without your patch, this call updates 'cmdline' to point 
to the position within the original string (although that position has 
already skipped spaces).

>       if (!cmd) {
>           return;
>       }
> @@ -3382,7 +3389,7 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline)
>       qdict = monitor_parse_arguments(mon, &cmdline, cmd);
>       if (!qdict) {
>           monitor_printf(mon, "Try \"help %s\" for more information\n",
> -                       cmd->name);
> +                       fullname);

So rather than trying to reconstruct a string, you could reuse what you 
already have.  This is a shorter patch that I think accomplishes the 
same goal:

diff --git i/monitor.c w/monitor.c
index 39f8ee17ba7..38736b3a20d 100644
--- i/monitor.c
+++ w/monitor.c
@@ -3371,6 +3371,7 @@ static void handle_hmp_command(Monitor *mon, const 
char *cmdline)
  {
      QDict *qdict;
      const mon_cmd_t *cmd;
+    const char *cmd_start = cmdline;

      trace_handle_hmp_command(mon, cmdline);

@@ -3381,8 +3382,11 @@ static void handle_hmp_command(Monitor *mon, 
const char *cmdline)

      qdict = monitor_parse_arguments(mon, &cmdline, cmd);
      if (!qdict) {
-        monitor_printf(mon, "Try \"help %s\" for more information\n",
-                       cmd->name);
+        while (cmdline > cmd_start && qemu_isspace(cmdline[-1])) {
+            cmdline--;
+        }
+        monitor_printf(mon, "Try \"help %.*s\" for more information\n",
+                       (int)(cmdline - cmd_start), cmd_start);
          return;
      }



-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH] monitor: report entirety of hmp command on error

[Dr. David Alan Gilbert]

* Eric Blake (eblake@redhat.com) wrote:
> On 05/04/2018 09:49 AM, Collin Walling wrote:
> > When a user incorrectly provides an hmp command, an error response will be
> > printed that prompts the user to try "help <command name>". However, when
> > the command contains multiple parts e.g. "info skeys", only the last
> > whitespace delimited string will be reported (in this example "info" will
> > be dropped and the message will read "Try "help skeys" for more information",
> > which is incorrect).
> 
> What's the exact formula for reproducing this?  I tried:
> 
> $ ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic --monitor stdio
> QEMU 2.12.50 monitor - type 'help' for more information
> (qemu) info skeys
> unknown command: 'info skeys'


Ah, so I fixed the 'info skeys' case in 250b8197640
and that ended up using cmdp_start

> Oh, I see now:
> 
> (qemu) info uuid blah
> uuid: extraneous characters at the end of line
> Try "help uuid" for more information

Yep that's fair that needs fixing.

Dave

> (qemu) help uuid
> (qemu)

> You'll want to update your commit message to document something that is
> reproducible (you may be adding an 'info skeys', but until that is in, it
> doesn't make a good example).
> 
> > 
> > Let's correct this by capturing the full name of the command as we recurse
> > through the function monitor_parse_command.
> > 
> > Reported-by: Mikhail Fokin <fokin@de.ibm.com>
> > Signed-off-by: Collin Walling <walling@linux.ibm.com>
> > ---
> >   monitor.c | 15 +++++++++++----
> >   1 file changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/monitor.c b/monitor.c
> > index 39f8ee1..d4844b4 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -2964,7 +2964,8 @@ static const mon_cmd_t *search_dispatch_table(const mon_cmd_t *disp_table,
> >   static const mon_cmd_t *monitor_parse_command(Monitor *mon,
> >                                                 const char *cmdp_start,
> >                                                 const char **cmdp,
> > -                                              mon_cmd_t *table)
> > +                                              mon_cmd_t *table,
> > +                                              char *fullname)
> 
> Umm, how is fullname any better than cmdp_start that we already have?
> 
> >   {
> >       const char *p;
> >       const mon_cmd_t *cmd;
> > @@ -2987,10 +2988,14 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
> >           p++;
> >       }
> > +    strncat(fullname, cmdname, strlen(cmdname));
> 
> gcc 8 is pickier about using strncat() [perhaps too picky - see
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85602], but it is generally NOT
> the function you want to be using.
> 
> > +
> >       *cmdp = p;
> >       /* search sub command */
> >       if (cmd->sub_table != NULL && *p != '\0') {
> > -        return monitor_parse_command(mon, cmdp_start, cmdp, cmd->sub_table);
> > +        strncat(fullname, " ", 1);
> > +        return monitor_parse_command(mon, cmdp_start, cmdp, cmd->sub_table,
> > +                                     fullname);
> 
> See, you're reconstructing a command into fullname, which already matches
> the original command in cmdp_start, so I see no reason to change the
> signature.
> 
> >       }
> >       return cmd;
> > @@ -3371,10 +3376,12 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline)
> >   {
> >       QDict *qdict;
> >       const mon_cmd_t *cmd;
> > +    char fullname[256];
> 
> EWWW. Don't do that.  You are just ASKING for a buffer overflow exploit that
> prints the wrong thing or causes a security hole, when I intentionally type
> a super-long garbage command into HMP.
> 
> >       trace_handle_hmp_command(mon, cmdline);
> > -    cmd = monitor_parse_command(mon, cmdline, &cmdline, mon->cmd_table);
> > +    cmd = monitor_parse_command(mon, cmdline, &cmdline, mon->cmd_table,
> > +                                fullname);
> 
> Note that even without your patch, this call updates 'cmdline' to point to
> the position within the original string (although that position has already
> skipped spaces).
> 
> >       if (!cmd) {
> >           return;
> >       }
> > @@ -3382,7 +3389,7 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline)
> >       qdict = monitor_parse_arguments(mon, &cmdline, cmd);
> >       if (!qdict) {
> >           monitor_printf(mon, "Try \"help %s\" for more information\n",
> > -                       cmd->name);
> > +                       fullname);
> 
> So rather than trying to reconstruct a string, you could reuse what you
> already have.  This is a shorter patch that I think accomplishes the same
> goal:
> 
> diff --git i/monitor.c w/monitor.c
> index 39f8ee17ba7..38736b3a20d 100644
> --- i/monitor.c
> +++ w/monitor.c
> @@ -3371,6 +3371,7 @@ static void handle_hmp_command(Monitor *mon, const
> char *cmdline)
>  {
>      QDict *qdict;
>      const mon_cmd_t *cmd;
> +    const char *cmd_start = cmdline;
> 
>      trace_handle_hmp_command(mon, cmdline);
> 
> @@ -3381,8 +3382,11 @@ static void handle_hmp_command(Monitor *mon, const
> char *cmdline)
> 
>      qdict = monitor_parse_arguments(mon, &cmdline, cmd);
>      if (!qdict) {
> -        monitor_printf(mon, "Try \"help %s\" for more information\n",
> -                       cmd->name);
> +        while (cmdline > cmd_start && qemu_isspace(cmdline[-1])) {
> +            cmdline--;
> +        }
> +        monitor_printf(mon, "Try \"help %.*s\" for more information\n",
> +                       (int)(cmdline - cmd_start), cmd_start);
>          return;
>      }
> 
> 
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] monitor: report entirety of hmp command on error

[Collin Walling]

On 05/04/2018 11:19 AM, Eric Blake wrote:
> On 05/04/2018 09:49 AM, Collin Walling wrote:
>> When a user incorrectly provides an hmp command, an error response will be
>> printed that prompts the user to try "help <command name>". However, when
>> the command contains multiple parts e.g. "info skeys", only the last
>> whitespace delimited string will be reported (in this example "info" will
>> be dropped and the message will read "Try "help skeys" for more information",
>> which is incorrect).
> 
> What's the exact formula for reproducing this?  I tried:
> 
> $ ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic --monitor stdio
> QEMU 2.12.50 monitor - type 'help' for more information
> (qemu) info skeys
> unknown command: 'info skeys'
> 
> Oh, I see now:
> 
> (qemu) info uuid blah
> uuid: extraneous characters at the end of line
> Try "help uuid" for more information
> (qemu) help uuid
> (qemu)
> 
> You'll want to update your commit message to document something that is reproducible (you may be adding an 'info skeys', but until that is in, it doesn't make a good example).
> 
>>
>> Let's correct this by capturing the full name of the command as we recurse
>> through the function monitor_parse_command.
>>
>> Reported-by: Mikhail Fokin <fokin@de.ibm.com>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> ---
>>   monitor.c | 15 +++++++++++----
>>   1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/monitor.c b/monitor.c
>> index 39f8ee1..d4844b4 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -2964,7 +2964,8 @@ static const mon_cmd_t *search_dispatch_table(const mon_cmd_t *disp_table,
>>   static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>>                                                 const char *cmdp_start,
>>                                                 const char **cmdp,
>> -                                              mon_cmd_t *table)
>> +                                              mon_cmd_t *table,
>> +                                              char *fullname)
> 
> Umm, how is fullname any better than cmdp_start that we already have?
> 
>>   {
>>       const char *p;
>>       const mon_cmd_t *cmd;
>> @@ -2987,10 +2988,14 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>>           p++;
>>       }
>>   +    strncat(fullname, cmdname, strlen(cmdname));
> 
> gcc 8 is pickier about using strncat() [perhaps too picky - see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85602], but it is generally NOT the function you want to be using.
> 
>> +
>>       *cmdp = p;
>>       /* search sub command */
>>       if (cmd->sub_table != NULL && *p != '\0') {
>> -        return monitor_parse_command(mon, cmdp_start, cmdp, cmd->sub_table);
>> +        strncat(fullname, " ", 1);
>> +        return monitor_parse_command(mon, cmdp_start, cmdp, cmd->sub_table,
>> +                                     fullname);
> 
> See, you're reconstructing a command into fullname, which already matches the original command in cmdp_start, so I see no reason to change the signature.
> 
>>       }
>>         return cmd;
>> @@ -3371,10 +3376,12 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline)
>>   {
>>       QDict *qdict;
>>       const mon_cmd_t *cmd;
>> +    char fullname[256];
> 
> EWWW. Don't do that.  You are just ASKING for a buffer overflow exploit that prints the wrong thing or causes a security hole, when I intentionally type a super-long garbage command into HMP.
> 
>>         trace_handle_hmp_command(mon, cmdline);
>>   -    cmd = monitor_parse_command(mon, cmdline, &cmdline, mon->cmd_table);
>> +    cmd = monitor_parse_command(mon, cmdline, &cmdline, mon->cmd_table,
>> +                                fullname);
> 
> Note that even without your patch, this call updates 'cmdline' to point to the position within the original string (although that position has already skipped spaces).
> 
>>       if (!cmd) {
>>           return;
>>       }
>> @@ -3382,7 +3389,7 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline)
>>       qdict = monitor_parse_arguments(mon, &cmdline, cmd);
>>       if (!qdict) {
>>           monitor_printf(mon, "Try \"help %s\" for more information\n",
>> -                       cmd->name);
>> +                       fullname);
> 
> So rather than trying to reconstruct a string, you could reuse what you already have.  This is a shorter patch that I think accomplishes the same goal:
> 
> diff --git i/monitor.c w/monitor.c
> index 39f8ee17ba7..38736b3a20d 100644
> --- i/monitor.c
> +++ w/monitor.c
> @@ -3371,6 +3371,7 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline)
>  {
>      QDict *qdict;
>      const mon_cmd_t *cmd;
> +    const char *cmd_start = cmdline;
> 
>      trace_handle_hmp_command(mon, cmdline);
> 
> @@ -3381,8 +3382,11 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline)
> 
>      qdict = monitor_parse_arguments(mon, &cmdline, cmd);
>      if (!qdict) {
> -        monitor_printf(mon, "Try \"help %s\" for more information\n",
> -                       cmd->name);
> +        while (cmdline > cmd_start && qemu_isspace(cmdline[-1])) {
> +            cmdline--;
> +        }
> +        monitor_printf(mon, "Try \"help %.*s\" for more information\n",
> +                       (int)(cmdline - cmd_start), cmd_start);
>          return;
>      }
> 
> 
> 

Very interesting... you managed to reuse what was in cmdline without printing anything extraneous that
the user might have provided... nicely done!

Your print statement is intriguing to me... I'm not entirely sure how it works.

How would you like to move forward with this patch?

-- 
Respectfully,
- Collin Walling

Re: [Qemu-devel] [PATCH] monitor: report entirety of hmp command on error

[Eric Blake]

On 05/04/2018 01:02 PM, Collin Walling wrote:

>> So rather than trying to reconstruct a string, you could reuse what you already have.  This is a shorter patch that I think accomplishes the same goal:
>>
>> diff --git i/monitor.c w/monitor.c
>> index 39f8ee17ba7..38736b3a20d 100644
>> --- i/monitor.c
>> +++ w/monitor.c
>> @@ -3371,6 +3371,7 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline)
>>   {
>>       QDict *qdict;
>>       const mon_cmd_t *cmd;
>> +    const char *cmd_start = cmdline;
>>
>>       trace_handle_hmp_command(mon, cmdline);
>>
>> @@ -3381,8 +3382,11 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline)
>>
>>       qdict = monitor_parse_arguments(mon, &cmdline, cmd);
>>       if (!qdict) {
>> -        monitor_printf(mon, "Try \"help %s\" for more information\n",
>> -                       cmd->name);
>> +        while (cmdline > cmd_start && qemu_isspace(cmdline[-1])) {
>> +            cmdline--;
>> +        }
>> +        monitor_printf(mon, "Try \"help %.*s\" for more information\n",
>> +                       (int)(cmdline - cmd_start), cmd_start);
>>           return;
>>       }
>>
>>
>>
> 
> Very interesting... you managed to reuse what was in cmdline without printing anything extraneous that
> the user might have provided... nicely done!
> 
> Your print statement is intriguing to me... I'm not entirely sure how it works.

The format specifiers in printf are %[flags][width][.precision]format. 
So I'm requesting a precision-limited string print (which says the 
maximum number of characters to print, rather than the usual semantics 
of printing until the trailing NUL is found), and the precision of .* 
(instead of a more typical .1 or similar) says that the precision will 
be an int argument to printf rather than inline (the width argument can 
also be passed via *).  The cast to int is annoyingly part of the 
specification (subtracting two pointers within a string results in a 
ptrdiff_t, but on 64-bit platforms, ptrdiff_t and int are not equally 
handled through vararg functions).  And that exact style of printf() 
magic was already in use in monitor_parse_command() that your patch 
attempt was touching (see where cmdp_start is used).  And if you really 
want weird, 'man 3 printf' states that "%.*s" is equivalent to 
"%2$.*1$s" (the $ syntax is for cases cases where you need to consume 
printf arguments out-of-order, often when dealing with translated strings).

> 
> How would you like to move forward with this patch?

I'm more than happy to let you post a v2 of the patch, incorporating the 
ideas you just learned from me.  (Or, if you do nothing, then in a week 
or so, I'll probably notice the patch is still sitting unapplied in my 
local repository and submit it myself - but then I wouldn't be helping 
the qemu community grow...)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH] monitor: report entirety of hmp command on error

[Collin Walling]

On 05/04/2018 02:19 PM, Eric Blake wrote:
> On 05/04/2018 01:02 PM, Collin Walling wrote:
> 
>>> So rather than trying to reconstruct a string, you could reuse what you already have.  This is a shorter patch that I think accomplishes the same goal:
>>>
>>> diff --git i/monitor.c w/monitor.c
>>> index 39f8ee17ba7..38736b3a20d 100644
>>> --- i/monitor.c
>>> +++ w/monitor.c
>>> @@ -3371,6 +3371,7 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline)
>>>   {
>>>       QDict *qdict;
>>>       const mon_cmd_t *cmd;
>>> +    const char *cmd_start = cmdline;
>>>
>>>       trace_handle_hmp_command(mon, cmdline);
>>>
>>> @@ -3381,8 +3382,11 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline)
>>>
>>>       qdict = monitor_parse_arguments(mon, &cmdline, cmd);
>>>       if (!qdict) {
>>> -        monitor_printf(mon, "Try \"help %s\" for more information\n",
>>> -                       cmd->name);
>>> +        while (cmdline > cmd_start && qemu_isspace(cmdline[-1])) {
>>> +            cmdline--;
>>> +        }
>>> +        monitor_printf(mon, "Try \"help %.*s\" for more information\n",
>>> +                       (int)(cmdline - cmd_start), cmd_start);
>>>           return;
>>>       }
>>>
>>>
>>>
>>
>> Very interesting... you managed to reuse what was in cmdline without printing anything extraneous that
>> the user might have provided... nicely done!
>>
>> Your print statement is intriguing to me... I'm not entirely sure how it works.
> 
> The format specifiers in printf are %[flags][width][.precision]format. So I'm requesting a precision-limited string print (which says the maximum number of characters to print, rather than the usual semantics of printing until the trailing NUL is found), and the precision of .* (instead of a more typical .1 or similar) says that the precision will be an int argument to printf rather than inline (the width argument can also be passed via *).  The cast to int is annoyingly part of the specification (subtracting two pointers within a string results in a ptrdiff_t, but on 64-bit platforms, ptrdiff_t and int are not equally handled through vararg functions).  And that exact style of printf() magic was already in use in monitor_parse_command() that your patch attempt was touching (see where cmdp_start is used).  And if you really want weird, 'man 3 printf' states that "%.*s" is equivalent to "%2$.*1$s" (the $ syntax is for cases cases where you need to consume printf arguments
> out-of-order, often when dealing with translated strings).
> 

Very cool! Thank you for clearing that up for me. I must've glanced over the usage in monitor_parse_command().

>>
>> How would you like to move forward with this patch?
> 
> I'm more than happy to let you post a v2 of the patch, incorporating the ideas you just learned from me.  (Or, if you do nothing, then in a week or so, I'll probably notice the patch is still sitting unapplied in my local repository and submit it myself - but then I wouldn't be helping the qemu community grow...)
> 

Much appreciated. I'll post v2 as a reply to this email chain (since it's very small and I don't expect much discussion to follow)
with your suggested changes.


-- 
Respectfully,
- Collin Walling

Re: [Qemu-devel] [PATCH v2] monitor: report entirety of hmp command on error

[Collin Walling]

When a user incorrectly provides an hmp command, an error response will be
printed that prompts the user to try "help <command name>". However, when
the command contains multiple parts e.g. "info uuid xyz", only the last
whitespace delimited string will be reported (in this example "info" will
be dropped and the message will read "Try "help uuid" for more information",
which is incorrect).

Let's correct this by capturing the entirety of the command from the command
line -- excluding any extraneous characters.

Reported-by: Mikhail Fokin <fokin@de.ibm.com>
Signed-off-by: Collin Walling <walling@linux.ibm.com>
---
 monitor.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/monitor.c b/monitor.c
index 39f8ee1..38736b3 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3371,6 +3371,7 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline)
 {
     QDict *qdict;
     const mon_cmd_t *cmd;
+    const char *cmd_start = cmdline;
 
     trace_handle_hmp_command(mon, cmdline);
 
@@ -3381,8 +3382,11 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline)
 
     qdict = monitor_parse_arguments(mon, &cmdline, cmd);
     if (!qdict) {
-        monitor_printf(mon, "Try \"help %s\" for more information\n",
-                       cmd->name);
+        while (cmdline > cmd_start && qemu_isspace(cmdline[-1])) {
+            cmdline--;
+        }
+        monitor_printf(mon, "Try \"help %.*s\" for more information\n",
+                       (int)(cmdline - cmd_start), cmd_start);
         return;
     }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2] monitor: report entirety of hmp command on error

[Eric Blake]

On 05/07/2018 09:30 AM, Collin Walling wrote:
> When a user incorrectly provides an hmp command, an error response will be
> printed that prompts the user to try "help <command name>". However, when
> the command contains multiple parts e.g. "info uuid xyz", only the last
> whitespace delimited string will be reported (in this example "info" will
> be dropped and the message will read "Try "help uuid" for more information",
> which is incorrect).
> 
> Let's correct this by capturing the entirety of the command from the command
> line -- excluding any extraneous characters.
> 

It's better to post a v2 patch as a new top-level thread instead of 
in-reply to an earlier version, as some of our automated tooling is more 
likely to see it.

> Reported-by: Mikhail Fokin <fokin@de.ibm.com>
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---
>   monitor.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v2] monitor: report entirety of hmp command on error

[Collin Walling]

On 05/07/2018 12:44 PM, Eric Blake wrote:
> On 05/07/2018 09:30 AM, Collin Walling wrote:
>> When a user incorrectly provides an hmp command, an error response will be
>> printed that prompts the user to try "help <command name>". However, when
>> the command contains multiple parts e.g. "info uuid xyz", only the last
>> whitespace delimited string will be reported (in this example "info" will
>> be dropped and the message will read "Try "help uuid" for more information",
>> which is incorrect).
>>
>> Let's correct this by capturing the entirety of the command from the command
>> line -- excluding any extraneous characters.
>>
> 
> It's better to post a v2 patch as a new top-level thread instead of in-reply to an earlier version, as some of our automated tooling is more likely to see it.
> 
>> Reported-by: Mikhail Fokin <fokin@de.ibm.com>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> ---
>>   monitor.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Noted. And thank you for the r-b.

-- 
Respectfully,
- Collin Walling

Re: [Qemu-devel] [PATCH v2] monitor: report entirety of hmp command on error

[Markus Armbruster]

David, looks like your turf.

Collin Walling <walling@linux.ibm.com> writes:

> When a user incorrectly provides an hmp command, an error response will be
> printed that prompts the user to try "help <command name>". However, when
> the command contains multiple parts e.g. "info uuid xyz", only the last
> whitespace delimited string will be reported (in this example "info" will
> be dropped and the message will read "Try "help uuid" for more information",
> which is incorrect).
>
> Let's correct this by capturing the entirety of the command from the command
> line -- excluding any extraneous characters.
>
> Reported-by: Mikhail Fokin <fokin@de.ibm.com>
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---
>  monitor.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 39f8ee1..38736b3 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3371,6 +3371,7 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline)
>  {
>      QDict *qdict;
>      const mon_cmd_t *cmd;
> +    const char *cmd_start = cmdline;
>  
>      trace_handle_hmp_command(mon, cmdline);
>  
> @@ -3381,8 +3382,11 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline)
>  
>      qdict = monitor_parse_arguments(mon, &cmdline, cmd);
>      if (!qdict) {
> -        monitor_printf(mon, "Try \"help %s\" for more information\n",
> -                       cmd->name);
> +        while (cmdline > cmd_start && qemu_isspace(cmdline[-1])) {
> +            cmdline--;
> +        }
> +        monitor_printf(mon, "Try \"help %.*s\" for more information\n",
> +                       (int)(cmdline - cmd_start), cmd_start);
>          return;
>      }

Re: [Qemu-devel] [PATCH v2] monitor: report entirety of hmp command on error

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> David, looks like your turf.

Yep, I've got it on my list to take.

Dave

> Collin Walling <walling@linux.ibm.com> writes:
> 
> > When a user incorrectly provides an hmp command, an error response will be
> > printed that prompts the user to try "help <command name>". However, when
> > the command contains multiple parts e.g. "info uuid xyz", only the last
> > whitespace delimited string will be reported (in this example "info" will
> > be dropped and the message will read "Try "help uuid" for more information",
> > which is incorrect).
> >
> > Let's correct this by capturing the entirety of the command from the command
> > line -- excluding any extraneous characters.
> >
> > Reported-by: Mikhail Fokin <fokin@de.ibm.com>
> > Signed-off-by: Collin Walling <walling@linux.ibm.com>
> > ---
> >  monitor.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 39f8ee1..38736b3 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -3371,6 +3371,7 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline)
> >  {
> >      QDict *qdict;
> >      const mon_cmd_t *cmd;
> > +    const char *cmd_start = cmdline;
> >  
> >      trace_handle_hmp_command(mon, cmdline);
> >  
> > @@ -3381,8 +3382,11 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline)
> >  
> >      qdict = monitor_parse_arguments(mon, &cmdline, cmd);
> >      if (!qdict) {
> > -        monitor_printf(mon, "Try \"help %s\" for more information\n",
> > -                       cmd->name);
> > +        while (cmdline > cmd_start && qemu_isspace(cmdline[-1])) {
> > +            cmdline--;
> > +        }
> > +        monitor_printf(mon, "Try \"help %.*s\" for more information\n",
> > +                       (int)(cmdline - cmd_start), cmd_start);
> >          return;
> >      }
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2] monitor: report entirety of hmp command on error

[Dr. David Alan Gilbert]

* Collin Walling (walling@linux.ibm.com) wrote:
> When a user incorrectly provides an hmp command, an error response will be
> printed that prompts the user to try "help <command name>". However, when
> the command contains multiple parts e.g. "info uuid xyz", only the last
> whitespace delimited string will be reported (in this example "info" will
> be dropped and the message will read "Try "help uuid" for more information",
> which is incorrect).
> 
> Let's correct this by capturing the entirety of the command from the command
> line -- excluding any extraneous characters.
> 
> Reported-by: Mikhail Fokin <fokin@de.ibm.com>
> Signed-off-by: Collin Walling <walling@linux.ibm.com>

Queued

> ---
>  monitor.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 39f8ee1..38736b3 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3371,6 +3371,7 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline)
>  {
>      QDict *qdict;
>      const mon_cmd_t *cmd;
> +    const char *cmd_start = cmdline;
>  
>      trace_handle_hmp_command(mon, cmdline);
>  
> @@ -3381,8 +3382,11 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline)
>  
>      qdict = monitor_parse_arguments(mon, &cmdline, cmd);
>      if (!qdict) {
> -        monitor_printf(mon, "Try \"help %s\" for more information\n",
> -                       cmd->name);
> +        while (cmdline > cmd_start && qemu_isspace(cmdline[-1])) {
> +            cmdline--;
> +        }
> +        monitor_printf(mon, "Try \"help %.*s\" for more information\n",
> +                       (int)(cmdline - cmd_start), cmd_start);
>          return;
>      }
>  
> -- 
> 2.7.4
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2] monitor: report entirety of hmp command on error

[Collin Walling]

On 06/21/2018 07:19 AM, Dr. David Alan Gilbert wrote:
> * Collin Walling (walling@linux.ibm.com) wrote:
>> When a user incorrectly provides an hmp command, an error response will be
>> printed that prompts the user to try "help <command name>". However, when
>> the command contains multiple parts e.g. "info uuid xyz", only the last
>> whitespace delimited string will be reported (in this example "info" will
>> be dropped and the message will read "Try "help uuid" for more information",
>> which is incorrect).
>>
>> Let's correct this by capturing the entirety of the command from the command
>> line -- excluding any extraneous characters.
>>
>> Reported-by: Mikhail Fokin <fokin@de.ibm.com>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> 
> Queued

Thank you!

> 
>> ---
>>  monitor.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/monitor.c b/monitor.c
>> index 39f8ee1..38736b3 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -3371,6 +3371,7 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline)
>>  {
>>      QDict *qdict;
>>      const mon_cmd_t *cmd;
>> +    const char *cmd_start = cmdline;
>>  
>>      trace_handle_hmp_command(mon, cmdline);
>>  
>> @@ -3381,8 +3382,11 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline)
>>  
>>      qdict = monitor_parse_arguments(mon, &cmdline, cmd);
>>      if (!qdict) {
>> -        monitor_printf(mon, "Try \"help %s\" for more information\n",
>> -                       cmd->name);
>> +        while (cmdline > cmd_start && qemu_isspace(cmdline[-1])) {
>> +            cmdline--;
>> +        }
>> +        monitor_printf(mon, "Try \"help %.*s\" for more information\n",
>> +                       (int)(cmdline - cmd_start), cmd_start);
>>          return;
>>      }
>>  
>> -- 
>> 2.7.4
>>
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 


-- 
Respectfully,
- Collin Walling

Re: [Qemu-devel] [PATCH] monitor: report entirety of hmp command on error

[Eric Blake]

On 05/04/2018 09:49 AM, Collin Walling wrote:
> When a user incorrectly provides an hmp command, an error response will be
> printed that prompts the user to try "help <command name>". However, when
> the command contains multiple parts e.g. "info skeys", only the last
> whitespace delimited string will be reported (in this example "info" will
> be dropped and the message will read "Try "help skeys" for more information",
> which is incorrect).
> 
> Let's correct this by capturing the full name of the command as we recurse
> through the function monitor_parse_command.
> 
> Reported-by: Mikhail Fokin <fokin@de.ibm.com>
> Signed-off-by: Collin Walling <walling@linux.ibm.com>

Side note:

git shortlog --author=Collin

lists two different spellings for your name based on your previous 
contributions.  You may want to propose a patch to .mailmap to settle on 
the variant that you prefer, so that people grabbing statistics from git 
don't get as confused.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org