| Message ID | 1357895645-30359-5-git-send-email-xiawenc@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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; > }
于 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; >> } >
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. [...]
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).
于 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. > > [...] >
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; }
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(-)