[PATCH v4 5/7] winedbg: Define table for GDB qXfer command handlers.

[Rémi Bernon]

On 11/19/21 14:41, Jinoh Kang wrote:
> +struct qxfer
> +{
> +    const char*        name;
> +    enum packet_return (*handler)(struct gdb_context* gdbctx);
> +} qxfer_handlers[] =
> +{
> +    {"libraries", packet_query_libraries},
> +    {"threads"  , packet_query_threads  },
> +    {"features" , packet_query_features },
> +};
> +
> +static BOOL parse_xfer_read(const char* ptr, char* name_buf, char* annex_buf, unsigned int* offp, unsigned int* lenp)
> +{
> +    int n;
> +
> +    /* Xfer:<object>:read:<annex>:<offset-hex>,<length-hex> */
> +
> +    if (sscanf(ptr, "Xfer:%" STRINGIFY(MAX_QX_NAME_LEN) "[^:]:read:%n", name_buf, &n) != 1)
> +        return FALSE;
> +    ptr += (unsigned int)n;
> +
> +    if (sscanf(ptr, "%" STRINGIFY(MAX_QX_ANNEX_LEN) "[^:]%n", annex_buf, &n) != 1)
> +    {
> +        annex_buf[0] = '\0';
> +        n = 0;
> +    }
> +    ptr += (unsigned int)n;
> +
> +    return sscanf(ptr, ":%x,%x", offp, lenp) == 2;
> +}
> +

I feel that this is much more complicated than it needs to be, 
expecially with the stringify to get buffer lengths. We usually just use 
MAX_PATH uness we really need something bigger, and then hardcoding the 
length in sscanf doesn't seem too bad.

Something like this below instead of this helper should be more readable 
IMHO:

> char object_name[MAX_PATH] = {0}, annex[MAX_PATH] = {0};
> 
> /* [...] */
> 
>     if (sscanf(gdbctx->in_packet, "Xfer:%256[^:]:read:%256[^:]:%x,%x", object_name, annex, &off, &len) == 4 ||
>         sscanf(gdbctx->in_packet, "Xfer:%256[^:]:read::%x,%x", object_name, &off, &len) == 3)


>   static enum packet_return packet_query(struct gdb_context* gdbctx)
>   {
> +    char object_name[QX_NAME_SIZE], annex[QX_ANNEX_SIZE];
>       unsigned int off, len;
> -    struct backend_cpu *cpu;
>   
>       switch (gdbctx->in_packet[0])
>       {
> @@ -2008,11 +2082,16 @@ static enum packet_return packet_query(struct gdb_context* gdbctx)
>               return packet_ok;
>           if (strncmp(gdbctx->in_packet, "Supported", 9) == 0)
>           {
> +            size_t i;
> +
>               packet_reply_open(gdbctx);
>               packet_reply_add(gdbctx, "QStartNoAckMode+;");
> -            packet_reply_add(gdbctx, "qXfer:libraries:read+;");
> -            packet_reply_add(gdbctx, "qXfer:threads:read+;");
> -            packet_reply_add(gdbctx, "qXfer:features:read+;");
> +            for (i = 0; i < ARRAY_SIZE(qxfer_handlers); i++)
> +            {
> +                packet_reply_add(gdbctx, "qXfer:");
> +                packet_reply_add(gdbctx, qxfer_handlers[i].name);
> +                packet_reply_add(gdbctx, ":read+;");
> +            }
>               packet_reply_close(gdbctx);
>               return packet_done;
>           }
> @@ -2043,53 +2122,47 @@ static enum packet_return packet_query(struct gdb_context* gdbctx)
>           }
>           break;
>       case 'X':
> -        if (sscanf(gdbctx->in_packet, "Xfer:libraries:read::%x,%x", &off, &len) == 2)
> +        if (parse_xfer_read(gdbctx->in_packet, object_name, annex, &off, &len))
>           {
> +            enum packet_return result;
> +            int i;
>               BOOL more;
>   
> -            if (!gdbctx->process) return packet_error;
> +            for (i = 0; ; i++)
> +            {
> +                if (i >= ARRAY_SIZE(qxfer_handlers))
> +                {
> +                    ERR("unhandled qXfer %s read %s %u,%u\n", debugstr_a(object_name), debugstr_a(annex), off, len);
> +                    return packet_error;
> +                }
> +                if (strncmp(qxfer_handlers[i].name, object_name, QX_NAME_SIZE) == 0)
> +                    break;
> +            }
>   

I usually prefer the pattern where the loop does lookup and break, and 
the outcome is checked outside, like:

> for (i = 0; i < ARRAY_SIZE(qxfer_handlers); i++)
>     if (!strcmp(qxfer_handlers[i].name, object_name))
>         break;
> if (i == ARRAY_SIZE(qxfer_handlers))
> {
>     ERR("unhandled qXfer %s read %s %u,%u\n", debugstr_a(object_name),
>         debugstr_a(annex), off, len);
>     return packet_error;
> }


> -        if (sscanf(gdbctx->in_packet, "Xfer:features:read:target.xml:%x,%x", &off, &len) == 2)
> -        {
> -            BOOL more;
> +            gdbctx->qxfer_object_idx = i;
> +            lstrcpynA(gdbctx->qxfer_object_annex, annex, QX_ANNEX_SIZE);

I think you can probably assume zero-terminated strings, especially if 
the buffers are zero initialized and a smaller size is passed to sscanf 
(but I think it's supposed to add a zero terminator anyway), so you can 
use strcpy / strcmp everywhere. There's a couple of other calls 
elsewhere in this patch.
-- 
Rémi Bernon <rbernon at codeweavers.com>