* [Qemu-devel] [PATCH v5 0/4] monitor: suggest running "help" for command errors @ 2015-06-03 22:38 Bandan Das 2015-06-03 22:38 ` [Qemu-devel] [PATCH v5 1/4] monitor: remove debug prints Bandan Das ` (5 more replies) 0 siblings, 6 replies; 10+ messages in thread From: Bandan Das @ 2015-06-03 22:38 UTC (permalink / raw) To: qemu-devel; +Cc: armbru v5: Move "monitor: remove debug prints" to first in the series Minor fixes to comments and commit messages v4: Better name for cmdline index pointer [1/4] Change comment for monitor_parse_command as suggested in review [1/4] Fix potential compilation failure in debug print [1/4] New - Fix failure path for argument type "S" [3/4] New - Remove debug prints [4/4] v3: Track the current location directly in the command line [1/2] Fix potential qdict leak [1/2] Document char **endp [1/2] Rebase on top of changes and add reviewed-by [2/2] 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 (4): monitor: remove debug prints monitor: cleanup parsing of cmd name and cmd arguments monitor: Point to "help" command on syntax error monitor: Fix failure path for "S" argument monitor.c | 122 +++++++++++++++++++++++++++++++------------------------------- 1 file changed, 61 insertions(+), 61 deletions(-) -- 2.1.0 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH v5 1/4] monitor: remove debug prints 2015-06-03 22:38 [Qemu-devel] [PATCH v5 0/4] monitor: suggest running "help" for command errors Bandan Das @ 2015-06-03 22:38 ` Bandan Das 2015-06-03 22:38 ` [Qemu-devel] [PATCH v5 2/4] monitor: cleanup parsing of cmd name and cmd arguments Bandan Das ` (4 subsequent siblings) 5 siblings, 0 replies; 10+ messages in thread From: Bandan Das @ 2015-06-03 22:38 UTC (permalink / raw) To: qemu-devel; +Cc: armbru The preferred solution is to use tracepoints and there is good chance of bitrot with the debug prints not being enabled at compile time. Remove them. Suggested-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Bandan Das <bsd@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com> --- monitor.c | 20 +------------------- 1 file changed, 1 insertion(+), 19 deletions(-) diff --git a/monitor.c b/monitor.c index b2561e1..cc4e7d1 100644 --- a/monitor.c +++ b/monitor.c @@ -81,9 +81,6 @@ #endif #include "hw/lm32/lm32_pic.h" -//#define DEBUG -//#define DEBUG_COMPLETION - /* * Supported types: * @@ -3707,10 +3704,6 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon, char buf[1024]; char *key; -#ifdef DEBUG - monitor_printf(mon, "command='%s', start='%d'\n", cmdline, start); -#endif - /* extract the command name */ p = get_command_name(cmdline + start, cmdname, sizeof(cmdname)); if (!p) @@ -4189,10 +4182,7 @@ static void file_completion(Monitor *mon, const char *input) path[input_path_len] = '\0'; pstrcpy(file_prefix, sizeof(file_prefix), p + 1); } -#ifdef DEBUG_COMPLETION - monitor_printf(mon, "input='%s' path='%s' prefix='%s'\n", - input, path, file_prefix); -#endif + ffs = opendir(path); if (!ffs) return; @@ -4770,14 +4760,6 @@ static void monitor_find_completion(void *opaque, if (parse_cmdline(cmdline, &nb_args, args) < 0) { return; } -#ifdef DEBUG_COMPLETION - { - int i; - for (i = 0; i < nb_args; i++) { - monitor_printf(mon, "arg%d = '%s'\n", i, args[i]); - } - } -#endif /* if the line ends with a space, it means we want to complete the next arg */ -- 2.1.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH v5 2/4] monitor: cleanup parsing of cmd name and cmd arguments 2015-06-03 22:38 [Qemu-devel] [PATCH v5 0/4] monitor: suggest running "help" for command errors Bandan Das 2015-06-03 22:38 ` [Qemu-devel] [PATCH v5 1/4] monitor: remove debug prints Bandan Das @ 2015-06-03 22:38 ` Bandan Das 2015-06-12 6:05 ` Markus Armbruster 2015-06-03 22:38 ` [Qemu-devel] [PATCH v5 3/4] monitor: Point to "help" command on syntax error Bandan Das ` (3 subsequent siblings) 5 siblings, 1 reply; 10+ messages in thread From: Bandan Das @ 2015-06-03 22:38 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. 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 | 98 +++++++++++++++++++++++++++++++++++++-------------------------- 1 file changed, 57 insertions(+), 41 deletions(-) diff --git a/monitor.c b/monitor.c index cc4e7d1..33d088e 100644 --- a/monitor.c +++ b/monitor.c @@ -3680,39 +3680,32 @@ 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 - * 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. - * Do not assume the returned command points into @table! It doesn't - * when the command is a sub-command. + * 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. */ static const mon_cmd_t *monitor_parse_command(Monitor *mon, - const char *cmdline, - int start, - mon_cmd_t *table, - QDict *qdict) + const char **cmdp, + 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; /* extract the command name */ - p = get_command_name(cmdline + start, cmdname, sizeof(cmdname)); + p = get_command_name(*cmdp, 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 - *cmdp), *cmdp); return NULL; } @@ -3720,16 +3713,34 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon, while (qemu_isspace(*p)) { p++; } + + *cmdp = 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, cmdp, 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 a QDict, and return it. + * Note: On success, caller has to free the QDict structure. + */ + +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(;;) { @@ -3759,14 +3770,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; @@ -3908,7 +3919,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') { @@ -4016,7 +4027,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; @@ -4050,7 +4061,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)); @@ -4059,7 +4070,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); @@ -4070,13 +4081,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; } @@ -4108,11 +4120,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); @@ -4130,7 +4146,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); } -- 2.1.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v5 2/4] monitor: cleanup parsing of cmd name and cmd arguments 2015-06-03 22:38 ` [Qemu-devel] [PATCH v5 2/4] monitor: cleanup parsing of cmd name and cmd arguments Bandan Das @ 2015-06-12 6:05 ` Markus Armbruster 2015-06-12 22:12 ` Bandan Das 0 siblings, 1 reply; 10+ messages in thread From: Markus Armbruster @ 2015-06-12 6:05 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. 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> Doesn't apply cleanly anymore. Please double-check my conflict resolution carefully: diff --git a/monitor.c b/monitor.c index bcb88cd..0b0a8df 100644 --- a/monitor.c +++ b/monitor.c [...] @@ -4156,13 +4168,17 @@ static void handle_hmp_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) { - cmd->mhandler.cmd(mon, qdict); + qdict = monitor_parse_arguments(mon, &cmdline, cmd); + if (!qdict) { + return; } + cmd->mhandler.cmd(mon, qdict); QDECREF(qdict); } ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v5 2/4] monitor: cleanup parsing of cmd name and cmd arguments 2015-06-12 6:05 ` Markus Armbruster @ 2015-06-12 22:12 ` Bandan Das 0 siblings, 0 replies; 10+ messages in thread From: Bandan Das @ 2015-06-12 22:12 UTC (permalink / raw) To: Markus Armbruster; +Cc: qemu-devel 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> > > Doesn't apply cleanly anymore. Please double-check my conflict > resolution carefully: Looks good, Markus. Thank you for taking care of the conflict. Bandan > diff --git a/monitor.c b/monitor.c > index bcb88cd..0b0a8df 100644 > --- a/monitor.c > +++ b/monitor.c > [...] > @@ -4156,13 +4168,17 @@ static void handle_hmp_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) { > - cmd->mhandler.cmd(mon, qdict); > + qdict = monitor_parse_arguments(mon, &cmdline, cmd); > + if (!qdict) { > + return; > } > > + cmd->mhandler.cmd(mon, qdict); > QDECREF(qdict); > } > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH v5 3/4] monitor: Point to "help" command on syntax error 2015-06-03 22:38 [Qemu-devel] [PATCH v5 0/4] monitor: suggest running "help" for command errors Bandan Das 2015-06-03 22:38 ` [Qemu-devel] [PATCH v5 1/4] monitor: remove debug prints Bandan Das 2015-06-03 22:38 ` [Qemu-devel] [PATCH v5 2/4] monitor: cleanup parsing of cmd name and cmd arguments Bandan Das @ 2015-06-03 22:38 ` Bandan Das 2015-06-03 22:38 ` [Qemu-devel] [PATCH v5 4/4] monitor: Fix failure path for "S" argument Bandan Das ` (2 subsequent siblings) 5 siblings, 0 replies; 10+ messages in thread From: Bandan Das @ 2015-06-03 22:38 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. Signed-off-by: Bandan Das <bsd@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com> --- monitor.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/monitor.c b/monitor.c index 33d088e..640c05c 100644 --- a/monitor.c +++ b/monitor.c @@ -4127,6 +4127,8 @@ static void handle_user_command(Monitor *mon, const char *cmdline) qdict = monitor_parse_arguments(mon, &cmdline, 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] 10+ messages in thread
* [Qemu-devel] [PATCH v5 4/4] monitor: Fix failure path for "S" argument 2015-06-03 22:38 [Qemu-devel] [PATCH v5 0/4] monitor: suggest running "help" for command errors Bandan Das ` (2 preceding siblings ...) 2015-06-03 22:38 ` [Qemu-devel] [PATCH v5 3/4] monitor: Point to "help" command on syntax error Bandan Das @ 2015-06-03 22:38 ` Bandan Das 2015-06-08 8:53 ` [Qemu-devel] [PATCH v5 0/4] monitor: suggest running "help" for command errors Markus Armbruster 2015-06-16 15:10 ` Markus Armbruster 5 siblings, 0 replies; 10+ messages in thread From: Bandan Das @ 2015-06-03 22:38 UTC (permalink / raw) To: qemu-devel; +Cc: armbru Since the "S" argument type is only used with the "?" flag, the bug can't bite. Signed-off-by: Bandan Das <bsd@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com> --- monitor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/monitor.c b/monitor.c index 640c05c..5a18844 100644 --- a/monitor.c +++ b/monitor.c @@ -4062,7 +4062,7 @@ static QDict *monitor_parse_arguments(Monitor *mon, if (len <= 0) { monitor_printf(mon, "%s: string expected\n", cmd->name); - break; + goto fail; } qdict_put(qdict, key, qstring_from_str(p)); p += len; -- 2.1.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v5 0/4] monitor: suggest running "help" for command errors 2015-06-03 22:38 [Qemu-devel] [PATCH v5 0/4] monitor: suggest running "help" for command errors Bandan Das ` (3 preceding siblings ...) 2015-06-03 22:38 ` [Qemu-devel] [PATCH v5 4/4] monitor: Fix failure path for "S" argument Bandan Das @ 2015-06-08 8:53 ` Markus Armbruster 2015-06-11 17:58 ` Luiz Capitulino 2015-06-16 15:10 ` Markus Armbruster 5 siblings, 1 reply; 10+ messages in thread From: Markus Armbruster @ 2015-06-08 8:53 UTC (permalink / raw) To: Bandan Das; +Cc: qemu-devel, Luiz Capitulino Copying HMP maintainer Luiz. Series Reviewed-by: Markus Armbruster <armbru@redhat.com> Bandan, thanks for your patience. Luiz, my monitor/QMP queue is currently empty, but if it fills up before you get around to doing a monitor/HMP pull request, I'm happy to take this series along, if it gets your Acked-by. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v5 0/4] monitor: suggest running "help" for command errors 2015-06-08 8:53 ` [Qemu-devel] [PATCH v5 0/4] monitor: suggest running "help" for command errors Markus Armbruster @ 2015-06-11 17:58 ` Luiz Capitulino 0 siblings, 0 replies; 10+ messages in thread From: Luiz Capitulino @ 2015-06-11 17:58 UTC (permalink / raw) To: Markus Armbruster; +Cc: Bandan Das, qemu-devel On Mon, 08 Jun 2015 10:53:23 +0200 Markus Armbruster <armbru@redhat.com> wrote: > Copying HMP maintainer Luiz. > > Series > Reviewed-by: Markus Armbruster <armbru@redhat.com> > > Bandan, thanks for your patience. > > Luiz, my monitor/QMP queue is currently empty, but if it fills up before > you get around to doing a monitor/HMP pull request, I'm happy to take > this series along, if it gets your Acked-by. I'd be immensely grateful if you pick this series along with the other ones, as we've spoken in pvt. Thanks a lot Markus for your help! Acked-by: Luiz Capitulino <lcapitulino@redhat.com> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v5 0/4] monitor: suggest running "help" for command errors 2015-06-03 22:38 [Qemu-devel] [PATCH v5 0/4] monitor: suggest running "help" for command errors Bandan Das ` (4 preceding siblings ...) 2015-06-08 8:53 ` [Qemu-devel] [PATCH v5 0/4] monitor: suggest running "help" for command errors Markus Armbruster @ 2015-06-16 15:10 ` Markus Armbruster 5 siblings, 0 replies; 10+ messages in thread From: Markus Armbruster @ 2015-06-16 15:10 UTC (permalink / raw) To: Bandan Das; +Cc: qemu-devel Applied to my (badly named) qapi-next branch, thanks! ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-06-16 15:10 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-06-03 22:38 [Qemu-devel] [PATCH v5 0/4] monitor: suggest running "help" for command errors Bandan Das 2015-06-03 22:38 ` [Qemu-devel] [PATCH v5 1/4] monitor: remove debug prints Bandan Das 2015-06-03 22:38 ` [Qemu-devel] [PATCH v5 2/4] monitor: cleanup parsing of cmd name and cmd arguments Bandan Das 2015-06-12 6:05 ` Markus Armbruster 2015-06-12 22:12 ` Bandan Das 2015-06-03 22:38 ` [Qemu-devel] [PATCH v5 3/4] monitor: Point to "help" command on syntax error Bandan Das 2015-06-03 22:38 ` [Qemu-devel] [PATCH v5 4/4] monitor: Fix failure path for "S" argument Bandan Das 2015-06-08 8:53 ` [Qemu-devel] [PATCH v5 0/4] monitor: suggest running "help" for command errors Markus Armbruster 2015-06-11 17:58 ` Luiz Capitulino 2015-06-16 15:10 ` Markus Armbruster
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.