Patchwork [V5,4/6] HMP: filter out space before check of sub-command

login
register
mail settings
Submitter Wayne Xia
Date Jan. 11, 2013, 9:14 a.m.
Message ID <1357895645-30359-5-git-send-email-xiawenc@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/211253/
State New
Headers show

Comments

Wayne Xia - Jan. 11, 2013, 9:14 a.m.
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(-)
Luiz Capitulino - Jan. 11, 2013, 8:12 p.m.
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?

Actually, I guess that you could skip the spaces after the
search_dispatch_table() call (using p) and drop p1. But please test it
before doing so :)

Otherwise series looks good to me.

>          if (*p1 == '\0') {
>              return cmd;
>          }
Wayne Xia - Jan. 14, 2013, 2:11 a.m.
于 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 and I think this make things clear. I am OK
to merge it and drop p1.

> Actually, I guess that you could skip the spaces after the
> search_dispatch_table() call (using p) and drop p1. But please test it
> before doing so :)
>
> Otherwise series looks good to me.
>
>>           if (*p1 == '\0') {
>>               return cmd;
>>           }
>
Markus Armbruster - Jan. 14, 2013, 1:46 p.m.
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.

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.

[...]
Luiz Capitulino - Jan. 14, 2013, 3:33 p.m.
On Mon, 14 Jan 2013 14:46:35 +0100
Markus Armbruster <armbru@redhat.com> wrote:

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

That was my point in my second paragraph (which cut out).
Wayne Xia - Jan. 15, 2013, 3:06 a.m.
于 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.
>
> [...]
>

Patch

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++;
+        }
         if (*p1 == '\0') {
             return cmd;
         }