Re: [PATCH v3 03/27] util/hexdump: Use a GString for qemu_hexdump_line

[Stefan Hajnoczi]

On Sat, 13 Apr 2024 at 05:46, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 12/4/24 20:59, Richard Henderson wrote:
> > On 4/12/24 10:41, Philippe Mathieu-Daudé wrote:
> >>> -void qemu_hexdump_line(char *line, const void *bufptr, size_t len)
> >>> +GString *qemu_hexdump_line(GString *str, const void *vbuf, size_t len)
> >>>   {
> >>> -    const char *buf = bufptr;
> >>> -    int i, c;
> >>> +    const uint8_t *buf = vbuf;
> >>> +    size_t i;
> >>> -    if (len > QEMU_HEXDUMP_LINE_BYTES) {
> >>> -        len = QEMU_HEXDUMP_LINE_BYTES;
> >>> +    if (str == NULL) {
> >>> +        /* Estimate the length of the output to avoid reallocs. */
> >>> +        i = len * 3 + len / 4;
> >>> +        str = g_string_sized_new(i + 1);
> >>>       }
> >>
> >> [*]
> >>           else {
> >>             g_string_truncate(str, 0);
> >>           }
> >>
> > ...
> >>> @@ -49,24 +52,26 @@ static void asciidump_line(char *line, const void
> >>> *bufptr, size_t len)
> >>>       *line = '\0';
> >>>   }
> >>> +#define QEMU_HEXDUMP_LINE_BYTES 16
> >>>   #define QEMU_HEXDUMP_LINE_WIDTH \
> >>>       (QEMU_HEXDUMP_LINE_BYTES * 2 + QEMU_HEXDUMP_LINE_BYTES / 4)
> >>>   void qemu_hexdump(FILE *fp, const char *prefix,
> >>>                     const void *bufptr, size_t size)
> >>>   {
> >>> -    char line[QEMU_HEXDUMP_LINE_LEN];
> >>> +    g_autoptr(GString) str =
> >>> g_string_sized_new(QEMU_HEXDUMP_LINE_WIDTH + 1);
> >>>       char ascii[QEMU_HEXDUMP_LINE_BYTES + 1];
> >>>       size_t b, len;
> >>>       for (b = 0; b < size; b += len) {
> >>>           len = MIN(size - b, QEMU_HEXDUMP_LINE_BYTES);
> >>> -        qemu_hexdump_line(line, bufptr + b, len);
> >>> +        g_string_truncate(str, 0);
> >>
> >> Shouldn't we truncate in [*] ?
> >
> > The usage in tpm puts several lines together in one string,
> > adding \n in between, for output in one go.
>
> I see the trace_tpm_util_show_buffer() call. However this
> isn't a recommended use of the tracing API (Cc'ing Stefan).
> It breaks the "log" backend output, and is sub-optimal for
> all other backends.
>
> IMHO the TPM buffer should be traced by multiple calls of
> (offset, hexbuf) instead.

I think so too.

Stefan