* [Qemu-devel] [PATCH v2 0/2] monitor: suggest running "help" for command errors @ 2015-05-26 23:45 Bandan Das 2015-05-26 23:45 ` [Qemu-devel] [PATCH v2 1/2] monitor: cleanup parsing of cmd name and cmd arguments Bandan Das 2015-05-26 23:45 ` [Qemu-devel] [PATCH v2 2/2] monitor: suggest running "help" for command errors Bandan Das 0 siblings, 2 replies; 5+ messages in thread From: Bandan Das @ 2015-05-26 23:45 UTC (permalink / raw) To: qemu-devel; +Cc: armbru v2: Split up the command name and arguments parsing into separate functions. [1/2] Skip checking for failures with commands that use the .cmd_new interface or the async interface since they are scheduled for removal [2/2] Bandan Das (2): monitor: cleanup parsing of cmd name and cmd arguments monitor: suggest running "help" for command errors monitor.c | 86 +++++++++++++++++++++++++++++++++++++++------------------------ 1 file changed, 53 insertions(+), 33 deletions(-) -- 2.1.0 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [Qemu-devel] [PATCH v2 1/2] monitor: cleanup parsing of cmd name and cmd arguments 2015-05-26 23:45 [Qemu-devel] [PATCH v2 0/2] monitor: suggest running "help" for command errors Bandan Das @ 2015-05-26 23:45 ` Bandan Das 2015-05-27 8:10 ` Markus Armbruster 2015-05-26 23:45 ` [Qemu-devel] [PATCH v2 2/2] monitor: suggest running "help" for command errors Bandan Das 1 sibling, 1 reply; 5+ messages in thread From: Bandan Das @ 2015-05-26 23:45 UTC (permalink / raw) To: qemu-devel; +Cc: armbru 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. Suggested-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Bandan Das <bsd@redhat.com> --- monitor.c | 84 ++++++++++++++++++++++++++++++++++++++------------------------- 1 file changed, 51 insertions(+), 33 deletions(-) diff --git a/monitor.c b/monitor.c index b2561e1..f0c1a8c 100644 --- a/monitor.c +++ b/monitor.c @@ -3685,8 +3685,7 @@ 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 @cmdline is invalid, report to @mon and return NULL. * If a sub-command table exists, and if @cmdline 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 @@ -3696,23 +3695,19 @@ static const mon_cmd_t *qmp_find_cmd(const char *cmdname) */ static const mon_cmd_t *monitor_parse_command(Monitor *mon, const char *cmdline, - int start, - mon_cmd_t *table, - QDict *qdict) + int *start, + 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(cmdline + *start, cmdname, sizeof(cmdname)); if (!p) return NULL; @@ -3727,16 +3722,35 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon, while (qemu_isspace(*p)) { p++; } + + *start = p - cmdline; /* 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, cmdline, start, + cmd->sub_table); } + return cmd; +} + +/* + * Parse arguments for @cmd + * 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 *cmdline, + int *start, + const mon_cmd_t *cmd) +{ + const char *typestr; + char *key; + int c; + const char *p = cmdline + *start; + char buf[1024]; + QDict *qdict = qdict_new(); + /* parse the parameters */ typestr = cmd->args_type; for(;;) { @@ -3766,14 +3780,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 +3929,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 +4037,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 +4071,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 +4080,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 +4091,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; } @@ -4114,12 +4129,17 @@ static void handle_user_command(Monitor *mon, const char *cmdline) { QDict *qdict; const mon_cmd_t *cmd; + int start = 0; - qdict = qdict_new(); + cmd = monitor_parse_command(mon, cmdline, &start, 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, &start, cmd); + if (!qdict) { + return; + } if (handler_is_async(cmd)) { user_async_cmd_handler(mon, cmd, qdict); @@ -4137,8 +4157,6 @@ static void handle_user_command(Monitor *mon, const char *cmdline) cmd->mhandler.cmd(mon, qdict); } -out: - QDECREF(qdict); } static void cmd_completion(Monitor *mon, const char *name, const char *list) -- 2.1.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] monitor: cleanup parsing of cmd name and cmd arguments 2015-05-26 23:45 ` [Qemu-devel] [PATCH v2 1/2] monitor: cleanup parsing of cmd name and cmd arguments Bandan Das @ 2015-05-27 8:10 ` Markus Armbruster 0 siblings, 0 replies; 5+ messages in thread From: Markus Armbruster @ 2015-05-27 8:10 UTC (permalink / raw) To: Bandan Das; +Cc: qemu-devel 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. > > Suggested-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Bandan Das <bsd@redhat.com> > --- > monitor.c | 84 ++++++++++++++++++++++++++++++++++++++------------------------- > 1 file changed, 51 insertions(+), 33 deletions(-) > > diff --git a/monitor.c b/monitor.c > index b2561e1..f0c1a8c 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -3685,8 +3685,7 @@ 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 @cmdline is invalid, report to @mon and return NULL. > * If a sub-command table exists, and if @cmdline 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 Function comment fails to document @start. Since you're touching it, you get to fix that :) I figure this would become a bit simpler if we replaced in/out parameter int start by an out parameter const char **endp. > @@ -3696,23 +3695,19 @@ static const mon_cmd_t *qmp_find_cmd(const char *cmdname) > */ > static const mon_cmd_t *monitor_parse_command(Monitor *mon, > const char *cmdline, > - int start, > - mon_cmd_t *table, > - QDict *qdict) > + int *start, > + 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(cmdline + *start, cmdname, sizeof(cmdname)); > if (!p) > return NULL; > > @@ -3727,16 +3722,35 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon, > while (qemu_isspace(*p)) { > p++; > } > + > + *start = p - cmdline; > /* 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, cmdline, start, > + cmd->sub_table); > } > > + return cmd; > +} > + > +/* > + * Parse arguments for @cmd > + * If it can't be parsed, report to @mon, and return NULL. > + * Else, insert command arguments into @qdict, and return it. > + */ Please document @cmdline and @start. > + > +static QDict *monitor_parse_arguments(Monitor *mon, > + const char *cmdline, > + int *start, > + const mon_cmd_t *cmd) > +{ > + const char *typestr; > + char *key; > + int c; > + const char *p = cmdline + *start; > + char buf[1024]; > + QDict *qdict = qdict_new(); > + > /* parse the parameters */ > typestr = cmd->args_type; > for(;;) { > @@ -3766,14 +3780,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 +3929,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 +4037,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 +4071,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 +4080,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 +4091,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; > } > @@ -4114,12 +4129,17 @@ static void handle_user_command(Monitor *mon, const char *cmdline) This hunk conflicts with my "[PATCH v2 00/20] monitor: Wean core off QError, and other cleanups". Shouldn't be hard to resolve. > { > QDict *qdict; > const mon_cmd_t *cmd; > + int start = 0; > > - qdict = qdict_new(); > + cmd = monitor_parse_command(mon, cmdline, &start, 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, &start, cmd); Since we're not interested in text following arguments (if that's even possible), we could simply do qdict = monitor_parse_arguments(mon, cmdline + start, cmd); If we replace in/out start by out endp as mentioned above, things become simpler still: cmd = monitor_parse_command(mon, cmdline, &endp, mon->cmd_table); if (!cmd) { return; } qdict = monitor_parse_arguments(mon, endp, cmd); > + if (!qdict) { > + return; > + } > > if (handler_is_async(cmd)) { > user_async_cmd_handler(mon, cmd, qdict); > @@ -4137,8 +4157,6 @@ static void handle_user_command(Monitor *mon, const char *cmdline) > cmd->mhandler.cmd(mon, qdict); > } > > -out: > - QDECREF(qdict); Doesn't this leak qdict? > } > > static void cmd_completion(Monitor *mon, const char *name, const char *list) Overall, I rather like how this turned out. ^ permalink raw reply [flat|nested] 5+ messages in thread
* [Qemu-devel] [PATCH v2 2/2] monitor: suggest running "help" for command errors 2015-05-26 23:45 [Qemu-devel] [PATCH v2 0/2] monitor: suggest running "help" for command errors Bandan Das 2015-05-26 23:45 ` [Qemu-devel] [PATCH v2 1/2] monitor: cleanup parsing of cmd name and cmd arguments Bandan Das @ 2015-05-26 23:45 ` Bandan Das 2015-05-27 8:11 ` Markus Armbruster 1 sibling, 1 reply; 5+ messages in thread From: Bandan Das @ 2015-05-26 23:45 UTC (permalink / raw) To: qemu-devel; +Cc: armbru When a command fails due to incorrect syntax or input, suggest using the "help" command to get more information about the command. This is only applicable for HMP. Before: (qemu) drive_add usb_flash_drive drive_add: string expected After: (qemu) drive_add usb_flash_drive drive_add: string expected Try "help drive_add" for more information Signed-off-by: Bandan Das <bsd@redhat.com> --- monitor.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/monitor.c b/monitor.c index f0c1a8c..af73195 100644 --- a/monitor.c +++ b/monitor.c @@ -4138,6 +4138,8 @@ static void handle_user_command(Monitor *mon, const char *cmdline) qdict = monitor_parse_arguments(mon, cmdline, &start, cmd); if (!qdict) { + monitor_printf(mon, "Try \"help %s\" for more information\n", + cmd->name); return; } -- 2.1.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] monitor: suggest running "help" for command errors 2015-05-26 23:45 ` [Qemu-devel] [PATCH v2 2/2] monitor: suggest running "help" for command errors Bandan Das @ 2015-05-27 8:11 ` Markus Armbruster 0 siblings, 0 replies; 5+ messages in thread From: Markus Armbruster @ 2015-05-27 8:11 UTC (permalink / raw) To: Bandan Das; +Cc: qemu-devel Bandan Das <bsd@redhat.com> writes: > When a command fails due to incorrect syntax or input, > suggest using the "help" command to get more information > about the command. This is only applicable for HMP. > > Before: > (qemu) drive_add usb_flash_drive > drive_add: string expected > After: > (qemu) drive_add usb_flash_drive > drive_add: string expected > Try "help drive_add" for more information Reviewed-by: Markus Armbruster <armbru@redhat.com> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-05-27 8:11 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-05-26 23:45 [Qemu-devel] [PATCH v2 0/2] monitor: suggest running "help" for command errors Bandan Das 2015-05-26 23:45 ` [Qemu-devel] [PATCH v2 1/2] monitor: cleanup parsing of cmd name and cmd arguments Bandan Das 2015-05-27 8:10 ` Markus Armbruster 2015-05-26 23:45 ` [Qemu-devel] [PATCH v2 2/2] monitor: suggest running "help" for command errors Bandan Das 2015-05-27 8:11 ` Markus Armbruster
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).