monitor: suggest running "help" for command errors
v5 cover monitor: suggest running "help" for command errors
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(-)
Signature · 1 line
2.1.0
PATCH v5 1/4 monitor: remove debug prints
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.
--- monitor.c | 20 +------------------- 1 file changed, 1 insertion(+), 19 deletions(-)
monitor.c +1 -19
@@ -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 */
Signature · 1 line
2.1.0
PATCH v5 2/4 monitor: cleanup parsing of cmd name and cmd arguments
In reply to Bandan DasThere'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.
--- monitor.c | 98 +++++++++++++++++++++++++++++++++++++-------------------------- 1 file changed, 57 insertions(+), 41 deletions(-)
monitor.c +57 -41
@@ -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); }
Signature · 1 line
2.1.0
Re: [Qemu-devel] [PATCH v5 2/4] monitor: cleanup parsing of cmd name and cmd arguments
Quoted 10 lines · Bandan Das
There's too much going on in
monitor_parse_command(). Split up the arguments parsing bits into a separate functionmonitor_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 <[email protected]>Signed-off-by Bandan Das <[email protected]>
Doesn't apply cleanly anymore. Please double-check my conflict resolution carefully:
monitor.c +0 -0
[...]
@@ -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);}
Re: [Qemu-devel] [PATCH v5 2/4] monitor: cleanup parsing of cmd name and cmd arguments
Quoted 10 lines · Markus Armbruster
Quoted 8 lines · Bandan Das
There's too much going on in
monitor_parse_command(). Split up the arguments parsing bits into a separate functionmonitor_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.
Quoted 5 lines
Quoted 2 lines
Suggested-by Markus Armbruster <[email protected]>Signed-off-by Bandan Das <[email protected]>Doesn't apply cleanly anymore. Please double-check my conflict resolution carefully:
Looks good, Markus. Thank you for taking care of the conflict.
Bandan
Quoted 27 lines
monitor.c +8
-4@@ -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); }
PATCH v5 3/4 monitor: Point to "help" command on syntax error
In reply to Bandan DasWhen 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.
--- monitor.c | 2 ++ 1 file changed, 2 insertions(+)
monitor.c +2 -0
@@ -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 ; }
Signature · 1 line
2.1.0
PATCH v5 4/4 monitor: Fix failure path for "S" argument
In reply to Bandan DasSince the "S" argument type is only used with the "?" flag, the bug can't bite.
--- monitor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
monitor.c +1 -1
@@ -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;
Signature · 1 line
2.1.0
Copying HMP maintainer Luiz.
Series
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.
Luiz Capitulino
Acked-byQuoted 10 lines · On Mon, 08 Jun 2015 10:53:23 +0200 Markus Armbruster <[email protected]> wrote:
Copying HMP maintainer Luiz.
Series
Reviewed-by Markus Armbruster <[email protected]>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!
Markus Armbruster
AppliedApplied to my (badly named) qapi-next branch, thanks!