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