Re: [Qemu-devel] [PATCH v3 11/15] monitor: Split out monitor/hmp.c

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Move HMP infrastructure from monitor/misc.c to monitor/hmp.c. This is
> code that can be shared for all targets, so compile it only once.
>
> The amount of function and particularly extern variables in
> monitor_int.h is probably a bit larger than it needs to be, but this way
> no non-trivial code modifications are needed. The interfaces between HMP
> and the monitor core can be cleaned up later.
>
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
[...]
> diff --git a/monitor/hmp.c b/monitor/hmp.c
> new file mode 100644
> index 0000000000..3621b195ed
> --- /dev/null
> +++ b/monitor/hmp.c
> @@ -0,0 +1,1415 @@
[...]
> +static int64_t expr_unary(Monitor *mon)
> +{
> +    int64_t n;
> +    char *p;
> +    int ret;
> +
> +    switch (*pch) {
> +    case '+':
> +        next();
> +        n = expr_unary(mon);
> +        break;
> +    case '-':
> +        next();
> +        n = -expr_unary(mon);
> +        break;
> +    case '~':
> +        next();
> +        n = ~expr_unary(mon);
> +        break;
> +    case '(':
> +        next();
> +        n = expr_sum(mon);
> +        if (*pch != ')') {
> +            expr_error(mon, "')' expected");
> +        }
> +        next();
> +        break;
> +    case '\'':
> +        pch++;
> +        if (*pch == '\0') {
> +            expr_error(mon, "character constant expected");
> +        }
> +        n = *pch;
> +        pch++;
> +        if (*pch != '\'') {
> +            expr_error(mon, "missing terminating \' character");
> +        }
> +        next();
> +        break;
> +    case '$':
> +        {
> +            char buf[128], *q;
> +            int64_t reg = 0;
> +
> +            pch++;
> +            q = buf;
> +            while ((*pch >= 'a' && *pch <= 'z') ||
> +                   (*pch >= 'A' && *pch <= 'Z') ||
> +                   (*pch >= '0' && *pch <= '9') ||
> +                   *pch == '_' || *pch == '.') {
> +                if ((q - buf) < sizeof(buf) - 1) {
> +                    *q++ = *pch;
> +                }
> +                pch++;
> +            }
> +            while (qemu_isspace(*pch)) {
> +                pch++;
> +            }
> +            *q = 0;
> +            ret = get_monitor_def(&reg, buf);
> +            if (ret < 0) {
> +                expr_error(mon, "unknown register");
> +            }
> +            n = reg;
> +        }
> +        break;
> +    case '\0':
> +        expr_error(mon, "unexpected end of expression");
> +        n = 0;
> +        break;
> +    default:
> +        errno = 0;
> +        n = strtoull(pch, &p, 0);

checkpatch.pl gripes:

    ERROR: consider using qemu_strtoull in preference to strtoull


Let's add a TODO comment.  

> +        if (errno == ERANGE) {
> +            expr_error(mon, "number too large");
> +        }
> +        if (pch == p) {
> +            expr_error(mon, "invalid char '%c' in expression", *p);
> +        }
> +        pch = p;
> +        while (qemu_isspace(*pch)) {
> +            pch++;
> +        }
> +        break;
> +    }
> +    return n;
> +}
[...]
> +static void monitor_find_completion(void *opaque,
> +                                    const char *cmdline)
> +{
> +    MonitorHMP *mon = opaque;
> +    char *args[MAX_ARGS];
> +    int nb_args, len;
> +
> +    /* 1. parse the cmdline */
> +    if (parse_cmdline(cmdline, &nb_args, args) < 0) {
> +        return;
> +    }
> +
> +    /* if the line ends with a space, it means we want to complete the
> +     * next arg */

checkpatch.pl again:

    WARNING: Block comments use a leading /* on a separate line
    WARNING: Block comments use a trailing */ on a separate line

Can touch up in my tree.

> +    len = strlen(cmdline);
> +    if (len > 0 && qemu_isspace(cmdline[len - 1])) {
> +        if (nb_args >= MAX_ARGS) {
> +            goto cleanup;
> +        }
> +        args[nb_args++] = g_strdup("");
> +    }
> +
> +    /* 2. auto complete according to args */
> +    monitor_find_completion_by_table(mon, hmp_cmds, args, nb_args);
> +
> +cleanup:
> +    free_cmdline_args(args, nb_args);
> +}
[...]
> diff --git a/monitor/misc.c b/monitor/misc.c
> index 368b8297d4..c8289959c0 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
[...]
> @@ -612,245 +580,27 @@ out:
>      return output;
>  }
>  
> -static int compare_cmd(const char *name, const char *list)
> +/**
> + * Is @name in the '|' separated list of names @list?
> + */
> +int hmp_compare_cmd(const char *name, const char *list)
>  {
>      const char *p, *pstart;
>      int len;
>      len = strlen(name);
>      p = list;
> -    for(;;) {
> +    for (;;) {
>          pstart = p;
>          p = qemu_strchrnul(p, '|');
> -        if ((p - pstart) == len && !memcmp(pstart, name, len))
> +        if ((p - pstart) == len && !memcmp(pstart, name, len)) {
>              return 1;

The diff gets confusing here.  The function remains unchanged.  Good.

> -        if (*p == '\0')
> -            break;
> -        p++;
> -    }
> -    return 0;
> -}
> -
> -static int get_str(char *buf, int buf_size, const char **pp)
> -{
> -    const char *p;
> -    char *q;
> -    int c;
> -
> -    q = buf;
> -    p = *pp;
> -    while (qemu_isspace(*p)) {
> -        p++;
> -    }
> -    if (*p == '\0') {
> -    fail:
> -        *q = '\0';
> -        *pp = p;
> -        return -1;
> -    }
> -    if (*p == '\"') {
> -        p++;
> -        while (*p != '\0' && *p != '\"') {
> -            if (*p == '\\') {
> -                p++;
> -                c = *p++;
> -                switch (c) {
> -                case 'n':
> -                    c = '\n';
> -                    break;
> -                case 'r':
> -                    c = '\r';
> -                    break;
> -                case '\\':
> -                case '\'':
> -                case '\"':
> -                    break;
> -                default:
> -                    printf("unsupported escape code: '\\%c'\n", c);
> -                    goto fail;
> -                }
> -                if ((q - buf) < buf_size - 1) {
> -                    *q++ = c;
> -                }
> -            } else {
> -                if ((q - buf) < buf_size - 1) {
> -                    *q++ = *p;
> -                }
> -                p++;
> -            }
> -        }
> -        if (*p != '\"') {
> -            printf("unterminated string\n");
> -            goto fail;
> -        }
> -        p++;
> -    } else {
> -        while (*p != '\0' && !qemu_isspace(*p)) {
> -            if ((q - buf) < buf_size - 1) {
> -                *q++ = *p;
> -            }
> -            p++;
> -        }
> -    }
> -    *q = '\0';
> -    *pp = p;
> -    return 0;
> -}
> -
> -#define MAX_ARGS 16
> -
> -static void free_cmdline_args(char **args, int nb_args)
> -{
> -    int i;
> -
> -    assert(nb_args <= MAX_ARGS);
> -
> -    for (i = 0; i < nb_args; i++) {
> -        g_free(args[i]);
> -    }
> -
> -}
> -
> -/*
> - * Parse the command line to get valid args.
> - * @cmdline: command line to be parsed.
> - * @pnb_args: location to store the number of args, must NOT be NULL.
> - * @args: location to store the args, which should be freed by caller, must
> - *        NOT be NULL.
> - *
> - * Returns 0 on success, negative on failure.
> - *
> - * NOTE: this parser is an approximate form of the real command parser. 
> Number
> - *       of args have a limit of MAX_ARGS. If cmdline contains more, it will
> - *       return with failure.
> - */
> -static int parse_cmdline(const char *cmdline,
> -                         int *pnb_args, char **args)
> -{
> -    const char *p;
> -    int nb_args, ret;
> -    char buf[1024];
> -
> -    p = cmdline;
> -    nb_args = 0;
> -    for (;;) {
> -        while (qemu_isspace(*p)) {
> -            p++;
>          }
>          if (*p == '\0') {
>              break;
>          }
> -        if (nb_args >= MAX_ARGS) {
> -            goto fail;
> -        }
> -        ret = get_str(buf, sizeof(buf), &p);
> -        if (ret < 0) {
> -            goto fail;
> -        }
> -        args[nb_args] = g_strdup(buf);
> -        nb_args++;
> +        p++;
>      }
> -    *pnb_args = nb_args;
>      return 0;
> -
> - fail:
> -    free_cmdline_args(args, nb_args);
> -    return -1;
> -}
> -
> -/*
> - * Can command @cmd be executed in preconfig state?
> - */
> -static bool cmd_can_preconfig(const HMPCommand *cmd)
> -{
> -    if (!cmd->flags) {
> -        return false;
> -    }
> -
> -    return strchr(cmd->flags, 'p');
> -}
> -
> -static void help_cmd_dump_one(Monitor *mon,
> -                              const HMPCommand *cmd,
> -                              char **prefix_args,
> -                              int prefix_args_nb)
> -{
> -    int i;
> -
> -    if (runstate_check(RUN_STATE_PRECONFIG) && !cmd_can_preconfig(cmd)) {
> -        return;
> -    }
> -
> -    for (i = 0; i < prefix_args_nb; i++) {
> -        monitor_printf(mon, "%s ", prefix_args[i]);
> -    }
> -    monitor_printf(mon, "%s %s -- %s\n", cmd->name, cmd->params, cmd->help);
> -}
> -
> -/* @args[@arg_index] is the valid command need to find in @cmds */
> -static void help_cmd_dump(Monitor *mon, const HMPCommand *cmds,
> -                          char **args, int nb_args, int arg_index)
> -{
> -    const HMPCommand *cmd;
> -    size_t i;
> -
> -    /* No valid arg need to compare with, dump all in *cmds */
> -    if (arg_index >= nb_args) {
> -        for (cmd = cmds; cmd->name != NULL; cmd++) {
> -            help_cmd_dump_one(mon, cmd, args, arg_index);
> -        }
> -        return;
> -    }
> -
> -    /* Find one entry to dump */
> -    for (cmd = cmds; cmd->name != NULL; cmd++) {
> -        if (compare_cmd(args[arg_index], cmd->name) &&
> -            ((!runstate_check(RUN_STATE_PRECONFIG) ||
> -                cmd_can_preconfig(cmd)))) {
> -            if (cmd->sub_table) {
> -                /* continue with next arg */
> -                help_cmd_dump(mon, cmd->sub_table,
> -                              args, nb_args, arg_index + 1);
> -            } else {
> -                help_cmd_dump_one(mon, cmd, args, arg_index);
> -            }
> -            return;
> -        }
> -    }
> -
> -    /* Command not found */
> -    monitor_printf(mon, "unknown command: '");
> -    for (i = 0; i <= arg_index; i++) {
> -        monitor_printf(mon, "%s%s", args[i], i == arg_index ? "'\n" : " ");
> -    }
> -}
> -
> -static void help_cmd(Monitor *mon, const char *name)
> -{
> -    char *args[MAX_ARGS];
> -    int nb_args = 0;
> -
> -    /* 1. parse user input */
> -    if (name) {
> -        /* special case for log, directly dump and return */
> -        if (!strcmp(name, "log")) {
> -            const QEMULogItem *item;
> -            monitor_printf(mon, "Log items (comma separated):\n");
> -            monitor_printf(mon, "%-10s %s\n", "none", "remove all logs");
> -            for (item = qemu_log_items; item->mask != 0; item++) {
> -                monitor_printf(mon, "%-10s %s\n", item->name, item->help);
> -            }
> -            return;
> -        }
> -
> -        if (parse_cmdline(name, &nb_args, args) < 0) {
> -            return;
> -        }
> -    }
> -
> -    /* 2. dump the contents according to parsed args */
> -    help_cmd_dump(mon, hmp_cmds, args, nb_args, 0);
> -
> -    free_cmdline_args(args, nb_args);
>  }
[...]

Reviewed-by: Markus Armbruster <address@hidden>