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

[Alex Bennée]

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?

> + *
> + *
> + * 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.


> +        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;
        }


> +            continue;
> +        }
> +
> +        max_num_params = 0;

You can set the default when you declare this above

> +        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...

> +
> +        if (cmd_parse_params(&data[strlen(cmds[i].cmd)],
> cmds[i].schema,

Is the strlen appropriate if we have cmd_startswith?

> +                             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