| Message ID | 1432768754-28523-2-git-send-email-bsd@redhat.com |
|---|---|
| State | New |
| Headers | show |
Bandan Das <bsd@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 <armbru@redhat.com> > Signed-off-by: Bandan Das <bsd@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. > * 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? > #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. > + */ > + > +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. > } > qdict_put(qdict, key, qstring_from_str(p)); > @@ -4066,7 +4077,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon, > break; > default: > bad_type: > - monitor_printf(mon, "%s: unknown type '%c'\n", cmdname, c); > + monitor_printf(mon, "%s: unknown type '%c'\n", cmd->name, c); > goto fail; > } > g_free(key); > @@ -4077,13 +4088,14 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon, > p++; > if (*p != '\0') { > monitor_printf(mon, "%s: extraneous characters at the end of line\n", > - cmdname); > + cmd->name); > goto fail; > } > > - return cmd; > + return qdict; > > fail: > + QDECREF(qdict); > g_free(key); > return NULL; > } > @@ -4115,11 +4127,15 @@ static void handle_user_command(Monitor *mon, const char *cmdline) > QDict *qdict; > const mon_cmd_t *cmd; > > - qdict = qdict_new(); > + cmd = monitor_parse_command(mon, &cmdline, mon->cmd_table); > + if (!cmd) { > + return; > + } > > - cmd = monitor_parse_command(mon, cmdline, 0, mon->cmd_table, qdict); > - if (!cmd) > - goto out; > + qdict = monitor_parse_arguments(mon, &cmdline, cmd); > + if (!qdict) { > + return; > + } > > if (handler_is_async(cmd)) { > user_async_cmd_handler(mon, cmd, qdict); > @@ -4137,7 +4153,7 @@ static void handle_user_command(Monitor *mon, const char *cmdline) > cmd->mhandler.cmd(mon, qdict); > } > > -out: > + /* Drop reference taken in monitor_parse_arguments */ > QDECREF(qdict); > } Very close to earning my R-by.
Markus Armbruster <armbru@redhat.com> writes: > Bandan Das <bsd@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 <armbru@redhat.com> >> Signed-off-by: Bandan Das <bsd@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
On 05/28/2015 12:48 PM, Bandan Das wrote: > Markus Armbruster <armbru@redhat.com> writes: > >> Bandan Das <bsd@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. >>> >>> #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. That's why I like solutions that can't bitrot; something like this framework (needs a bit more to actually compile, but you get the picture): #ifdef DEBUG # define DEBUG_MONITOR 1 #else # define DEBUG_MONITOR 0 #endif #define DEBUG_MONITOR_PRINTF(stuff...) do { \ if (DEBUG_MONITOR) { \ monitor_printf(stuff...); \ } \ } while (0) then you can avoid the #ifdef in the function body, and just do DEBUG_MONITOR_PRINTF(mon, "command='%s'....) and the compiler will always check for correct format vs. arguments, even when debugging is off. Of course, adding such a framework in this file would be a separate patch, and does not have to be done as a prerequisite of this series.
Eric Blake <eblake@redhat.com> writes: > On 05/28/2015 12:48 PM, Bandan Das wrote: >> Markus Armbruster <armbru@redhat.com> writes: >> >>> Bandan Das <bsd@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. > >>>> >>>> #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. > > That's why I like solutions that can't bitrot; something like this > framework (needs a bit more to actually compile, but you get the picture): > > #ifdef DEBUG > # define DEBUG_MONITOR 1 > #else > # define DEBUG_MONITOR 0 > #endif > #define DEBUG_MONITOR_PRINTF(stuff...) do { \ > if (DEBUG_MONITOR) { \ > monitor_printf(stuff...); \ > } \ > } while (0) > > then you can avoid the #ifdef in the function body, and just do > DEBUG_MONITOR_PRINTF(mon, "command='%s'....) and the compiler will > always check for correct format vs. arguments, even when debugging is off. > > Of course, adding such a framework in this file would be a separate > patch, and does not have to be done as a prerequisite of this series. Thanks, Eric. Ok, I will post patch(es) separately.
Bandan Das <bsd@redhat.com> writes: > Eric Blake <eblake@redhat.com> writes: > >> On 05/28/2015 12:48 PM, Bandan Das wrote: >>> Markus Armbruster <armbru@redhat.com> writes: >>> >>>> Bandan Das <bsd@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. >> >>>>> >>>>> #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. >> >> That's why I like solutions that can't bitrot; something like this >> framework (needs a bit more to actually compile, but you get the picture): >> >> #ifdef DEBUG >> # define DEBUG_MONITOR 1 >> #else >> # define DEBUG_MONITOR 0 >> #endif >> #define DEBUG_MONITOR_PRINTF(stuff...) do { \ >> if (DEBUG_MONITOR) { \ >> monitor_printf(stuff...); \ >> } \ >> } while (0) >> >> then you can avoid the #ifdef in the function body, and just do >> DEBUG_MONITOR_PRINTF(mon, "command='%s'....) and the compiler will >> always check for correct format vs. arguments, even when debugging is off. >> >> Of course, adding such a framework in this file would be a separate >> patch, and does not have to be done as a prerequisite of this series. > > Thanks, Eric. Ok, I will post patch(es) separately. The preferred solution in QEMU is tracepoints. In this case, I wouldn't mind ditching the debugging prints instead. All three of them.
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 * 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); #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. + */ + +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, len = strlen(p); if (len <= 0) { monitor_printf(mon, "%s: string expected\n", - cmdname); + cmd->name); break; } qdict_put(qdict, key, qstring_from_str(p)); @@ -4066,7 +4077,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon, break; default: bad_type: - monitor_printf(mon, "%s: unknown type '%c'\n", cmdname, c); + monitor_printf(mon, "%s: unknown type '%c'\n", cmd->name, c); goto fail; } g_free(key); @@ -4077,13 +4088,14 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon, p++; if (*p != '\0') { monitor_printf(mon, "%s: extraneous characters at the end of line\n", - cmdname); + cmd->name); goto fail; } - return cmd; + return qdict; fail: + QDECREF(qdict); g_free(key); return NULL; } @@ -4115,11 +4127,15 @@ static void handle_user_command(Monitor *mon, const char *cmdline) QDict *qdict; const mon_cmd_t *cmd; - qdict = qdict_new(); + cmd = monitor_parse_command(mon, &cmdline, mon->cmd_table); + if (!cmd) { + return; + } - cmd = monitor_parse_command(mon, cmdline, 0, mon->cmd_table, qdict); - if (!cmd) - goto out; + qdict = monitor_parse_arguments(mon, &cmdline, cmd); + if (!qdict) { + return; + } if (handler_is_async(cmd)) { user_async_cmd_handler(mon, cmd, qdict); @@ -4137,7 +4153,7 @@ static void handle_user_command(Monitor *mon, const char *cmdline) cmd->mhandler.cmd(mon, qdict); } -out: + /* Drop reference taken in monitor_parse_arguments */ QDECREF(qdict); }
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 <armbru@redhat.com> Signed-off-by: Bandan Das <bsd@redhat.com> --- monitor.c | 90 +++++++++++++++++++++++++++++++++++++-------------------------- 1 file changed, 53 insertions(+), 37 deletions(-)