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