Markus Armbruster <arm...@redhat.com> writes:
> Bandan Das <b...@redhat.com> writes:
>
>> There's too much going on in monitor_parse_command().
>> Split up the arguments parsing bits into a separate function
>> monitor_parse_arguments(). Let the original function check for
>> command validity and sub-commands if any and return data (*cmd)
>> that the newly introduced function can process and return a
>> QDict. Also, pass a pointer to the cmdline to track current
>> parser location.
>>
>> Suggested-by: Markus Armbruster <arm...@redhat.com>
>> Signed-off-by: Bandan Das <b...@redhat.com>
>> ---
>> monitor.c | 90
>> +++++++++++++++++++++++++++++++++++++--------------------------
>> 1 file changed, 53 insertions(+), 37 deletions(-)
>>
>> diff --git a/monitor.c b/monitor.c
>> index b2561e1..521258d 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -3683,11 +3683,10 @@ static const mon_cmd_t *qmp_find_cmd(const char
>> *cmdname)
>> }
>>
>> /*
>> - * 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.
>> - * If a sub-command table exists, and if @cmdline contains an additional
>> string
>> + * Parse cmdline anchored at @endp according to command table @table.
>> + * If @*endp is blank, return NULL.
>> + * If @*endp is invalid, report to @mon and return NULL.
>> + * If a sub-command table exists, and if @*endp contains an additional
>> string
>
> @endp is a rather odd name now, don't you think? What about naming it
> @cmdp?
>
> Preexisting: comment suggests we have at most two levels of command
> tables. Code actually supports arbitrary nesting.
>
> What about:
>
> /*
> * Parse command name from @cmdp according to command table @table.
> * If blank, return NULL.
> * Else, if no valid command can be found, report to @mon, and return
> * NULL.
> * Else, change @cmdp to point right behind the name, and return its
> * command table entry.
> * Do not assume the return value points into @table! It doesn't when
> * the command is found in a sub-command table.
> */
>
> No longer explains how sub-commands work. Proper, because it's none of
> the callers business, and this is a function contract. The explanation
> belongs into mon_cmd_t. Which already has one that's good enough.
>
> If the function's code dealing with sub-commands was unobvious, we'd
> want to explain it some there. But it isn't. We explain a bit there
> anyway, which is fine with me.
Ok sounds good.
>> * for a sub-command, this function will try to search the sub-command
>> table.
>> * If no additional string for a sub-command is present, this function will
>> * return the command found in @table.
>> @@ -3695,31 +3694,26 @@ static const mon_cmd_t *qmp_find_cmd(const char
>> *cmdname)
>> * when the command is a sub-command.
>> */
>> static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>> - const char *cmdline,
>> - int start,
>> - mon_cmd_t *table,
>> - QDict *qdict)
>> + const char **endp,
>> + mon_cmd_t *table)
>> {
>> - const char *p, *typestr;
>> - int c;
>> + const char *p;
>> const mon_cmd_t *cmd;
>> char cmdname[256];
>> - char buf[1024];
>> - char *key;
>>
>> #ifdef DEBUG
>> - monitor_printf(mon, "command='%s', start='%d'\n", cmdline, start);
>> + monitor_printf(mon, "command='%s', start='%d'\n", cmdline, *start);
>
> Would this compile if we defined DEBUG?
No, it won't :) Sorry, will fix.
>> #endif
>>
>> /* extract the command name */
>> - p = get_command_name(cmdline + start, cmdname, sizeof(cmdname));
>> + p = get_command_name(*endp, cmdname, sizeof(cmdname));
>> if (!p)
>> return NULL;
>>
>> cmd = search_dispatch_table(table, cmdname);
>> if (!cmd) {
>> monitor_printf(mon, "unknown command: '%.*s'\n",
>> - (int)(p - cmdline), cmdline);
>> + (int)(p - *endp), *endp);
>> return NULL;
>> }
>>
>> @@ -3727,16 +3721,33 @@ static const mon_cmd_t
>> *monitor_parse_command(Monitor *mon,
>> while (qemu_isspace(*p)) {
>> p++;
>> }
>> +
>> + *endp = p;
>> /* search sub command */
>> - if (cmd->sub_table != NULL) {
>> - /* check if user set additional command */
>> - if (*p == '\0') {
>> - return cmd;
>> - }
>> - return monitor_parse_command(mon, cmdline, p - cmdline,
>> - cmd->sub_table, qdict);
>> + if (cmd->sub_table != NULL && *p != '\0') {
>> + return monitor_parse_command(mon, endp, cmd->sub_table);
>> }
>>
>> + return cmd;
>> +}
>> +
>> +/*
>> + * Parse arguments for @cmd anchored at @endp
>> + * If it can't be parsed, report to @mon, and return NULL.
>> + * Else, insert command arguments into @qdict, and return it.
>
> @qdict suggests there's a parameter with that name. Suggest "a QDict",
> or "a new QDict".
>
> Adding "Caller is responsible for freeing it" wouldn't hurt.
Yes, good idea. Will do.
>
>> + */
>> +
>> +static QDict *monitor_parse_arguments(Monitor *mon,
>> + const char **endp,
>> + const mon_cmd_t *cmd)
>> +{
>> + const char *typestr;
>> + char *key;
>> + int c;
>> + const char *p = *endp;
>> + char buf[1024];
>> + QDict *qdict = qdict_new();
>> +
>> /* parse the parameters */
>> typestr = cmd->args_type;
>> for(;;) {
>> @@ -3766,14 +3777,14 @@ static const mon_cmd_t
>> *monitor_parse_command(Monitor *mon,
>> switch(c) {
>> case 'F':
>> monitor_printf(mon, "%s: filename expected\n",
>> - cmdname);
>> + cmd->name);
>> break;
>> case 'B':
>> monitor_printf(mon, "%s: block device name
>> expected\n",
>> - cmdname);
>> + cmd->name);
>> break;
>> default:
>> - monitor_printf(mon, "%s: string expected\n",
>> cmdname);
>> + monitor_printf(mon, "%s: string expected\n",
>> cmd->name);
>> break;
>> }
>> goto fail;
>> @@ -3915,7 +3926,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor
>> *mon,
>> goto fail;
>> /* Check if 'i' is greater than 32-bit */
>> if ((c == 'i') && ((val >> 32) & 0xffffffff)) {
>> - monitor_printf(mon, "\'%s\' has failed: ", cmdname);
>> + monitor_printf(mon, "\'%s\' has failed: ", cmd->name);
>> monitor_printf(mon, "integer is for 32-bit values\n");
>> goto fail;
>> } else if (c == 'M') {
>> @@ -4023,7 +4034,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor
>> *mon,
>> if(!is_valid_option(p, typestr)) {
>>
>> monitor_printf(mon, "%s: unsupported option
>> -%c\n",
>> - cmdname, *p);
>> + cmd->name, *p);
>> goto fail;
>> } else {
>> skip_key = 1;
>> @@ -4057,7 +4068,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor
>> *mon,
> if (*typestr == '?') {
> typestr++;
> if (*p == '\0') {
> /* no remaining string: NULL argument */
> break;
> }
> }
>> len = strlen(p);
>> if (len <= 0) {
>> monitor_printf(mon, "%s: string expected\n",
>> - cmdname);
>> + cmd->name);
>> break;
>
> Preexisting: this error fails to fail the function.
>
> Bug currently can't bite, because argument type 'S' is only used
> together with '?', and then we don't reach this error.
>
> Mind to throw in the obvious fix? Separate patch, of course.
Ok sure!
>> }
....
>> -out:
>> + /* Drop reference taken in monitor_parse_arguments */
>> QDECREF(qdict);
>> }
>
> Very close to earning my R-by.
Yay!!
Thanks again for the meticulous review,
Bandan
