* Markus Armbruster (arm...@redhat.com) wrote:
> Kevin Wolf <kw...@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 <kw...@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(®, 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 <arm...@redhat.com>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
