[PATCH v6 1/6] winedbg: Refactor gdb_context::out_{buf*, len} into reply_buffer.

[Jinoh Kang]

On 11/23/21 20:20, Rémi Bernon wrote:
> Hi Jinoh!
> 
> A few nitpicks, otherwise it looks good.
> 
> On 11/22/21 15:59, Jinoh Kang wrote:
>> This is required for a subsequent patch that adds buffering for
>> GDB qXfer reply data.
>>
>> Signed-off-by: Jinoh Kang <jinoh.kang.kr at gmail.com>
>> ---
>>
>> Notes:
>>      v5 -> v6: Ddit commit message
>>
>>   programs/winedbg/gdbproxy.c | 141 ++++++++++++++++++++++--------------
>>   1 file changed, 87 insertions(+), 54 deletions(-)
>>
>> diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c
>> index c1d0bda1a41..2e7e04e6c93 100644
>> --- a/programs/winedbg/gdbproxy.c
>> +++ b/programs/winedbg/gdbproxy.c
>> @@ -54,6 +54,13 @@ struct gdb_xpoint
>>       unsigned int value;
>>   };
>>   +struct reply_buffer
>> +{
>> +    void *base;
>> +    size_t len;
>> +    size_t alloc;
>> +};
>> +
> 
> I think you can save a few casts by making base an unsigned char*. I don't think it's really useful to keep void* here (also note that this file seems to use left pointer alignment everywhere).
> 
>>   struct gdb_context
>>   {
>>       /* gdb information */
>> @@ -66,9 +73,7 @@ struct gdb_context
>>       char*                       in_packet;
>>       int                         in_packet_len;
>>       /* outgoing buffer */
>> -    char*                       out_buf;
>> -    int                         out_buf_alloc;
>> -    int                         out_len;
>> +    struct reply_buffer         out_buf;
>>       int                         out_curr_packet;
>>       /* generic GDB thread information */
>>       int                         exec_tid; /* tid used in step & continue */
>> @@ -224,12 +229,62 @@ static void hex_to(char* dst, const void* src, size_t len)
>>       }
>>   }
>>   -static unsigned char checksum(const char* ptr, int len)
>> +static void reply_buffer_empty(struct reply_buffer* reply)
>> +{
>> +    reply->len = 0;
>> +}
>> +
> 
> I know C++ made it canonical to use "empty" as a verb to clear containers but I feel like "reset" or "clear" is better.
> 
>> +static void reply_buffer_grow(struct reply_buffer* reply, size_t size)
>> +{
>> +    if (reply->alloc < reply->len + size)
>> +    {
>> +        reply->alloc = ((reply->len + size) / 32 + 1) * 32;
>> +        reply->base = realloc(reply->base, reply->alloc);
>> +    }
>> +}
> 
> Could we maybe grow by more than that? Something like:
> 
> (max(reply->alloc * 3 / 2, reply->len + size) + 0x1f) & ~0x1f
> 
> (I would even prefer to drop the alignment, but I don't remember why or if it was actually needed)
> 
> Or maybe we don't want to do that in this patch, which should not change the logic... As you prefer.
> 
>> +
>> +static void reply_buffer_append(struct reply_buffer* reply, const void* data, size_t size)
>> +{
>> +    reply_buffer_grow(reply, size);
>> +    memcpy((unsigned char *)reply->base + reply->len, data, size);
>> +    reply->len += size;
>> +}
>> +
>> +static inline void reply_buffer_append_str(struct reply_buffer* reply, const char* str)
>> +{
>> +    reply_buffer_append(reply, (const void *)str, strlen(str));
>> +}
>> +
> 
> This function is unused in this patch, it should be added in the next one, where it is. You probably don't need the inline by the way, here or elsewhere, and you don't need to cast to const void* either.

I just realised I forgot to take the inline keyword out.  The reason why inline
is there was simply because the old code was using it; nothing more.  I don't
think its presence or absence would make a big difference, though.

> 
>> +static inline void reply_buffer_append_hex(struct reply_buffer* reply, const void* src, size_t len)
>> +{
>> +    reply_buffer_grow(reply, len * 2);
>> +    hex_to((char *)reply->base + reply->len, src, len);
>> +    reply->len += len * 2;
>> +}
>> +
> Cheers,

-- 
Sincerely,
Jinoh Kang