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

login
register
mail settings
Submitter Wayne Xia
Date Dec. 27, 2012, 9:40 a.m.
Message ID <1356601227-8707-3-git-send-email-xiawenc@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/208288/
State New
Headers show

Comments

Wayne Xia - Dec. 27, 2012, 9:40 a.m.
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 |   44 +++++++++++++++++++++++++++-----------------
 1 files changed, 27 insertions(+), 17 deletions(-)
Luiz Capitulino - Jan. 9, 2013, 12:57 p.m.
On Thu, 27 Dec 2012 17:40:25 +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().
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  monitor.c |   44 +++++++++++++++++++++++++++-----------------
>  1 files changed, 27 insertions(+), 17 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index c7b3014..e4d4c59 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,17 @@ 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);
> -}

Touching qmp_find_cmd() is unrelated to this series.

> -
> -static const mon_cmd_t *monitor_parse_command(Monitor *mon,
> -                                              const char *cmdline,
> -                                              QDict *qdict)
> +/*
> + * 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 remain string, and if not
> + * found it return NULL.
> + */
> +static const mon_cmd_t *hmp_parse_command(Monitor *mon,
> +                                          const char *cmdline,
> +                                          mon_cmd_t *table,
> +                                          QDict *qdict)

Renaming is also not necessary, but if you do it please do in a different
patch.

>  {
> -    const char *p, *typestr;
> +    const char *p, *p1, *typestr;
>      int c;
>      const mon_cmd_t *cmd;
>      char cmdname[256];
> @@ -3564,12 +3561,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 hmp_parse_command(mon, p, cmd->sub_table, qdict);

This works, and seems less hack then the previous attempt.

Except for a small detail. This:

  (qemu) info bla

Currently returns a list of info commands, but with this series it returns
an error.

I'd like to get a reviewed-by from Markus before applying this as, besides
this being his idea, I honestly have a very hard time knowing how to move old
hmp hackery forward.

> +    }
> +
>      /* parse the parameters */
>      typestr = cmd->args_type;
>      for(;;) {
> @@ -3925,7 +3935,7 @@ static void handle_user_command(Monitor *mon, const char *cmdline)
>  
>      qdict = qdict_new();
>  
> -    cmd = monitor_parse_command(mon, cmdline, qdict);
> +    cmd = hmp_parse_command(mon, cmdline, mon_cmds, qdict);
>      if (!cmd)
>          goto out;
>  
> @@ -4483,7 +4493,7 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>          goto err_out;
>      }
>  
> -    cmd = qmp_find_cmd(cmd_name);
> +    cmd = search_dispatch_table(qmp_cmds, cmd_name);
>      if (!cmd) {
>          qerror_report(QERR_COMMAND_NOT_FOUND, cmd_name);
>          goto err_out;
Wayne Xia - Jan. 10, 2013, 3:30 a.m.
>> -static const mon_cmd_t *qmp_find_cmd(const char *cmdname)
>> -{
>> -    return search_dispatch_table(qmp_cmds, cmdname);
>> -}
>
> Touching qmp_find_cmd() is unrelated to this series.
>
   OK.

>> -
>> -static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>> -                                              const char *cmdline,
>> -                                              QDict *qdict)
>> +/*
>> + * 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 remain string, and if not
>> + * found it return NULL.
>> + */
>> +static const mon_cmd_t *hmp_parse_command(Monitor *mon,
>> +                                          const char *cmdline,
>> +                                          mon_cmd_t *table,
>> +                                          QDict *qdict)
>
> Renaming is also not necessary, but if you do it please do in a different
> patch.
>
   OK.

>>   {
>> +        }
>> +        return hmp_parse_command(mon, p, cmd->sub_table, qdict);
>
> This works, and seems less hack then the previous attempt.
>
> Except for a small detail. This:
>
>    (qemu) info bla
>
> Currently returns a list of info commands, but with this series it returns
> an error.
>
   Yep, I think command completion is another topic, but I am pretty
sure it can be solved in this framework, for that it is not hack and
what need to do is, let command completion function knows there may be
another sub layer too.

> I'd like to get a reviewed-by from Markus before applying this as, besides
> this being his idea, I honestly have a very hard time knowing how to move old
> hmp hackery forward.
>
>> +    }
>> +
>>       /* parse the parameters */
>>       typestr = cmd->args_type;
>>       for(;;) {
>> @@ -3925,7 +3935,7 @@ static void handle_user_command(Monitor *mon, const char *cmdline)
>>
>>       qdict = qdict_new();
>>
>> -    cmd = monitor_parse_command(mon, cmdline, qdict);
>> +    cmd = hmp_parse_command(mon, cmdline, mon_cmds, qdict);
>>       if (!cmd)
>>           goto out;
>>
>> @@ -4483,7 +4493,7 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>>           goto err_out;
>>       }
>>
>> -    cmd = qmp_find_cmd(cmd_name);
>> +    cmd = search_dispatch_table(qmp_cmds, cmd_name);
>>       if (!cmd) {
>>           qerror_report(QERR_COMMAND_NOT_FOUND, cmd_name);
>>           goto err_out;
>
>
wenchao xia - Jan. 10, 2013, 7:07 a.m.
于 2013-1-10 11:30, Wenchao Xia 写道:
>>> -static const mon_cmd_t *qmp_find_cmd(const char *cmdname)
>>> -{
>>> -    return search_dispatch_table(qmp_cmds, cmdname);
>>> -}
>>
>> Touching qmp_find_cmd() is unrelated to this series.
>>
>    OK.
>
>>> -
>>> -static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>>> -                                              const char *cmdline,
>>> -                                              QDict *qdict)
>>> +/*
>>> + * 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 remain string, and
>>> if not
>>> + * found it return NULL.
>>> + */
>>> +static const mon_cmd_t *hmp_parse_command(Monitor *mon,
>>> +                                          const char *cmdline,
>>> +                                          mon_cmd_t *table,
>>> +                                          QDict *qdict)
>>
>> Renaming is also not necessary, but if you do it please do in a different
>> patch.
>>
>    OK.
>
>>>   {
>>> +        }
>>> +        return hmp_parse_command(mon, p, cmd->sub_table, qdict);
>>
>> This works, and seems less hack then the previous attempt.
>>
>> Except for a small detail. This:
>>
>>    (qemu) info bla
>>
>> Currently returns a list of info commands, but with this series it
>> returns
>> an error.
>>
>    Yep, I think command completion is another topic, but I am pretty
> sure it can be solved in this framework, for that it is not hack and
> what need to do is, let command completion function knows there may be
> another sub layer too.
>
   Sorry I missed your point. Returning an error instead of info
commands, is made on purpose. IMHO always print the commands
make user confused, message tipping what is wrong is better.

>> I'd like to get a reviewed-by from Markus before applying this as,
>> besides
>> this being his idea, I honestly have a very hard time knowing how to
>> move old
>> hmp hackery forward.
>>
Markus Armbruster - Jan. 10, 2013, 9:23 a.m.
wenchao xia <xiaxia347os@163.com> writes:

> 于 2013-1-10 11:30, Wenchao Xia 写道:
>>>> -static const mon_cmd_t *qmp_find_cmd(const char *cmdname)
>>>> -{
>>>> -    return search_dispatch_table(qmp_cmds, cmdname);
>>>> -}
>>>
>>> Touching qmp_find_cmd() is unrelated to this series.
>>>
>>    OK.
>>
>>>> -
>>>> -static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>>>> -                                              const char *cmdline,
>>>> -                                              QDict *qdict)
>>>> +/*
>>>> + * 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 remain string, and
>>>> if not
>>>> + * found it return NULL.
>>>> + */
>>>> +static const mon_cmd_t *hmp_parse_command(Monitor *mon,
>>>> +                                          const char *cmdline,
>>>> +                                          mon_cmd_t *table,
>>>> +                                          QDict *qdict)
>>>
>>> Renaming is also not necessary, but if you do it please do in a different
>>> patch.
>>>
>>    OK.
>>
>>>>   {
>>>> +        }
>>>> +        return hmp_parse_command(mon, p, cmd->sub_table, qdict);
>>>
>>> This works, and seems less hack then the previous attempt.
>>>
>>> Except for a small detail. This:
>>>
>>>    (qemu) info bla
>>>
>>> Currently returns a list of info commands, but with this series it
>>> returns
>>> an error.
>>>
>>    Yep, I think command completion is another topic, but I am pretty
>> sure it can be solved in this framework, for that it is not hack and
>> what need to do is, let command completion function knows there may be
>> another sub layer too.
>>
>   Sorry I missed your point. Returning an error instead of info
> commands, is made on purpose. IMHO always print the commands
> make user confused, message tipping what is wrong is better.

I agree with the change, but it needs to be mentioned in the commit
message.  Separate patch would be even better.

>>> I'd like to get a reviewed-by from Markus before applying this as,
>>> besides
>>> this being his idea, I honestly have a very hard time knowing how to
>>> move old
>>> hmp hackery forward.

I'll review v4.

Patch

diff --git a/monitor.c b/monitor.c
index c7b3014..e4d4c59 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,17 @@  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);
-}
-
-static const mon_cmd_t *monitor_parse_command(Monitor *mon,
-                                              const char *cmdline,
-                                              QDict *qdict)
+/*
+ * 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 remain string, and if not
+ * found it return NULL.
+ */
+static const mon_cmd_t *hmp_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 +3561,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 hmp_parse_command(mon, p, cmd->sub_table, qdict);
+    }
+
     /* parse the parameters */
     typestr = cmd->args_type;
     for(;;) {
@@ -3925,7 +3935,7 @@  static void handle_user_command(Monitor *mon, const char *cmdline)
 
     qdict = qdict_new();
 
-    cmd = monitor_parse_command(mon, cmdline, qdict);
+    cmd = hmp_parse_command(mon, cmdline, mon_cmds, qdict);
     if (!cmd)
         goto out;
 
@@ -4483,7 +4493,7 @@  static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
         goto err_out;
     }
 
-    cmd = qmp_find_cmd(cmd_name);
+    cmd = search_dispatch_table(qmp_cmds, cmd_name);
     if (!cmd) {
         qerror_report(QERR_COMMAND_NOT_FOUND, cmd_name);
         goto err_out;