qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Wenchao Xia
Subject: Re: [Qemu-devel] [PATCH V5 4/6] HMP: filter out space before check of sub-command
Date: Tue, 15 Jan 2013 11:06:27 +0800
User-agent: Mozilla/5.0 (Windows NT 5.1; rv:17.0) Gecko/20130107 Thunderbird/17.0.2

于 2013-1-14 21:46, Markus Armbruster 写道:
Wenchao Xia <address@hidden> writes:

于 2013-1-12 4:12, Luiz Capitulino 写道:
On Fri, 11 Jan 2013 17:14:03 +0800
Wenchao Xia <address@hidden> 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 <address@hidden>
---
   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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]