Bandan Das <b...@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 <arm...@redhat.com>
> Signed-off-by: Bandan Das <b...@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.
