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

[Jinoh Kang]

On 11/20/21 00:15, Rémi Bernon wrote:
> 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.

Ack.

> We usually just use MAX_PATH uness we really need something bigger, and then hardcoding the length in sscanf doesn't seem too bad.

Ack.  MAX_PATH sounds plenty.

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

I tend to prefer avoiding duplication of strings, since it makes me scan
the code twice for any differences from seemingly similar code.
But this is just two lines of code, it shouldn't really matter...

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

Ack.

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

Sounds reasonable.

> 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),

Yes, looks like scanf always adds the \0 terminator.

> so you can use strcpy / strcmp everywhere. There's a couple of other calls elsewhere in this patch.

I'm okay with strcmp.  For strcpy with dynamic source though,
let's say perhaps I'm going to think about it for a bit...

-- 
Sincerely,
Jinoh Kang