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