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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Markus Armbruster (armbru@redhat.com) wrote:
> Kevin Wolf <kwolf@redhat.com> 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 <kwolf@redhat.com>
> > ---
> [...]
> > 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.

I wouldn't worry too much about fixing the existing problems here -
let's get the reorg done through kwolf's patches and then it's easier
to clean up later.

Dave

> > +    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 <armbru@redhat.com>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK