[PATCH v4 4/7] winedbg: Buffer and escape output of GDB qXfer commands.

[Rémi Bernon]

Hi Jinoh!

A few comments:

On 11/19/21 14:41, Jinoh Kang wrote:
> Signed-off-by: Jinoh Kang <jinoh.kang.kr at gmail.com>
> ---

Overall I'd say this one is probably worth splitting. Maybe consider a 
bit of prior refactoring to use the same "reply_buffer" both for the 
packet output buffer and the qxfer buffer?

At least it'd be nice to have the escaping done separately from the 
qxfer buffer addition, if possible.

>   
> +static void reply_buffer_resize(struct reply_buffer* reply, size_t alloc)
> +{
> +    reply->alloc = alloc;
> +    reply->base = realloc(reply->base, reply->alloc);
> +}
> +
> +static void reply_buffer_empty(struct reply_buffer* reply)
> +{
> +    reply->len = 0;
> +    reply_buffer_resize(reply, 0);
> +}
> +

Do we need to realloc when emptying? I think the less alloc call we do 
the better. I don't think we should worry too much about memory 
consumption here?

> +static void reply_buffer_grow(struct reply_buffer* reply, size_t size)
> +{
> +    if (reply->alloc < reply->len + size)
> +        reply_buffer_resize(reply, ((reply->len + size) / 32 + 1) * 32);
> +}
> +

Maybe just one grow helper is enough and we can drop the resize? 
Otherwise I'd also expect it to work the other way, where resize only 
realloc when needed, calling grow.

[...]

>   static enum packet_return packet_query(struct gdb_context* gdbctx)
> @@ -1929,32 +2045,50 @@ static enum packet_return packet_query(struct gdb_context* gdbctx)
>       case 'X':
>           if (sscanf(gdbctx->in_packet, "Xfer:libraries:read::%x,%x", &off, &len) == 2)
>           {
> +            BOOL more;
> +
>               if (!gdbctx->process) return packet_error;
>   
> -            packet_reply_open_xfer(gdbctx);
> +            reply_buffer_empty(&gdbctx->qxfer_buffer);
>               packet_query_libraries(gdbctx);
> -            packet_reply_close_xfer(gdbctx, off, len);
> +            packet_reply_xfer(gdbctx,
> +                              gdbctx->qxfer_buffer.base,
> +                              gdbctx->qxfer_buffer.len,
> +                              off, len, &more);
> +            reply_buffer_empty(&gdbctx->qxfer_buffer);
>               return packet_done;
>           }
>   
>           if (sscanf(gdbctx->in_packet, "Xfer:threads:read::%x,%x", &off, &len) == 2)
>           {
> +            BOOL more;
> +
>               if (!gdbctx->process) return packet_error;
>   
> -            packet_reply_open_xfer(gdbctx);
> +            reply_buffer_empty(&gdbctx->qxfer_buffer);
>               packet_query_threads(gdbctx);
> -            packet_reply_close_xfer(gdbctx, off, len);
> +            packet_reply_xfer(gdbctx,
> +                              gdbctx->qxfer_buffer.base,
> +                              gdbctx->qxfer_buffer.len,
> +                              off, len, &more);
> +            reply_buffer_empty(&gdbctx->qxfer_buffer);
>               return packet_done;
>           }
>   
>           if (sscanf(gdbctx->in_packet, "Xfer:features:read:target.xml:%x,%x", &off, &len) == 2)
>           {
> +            BOOL more;
> +

I think these more variables are not even used at this point, you could 
probably only add them later.

>               if (!gdbctx->process) return packet_error;
>               if (!(cpu = gdbctx->process->be_cpu)) return packet_error;
>   
> -            packet_reply_open_xfer(gdbctx);
> +            reply_buffer_empty(&gdbctx->qxfer_buffer);
>               packet_query_target_xml(gdbctx, cpu);
> -            packet_reply_close_xfer(gdbctx, off, len);
> +            packet_reply_xfer(gdbctx,
> +                              gdbctx->qxfer_buffer.base,
> +                              gdbctx->qxfer_buffer.len,
> +                              off, len, &more);
> +            reply_buffer_empty(&gdbctx->qxfer_buffer);
>               return packet_done;
>           }
>           break;

Do we really need to empty the buffer before and after?

Cheers,
-- 
Rémi Bernon <rbernon at codeweavers.com>