* [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
* [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 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
* 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.