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

[Rémi Bernon]

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.

> +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,
-- 
Rémi Bernon <rbernon at codeweavers.com>