Re: [Qemu-devel] [PATCH v3 01/20] gdbstub: Add infrastructure to parse cmd packets

[Jon Doron]

Thank you Alex I will publish v8 with fixes from your review :) please
see my comments below

On Thu, Apr 25, 2019 at 5:42 PM Alex Bennée <address@hidden> wrote:
>
>
> address@hidden writes:
>
> > From: Jon Doron <address@hidden>
> >
> > Signed-off-by: Jon Doron <address@hidden>
> > ---
> >  gdbstub.c | 215 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 215 insertions(+)
> >
> > diff --git a/gdbstub.c b/gdbstub.c
> > index d54abd17cc..b5bd01b913 100644
> > --- a/gdbstub.c
> > +++ b/gdbstub.c
> > @@ -1268,6 +1268,221 @@ out:
> >      return res;
> >  }
> >
> > +typedef union GdbCmdVariant {
> > +    const char *data;
> > +    uint8_t opcode;
> > +    unsigned long val_ul;
> > +    unsigned long long val_ull;
> > +    struct {
> > +        GDBThreadIdKind kind;
> > +        uint32_t pid;
> > +        uint32_t tid;
> > +    } thread_id;
> > +} GdbCmdVariant;
> > +
> > +static const char *cmd_next_param(const char *param, const char delimiter)
> > +{
> > +    const char *delim;
> > +    static char all_delimiters[] = ",;:=";
> > +    static char no_delimiter[] = "\0";
> > +    char curr_delimiters[2] = {0};
> > +    const char *delimiters;
> > +
> > +    if (delimiter == '?') {
> > +        delimiters = all_delimiters;
> > +    } else if (delimiter == '0') {
> > +        delimiters = no_delimiter;
> > +    } else if (delimiter == '.' && *param) {
> > +        return param + 1;
> > +    } else {
> > +        curr_delimiters[0] = delimiter;
> > +        delimiters = curr_delimiters;
> > +    }
> > +
> > +    while (*param) {
> > +        delim = delimiters;
> > +        while (*delim) {
> > +            if (*param == *delim) {
> > +                return param + 1;
> > +            }
> > +            delim++;
> > +        }
> > +        param++;
> > +    }
> > +
> > +    return param;
> > +}
> > +
> > +static int cmd_parse_params(const char *data, const char *schema,
> > +                            GdbCmdVariant *params, int *num_params)
> > +{
> > +    int curr_param;
> > +    const char *curr_schema, *curr_data;
> > +
> > +    *num_params = 0;
> > +
> > +    if (!schema) {
> > +        return 0;
> > +    }
> > +
> > +    curr_schema = schema;
> > +    curr_param = 0;
> > +    curr_data = data;
> > +    while (curr_schema[0] && curr_schema[1] && *curr_data) {
> > +        switch (curr_schema[0]) {
> > +        case 'l':
> > +            if (qemu_strtoul(curr_data, &curr_data, 16,
> > +                             &params[curr_param].val_ul)) {
> > +                return -EINVAL;
> > +            }
> > +            curr_param++;
> > +            curr_data = cmd_next_param(curr_data, curr_schema[1]);
> > +            break;
> > +        case 'L':
> > +            if (qemu_strtou64(curr_data, &curr_data, 16,
> > +                              (uint64_t *)&params[curr_param].val_ull)) {
> > +                return -EINVAL;
> > +            }
> > +            curr_param++;
> > +            curr_data = cmd_next_param(curr_data, curr_schema[1]);
> > +            break;
> > +        case 's':
> > +            params[curr_param].data = curr_data;
> > +            curr_param++;
> > +            curr_data = cmd_next_param(curr_data, curr_schema[1]);
> > +            break;
> > +        case 'o':
> > +            params[curr_param].opcode = *(uint8_t *)curr_data;
> > +            curr_param++;
> > +            curr_data = cmd_next_param(curr_data, curr_schema[1]);
> > +            break;
> > +        case 't':
> > +            params[curr_param].thread_id.kind =
> > +                read_thread_id(curr_data, &curr_data,
> > +                               &params[curr_param].thread_id.pid,
> > +                               &params[curr_param].thread_id.tid);
> > +            curr_param++;
> > +            curr_data = cmd_next_param(curr_data, curr_schema[1]);
> > +            break;
> > +        case 'x':
> > +            params[curr_param].data = curr_data;
> > +            curr_param++;
> > +            curr_data = cmd_next_param(curr_data, curr_schema[1]);
> > +            break;
> > +        case '?':
> > +            curr_data = cmd_next_param(curr_data, curr_schema[1]);
> > +            break;
> > +        default:
> > +            return -EINVAL;
> > +        }
> > +        curr_schema += 2;
> > +    }
> > +
> > +    *num_params = curr_param;
> > +    return 0;
> > +}
> > +
> > +typedef struct GdbCmdContext {
> > +    GDBState *s;
> > +    GdbCmdVariant *params;
> > +    int num_params;
> > +    uint8_t mem_buf[MAX_PACKET_LENGTH];
> > +    char str_buf[MAX_PACKET_LENGTH + 1];
> > +} GdbCmdContext;
> > +
> > +typedef void (*GdbCmdHandler)(GdbCmdContext *gdb_ctx, void *user_ctx);
> > +
> > +/*
> > + * cmd_startswith -> cmd is compared using startswith
> > + * cmd_full_match -> cmd is compared using strcmp
>
> Doesn't !cmd_full_match imply cmd_startswith?

Done

>
> > + *
> > + *
> > + * schema definitions:
> > + * Each schema parameter entry consists of 2 chars,
> > + * the first char represents the parameter type handling
> > + * the second char represents the delimiter for the next parameter
> > + *
> > + * Currently supported schema types:
> > + * 'l' -> unsigned long (stored in .val_ul)
> > + * 'L' -> unsigned long long (stored in .val_ull)
> > + * 's' -> string (stored in .data)
> > + * 'o' -> single char (stored in .opcode)
> > + * 't' -> thread id (stored in .thread_id)
> > + * 'x' -> xml (stored in .data), must have a ':' delimiter
> > + * '?' -> skip according to delimiter
> > + *
> > + * Currently supported delimiters:
> > + * '?' -> Stop at any delimiter (",;:=\0")
> > + * '0' -> Stop at "\0"
> > + * '.' -> Skip 1 char unless reached "\0"
> > + * Any other value is treated as the delimiter value itself
> > + */
> > +typedef struct GdbCmdParseEntry {
> > +    GdbCmdHandler handler;
> > +    const char *cmd;
> > +    union {
> > +        int flags;
> > +        struct {
> > +            int cmd_startswith:1;
> > +            int cmd_full_match:1;
> > +        };
> > +    };
> > +    const char *schema;
> > +} GdbCmdParseEntry;
> > +
> > +static inline int startswith(const char *string, const char *pattern)
> > +{
> > +  return !strncmp(string, pattern, strlen(pattern));
> > +}
> > +
> > +__attribute__((unused)) static int process_string_cmd(
> > +        GDBState *s, void *user_ctx, const char *data,
> > +        GdbCmdParseEntry *cmds, int num_cmds);
> > +
> > +static int process_string_cmd(GDBState *s, void *user_ctx, const char 
> > *data,
> > +                              GdbCmdParseEntry *cmds, int num_cmds)
> > +{
> > +    int i, schema_len, max_num_params;
> > +    GdbCmdContext gdb_ctx;
> > +
> > +    if (!cmds) {
> > +        return -1;
> > +    }
> > +
> > +    for (i = 0; i < num_cmds; i++) {
>
> Allocating:
>
>         GdbCmdParseEntry *cmd = &cmds[i];
>
> might make the remaining code a little simpler to deal with.
>
>

Done

> > +        if (!cmds[i].handler || !cmds[i].cmd ||
>
> Some of these seem like asserts:
>
>         g_assert(cmd->handler && cmd->cmd);
>
> > +            (cmds[i].cmd_startswith && !startswith(data, cmds[i].cmd)) ||
> > +            (cmds[i].cmd_full_match && strcmp(data, cmds[i].cmd))) {
>
> then we can have the skip test:
>
>         if (cmd->cmd_startswith && !startswith(data, cmd->cmd)) {
>             continue;
>         } else if (strcmp(cmd->cmd, data)!=0) {
>             continue;
>         }
>
>

Done

> > +            continue;
> > +        }
> > +
> > +        max_num_params = 0;
>
> You can set the default when you declare this above
>

Done but I figured it would be nicer to only initialize when required...

> > +        if (cmds[i].schema) {
> > +            schema_len = strlen(cmds[i].schema);
> > +            if (schema_len % 2) {
> > +                return -2;
> > +            }
> > +
> > +            max_num_params = schema_len / 2;
> > +        }
> > +
> > +        gdb_ctx.params =
> > +            (GdbCmdVariant *)alloca(sizeof(*gdb_ctx.params) * 
> > max_num_params);
> > +        memset(gdb_ctx.params, 0, sizeof(*gdb_ctx.params) *
> > max_num_params);
>
> You could use the glib function instead:
>
>   gdb_ctx.params = g_newa(GdbCmdVariant, max_num_params);
>
> Although you still have to zero it which you could avoid with a g_new0()
> but you'd then have to free the pointer later. Parsing code is fiddly I
> guess...
>

Stayed with _alloca for now unless you really want me to change...

> > +
> > +        if (cmd_parse_params(&data[strlen(cmds[i].cmd)],
> > cmds[i].schema,
>
> Is the strlen appropriate if we have cmd_startswith?
>

Well yes, because you want to skip the "cmd" part and get straight to
the parameters, the strlen is on the cmd string
only, i.e: Maddr,length:XX…
the command is "M" and you want to only skip the M and get to the parameters

> > +                             gdb_ctx.params, &gdb_ctx.num_params)) {
> > +            return -1;
> > +        }
> > +
> > +        gdb_ctx.s = s;
> > +        cmds[i].handler(&gdb_ctx, user_ctx);
> > +        return 0;
> > +    }
> > +
> > +    return -1;
> > +}
> > +
> >  static int gdb_handle_packet(GDBState *s, const char *line_buf)
> >  {
> >      CPUState *cpu;
>
>
> --
> Alex Bennée