[V4,2/4] HMP: add infrastructure for sub command

This patch make parsing of hmp command aware of that it may
have sub command. Also discard simple encapsulation function
monitor_find_command() and qmp_find_cmd().

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 monitor.c |   31 +++++++++++++++++++++++--------
 1 files changed, 23 insertions(+), 8 deletions(-)

Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:

>   This patch make parsing of hmp command aware of that it may
> have sub command. Also discard simple encapsulation function
> monitor_find_command() and qmp_find_cmd().
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  monitor.c |   31 +++++++++++++++++++++++--------
>  1 files changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index c7b3014..d49a362 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -130,6 +130,7 @@ typedef struct mon_cmd_t {
>                            MonitorCompletion *cb, void *opaque);
>      } mhandler;
>      int flags;
> +    struct mon_cmd_t *sub_table;
>  } mon_cmd_t;
>  
>  /* file descriptors passed via SCM_RIGHTS */
> @@ -3534,21 +3535,22 @@ static const mon_cmd_t *search_dispatch_table(const mon_cmd_t *disp_table,
>      return NULL;
>  }
>  
> -static const mon_cmd_t *monitor_find_command(const char *cmdname)
> -{
> -    return search_dispatch_table(mon_cmds, cmdname);
> -}
> -
>  static const mon_cmd_t *qmp_find_cmd(const char *cmdname)
>  {
>      return search_dispatch_table(qmp_cmds, cmdname);
>  }
>  
> +/*
> + * Try find the target mon_cmd_t instance. If it have sub_table and have string
> + * for it, this function will try search it with remaining string, and if not
> + * found it return NULL.
> + */

Thanks for going the extra mile and document the function.  What about:

   /*
    * Parse @cmdline according to command table @table.
    * If @cmdline is blank, return NULL.
    * If it can't be parsed, report to @mon, and return NULL.
    * Else, insert command arguments into @qdict, and return the command.
    * Do not assume the returned command points into @table!  It doesn't
    * when the command is a sub-command.
    */

>  static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>                                                const char *cmdline,
> +                                              mon_cmd_t *table,
>                                                QDict *qdict)
>  {
> -    const char *p, *typestr;
> +    const char *p, *p1, *typestr;
>      int c;
>      const mon_cmd_t *cmd;
>      char cmdname[256];
> @@ -3564,12 +3566,25 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>      if (!p)
>          return NULL;
>  
> -    cmd = monitor_find_command(cmdname);
> +    cmd = search_dispatch_table(table, cmdname);
>      if (!cmd) {
>          monitor_printf(mon, "unknown command: '%s'\n", cmdname);
>          return NULL;
>      }
>  
> +    /* search sub command */
> +    if (cmd->sub_table != NULL) {
> +        p1 = p;
> +        /* check if user set additional command */
> +        while (qemu_isspace(*p1)) {
> +            p1++;
> +        }
> +        if (*p1 == '\0') {
> +            return cmd;
> +        }
> +        return monitor_parse_command(mon, p, cmd->sub_table, qdict);
> +    }
> +
>      /* parse the parameters */
>      typestr = cmd->args_type;
>      for(;;) {

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.  If we care, we can try to do better in a follow-up patch.

We'll get sub-optimal error messages when the infrastructure is put to
use in patch 4/4, e.g.:

    (qemu) info apple pie
    unknown command: 'apple'

That's because cmdname is just "apple" at the place where the error is
diagnosed.

Several other messages also print cmdname, and thus are similarly
affected.  Many of them could probably just omit printing it, but that's
a separate issue.

A possible way to improve this: additional parameter int start, parse
starts at cmdline + start, and instead of printing cmdname, print
cmdline up to end of cmdname.  Something like

        monitor_printf(mon, "unknown command: '%.*s'\n",
                       (int)(p - cmdline), cmdline);

> @@ -3925,7 +3940,7 @@ static void handle_user_command(Monitor *mon, const char *cmdline)
>  
>      qdict = qdict_new();
>  
> -    cmd = monitor_parse_command(mon, cmdline, qdict);
> +    cmd = monitor_parse_command(mon, cmdline, mon_cmds, qdict);
>      if (!cmd)
>          goto out;

Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:

>   This patch make parsing of hmp command aware of that it may
> have sub command. Also discard simple encapsulation function
> monitor_find_command() and qmp_find_cmd().
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  monitor.c |   31 +++++++++++++++++++++++--------
>  1 files changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index c7b3014..d49a362 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -130,6 +130,7 @@ typedef struct mon_cmd_t {
>                            MonitorCompletion *cb, void *opaque);
>      } mhandler;
>      int flags;
> +    struct mon_cmd_t *sub_table;
>  } mon_cmd_t;
>  
>  /* file descriptors passed via SCM_RIGHTS */

Forgot to mention: please explain in a comment how mhandler and
sub_table interact.  Namely, if there are arguments, we use sub_table,
else mhandler.

On Thu, 10 Jan 2013 16:32:32 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

>   This patch make parsing of hmp command aware of that it may
> have sub command. Also discard simple encapsulation function
> monitor_find_command() and qmp_find_cmd().

You don't qmp_find_cmd() anymore..

> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> 
>>    This patch make parsing of hmp command aware of that it may
>> have sub command. Also discard simple encapsulation function
>> monitor_find_command() and qmp_find_cmd().
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   monitor.c |   31 +++++++++++++++++++++++--------
>>   1 files changed, 23 insertions(+), 8 deletions(-)
>>
>> diff --git a/monitor.c b/monitor.c
>> index c7b3014..d49a362 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -130,6 +130,7 @@ typedef struct mon_cmd_t {
>>                             MonitorCompletion *cb, void *opaque);
>>       } mhandler;
>>       int flags;
>> +    struct mon_cmd_t *sub_table;
>>   } mon_cmd_t;
>>   
>>   /* file descriptors passed via SCM_RIGHTS */
>> @@ -3534,21 +3535,22 @@ static const mon_cmd_t *search_dispatch_table(const mon_cmd_t *disp_table,
>>       return NULL;
>>   }
>>   
>> -static const mon_cmd_t *monitor_find_command(const char *cmdname)
>> -{
>> -    return search_dispatch_table(mon_cmds, cmdname);
>> -}
>> -
>>   static const mon_cmd_t *qmp_find_cmd(const char *cmdname)
>>   {
>>       return search_dispatch_table(qmp_cmds, cmdname);
>>   }
>>   
>> +/*
>> + * Try find the target mon_cmd_t instance. If it have sub_table and have string
>> + * for it, this function will try search it with remaining string, and if not
>> + * found it return NULL.
>> + */
> 
> Thanks for going the extra mile and document the function.  What about:
> 
>     /*
>      * Parse @cmdline according to command table @table.
>      * If @cmdline is blank, return NULL.
>      * If it can't be parsed, report to @mon, and return NULL.
>      * Else, insert command arguments into @qdict, and return the command.
>      * Do not assume the returned command points into @table!  It doesn't
>      * when the command is a sub-command.
>      */
> 
  OK, I'll use it in V5.

>>   static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>>                                                 const char *cmdline,
>> +                                              mon_cmd_t *table,
>>                                                 QDict *qdict)
>>   {
>> -    const char *p, *typestr;
>> +    const char *p, *p1, *typestr;
>>       int c;
>>       const mon_cmd_t *cmd;
>>       char cmdname[256];
>> @@ -3564,12 +3566,25 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>>       if (!p)
>>           return NULL;
>>   
>> -    cmd = monitor_find_command(cmdname);
>> +    cmd = search_dispatch_table(table, cmdname);
>>       if (!cmd) {
>>           monitor_printf(mon, "unknown command: '%s'\n", cmdname);
>>           return NULL;
>>       }
>>   
>> +    /* search sub command */
>> +    if (cmd->sub_table != NULL) {
>> +        p1 = p;
>> +        /* check if user set additional command */
>> +        while (qemu_isspace(*p1)) {
>> +            p1++;
>> +        }
>> +        if (*p1 == '\0') {
>> +            return cmd;
>> +        }
>> +        return monitor_parse_command(mon, p, cmd->sub_table, qdict);
>> +    }
>> +
>>       /* parse the parameters */
>>       typestr = cmd->args_type;
>>       for(;;) {
> 
> 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.  If we care, we can try to do better in a follow-up patch.
> 
  Yes it is checked to avoid getting NULL for case "info ". I'll split
up a patch for the check code.

> We'll get sub-optimal error messages when the infrastructure is put to
> use in patch 4/4, e.g.:
> 
>      (qemu) info apple pie
>      unknown command: 'apple'
> 
> That's because cmdname is just "apple" at the place where the error is
> diagnosed.
> 
> Several other messages also print cmdname, and thus are similarly
> affected.  Many of them could probably just omit printing it, but that's
> a separate issue.
> 
> A possible way to improve this: additional parameter int start, parse
> starts at cmdline + start, and instead of printing cmdname, print
> cmdline up to end of cmdname.  Something like
> 
>          monitor_printf(mon, "unknown command: '%.*s'\n",
>                         (int)(p - cmdline), cmdline);
> 
  Thanks, I think this could work.


>> @@ -3925,7 +3940,7 @@ static void handle_user_command(Monitor *mon, const char *cmdline)
>>   
>>       qdict = qdict_new();
>>   
>> -    cmd = monitor_parse_command(mon, cmdline, qdict);
>> +    cmd = monitor_parse_command(mon, cmdline, mon_cmds, qdict);
>>       if (!cmd)
>>           goto out;
>

于 2013-1-10 18:23, Markus Armbruster 写道:
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> 
>>    This patch make parsing of hmp command aware of that it may
>> have sub command. Also discard simple encapsulation function
>> monitor_find_command() and qmp_find_cmd().
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   monitor.c |   31 +++++++++++++++++++++++--------
>>   1 files changed, 23 insertions(+), 8 deletions(-)
>>
>> diff --git a/monitor.c b/monitor.c
>> index c7b3014..d49a362 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -130,6 +130,7 @@ typedef struct mon_cmd_t {
>>                             MonitorCompletion *cb, void *opaque);
>>       } mhandler;
>>       int flags;
>> +    struct mon_cmd_t *sub_table;
>>   } mon_cmd_t;
>>   
>>   /* file descriptors passed via SCM_RIGHTS */
> 
> Forgot to mention: please explain in a comment how mhandler and
> sub_table interact.  Namely, if there are arguments, we use sub_table,
> else mhandler.
> 
OK.

于 2013-1-10 21:12, Luiz Capitulino 写道:
> On Thu, 10 Jan 2013 16:32:32 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>>    This patch make parsing of hmp command aware of that it may
>> have sub command. Also discard simple encapsulation function
>> monitor_find_command() and qmp_find_cmd().
>
> You don't qmp_find_cmd() anymore..
>
   Sorry I have forgot change that, will mention it in next version.