Re: [Qemu-devel] [PATCH V4 2/4] HMP: add infrastructure for sub command

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

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


-- 
Best Regards

Wenchao Xia