Re: [Qemu-devel] [PATCH V5 4/6] HMP: filter out space before check of sub-command

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

于 2013-1-14 21:46, Markus Armbruster 写道:
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>
>> 于 2013-1-12 4:12, Luiz Capitulino 写道:
>>> On Fri, 11 Jan 2013 17:14:03 +0800
>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>>
>>>>     This fix the case when user input "@command ". Original
>>>> it will return NULL for monitor_parse_command(), now
>>>> it will return the @command related instance.
>>>>
>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>>> ---
>>>>    monitor.c |    3 +++
>>>>    1 files changed, 3 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/monitor.c b/monitor.c
>>>> index 5435dc3..7b752a2 100644
>>>> --- a/monitor.c
>>>> +++ b/monitor.c
>>>> @@ -3588,6 +3588,9 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>>>>        if (cmd->sub_table != NULL) {
>>>>            p1 = p;
>>>>            /* check if user set additional command */
>>>> +        while (qemu_isspace(*p1)) {
>>>> +            p1++;
>>>> +        }
>>>
>>> Is there a reason for this to be in a different patch? I mean, why don't
>>> you squash this into the previous patch?
>>>
>>    Markus suggest to do it
>
> Misunderstanding, sorry.  My point was we skip over whitespace twice:
> first here, and then again in monitor_parse_command().  Quote:
>
>      The check whether non-space characters follow is awkward.  We need
>      it only because we want to handle "@cmdline is blank" differently
>      than "it can't be parsed", but monitor_parse_command() returns NULL
>      for both cases.
>
> Merely an observation, not a request to do anything about it in this
> series:
>
>      If we care, we can try to do better in a follow-up patch.
>
> A possible way to do better is to have a handler for command "", to be
> invoked for entirely blank lines.  In table info_cmds, the handler would
> be do_info_help.  Then prefix commands don't need their handler anymore
> (it's in the sub_table), and sub_table can go into the mhandler union.
>
   Thanks for the declaration. Have a handler for command "" seems good,
but need a careful check about the parsing code since '\0' is a key
value. *sub_table plays an extra role of tagging if it have sub-commands
so I think it is better to not folder it into mhandler union.

> We'd need to suppress the "unknown command" error for cmdname "".
>
> If this isn't clear, but you want to understand it, I can write a patch
> on top.
>
>>                            and I think this make things clear. I am OK
>> to merge it and drop p1.
>
> I'm not sure it makes things clearer.  It makes behavior of "info"
> without argument change twice in the series, though, in the preceding
> patch, and in this one.  I think I agree with Luiz it's better to squash
> the two together again.
>
> [...]
>


-- 
Best Regards

Wenchao Xia