diff mbox series

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

Message ID 20240412073346.458116-4-richard.henderson@linaro.org
State Superseded
Headers show
Series misc: Replace sprintf | expand

Commit Message

Richard Henderson April 12, 2024, 7:33 a.m. UTC
Allocate a new, or append to an existing GString instead of
using a fixed sized buffer.  Require the caller to determine
the length of the line -- do not bound len here.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/qemu/cutils.h  | 15 ++++++++++-----
 hw/virtio/vhost-vdpa.c | 14 ++++++++------
 util/hexdump.c         | 29 +++++++++++++++++------------
 3 files changed, 35 insertions(+), 23 deletions(-)

Comments

Philippe Mathieu-Daudé April 12, 2024, 5:41 p.m. UTC | #1
On 12/4/24 09:33, Richard Henderson wrote:
> Allocate a new, or append to an existing GString instead of
> using a fixed sized buffer.  Require the caller to determine
> the length of the line -- do not bound len here.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   include/qemu/cutils.h  | 15 ++++++++++-----
>   hw/virtio/vhost-vdpa.c | 14 ++++++++------
>   util/hexdump.c         | 29 +++++++++++++++++------------
>   3 files changed, 35 insertions(+), 23 deletions(-)
> 
> diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
> index d0c5386e6c..7311fb36ca 100644
> --- a/include/qemu/cutils.h
> +++ b/include/qemu/cutils.h
> @@ -252,12 +252,17 @@ static inline const char *yes_no(bool b)
>    */
>   int parse_debug_env(const char *name, int max, int initial);
>   
> -/*
> - * Hexdump a line of a byte buffer into a hexadecimal/ASCII buffer
> +/**
> + * qemu_hexdump_line:
> + * @str: GString into which to append
> + * @buf: buffer to dump
> + * @len: number of bytes to dump
> + *
> + * Append @len bytes of @buf as hexadecimal into @str.
> + * If @str is NULL, allocate a new string and return it;
> + * otherwise return @str.
>    */
> -#define QEMU_HEXDUMP_LINE_BYTES 16 /* Number of bytes to dump */
> -#define QEMU_HEXDUMP_LINE_LEN 75   /* Number of characters in line */
> -void qemu_hexdump_line(char *line, const void *bufptr, size_t len);
> +GString *qemu_hexdump_line(GString *str, const void *buf, size_t len);


> diff --git a/util/hexdump.c b/util/hexdump.c
> index dbc536fe84..521e346bc6 100644
> --- a/util/hexdump.c
> +++ b/util/hexdump.c
> @@ -16,22 +16,25 @@
>   #include "qemu/osdep.h"
>   #include "qemu/cutils.h"
>   
> -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);
          }

>   
>       for (i = 0; i < len; i++) {
>           if (i != 0 && (i % 4) == 0) {
> -            *line++ = ' ';
> +            g_string_append_c(str, ' ');
>           }
> -        line += sprintf(line, " %02x", (unsigned char)buf[i]);
> +        g_string_append_printf(str, " %02x", buf[i]);
>       }
> -    *line = '\0';
> +
> +    return str;
>   }
>   
>   static void asciidump_line(char *line, const void *bufptr, size_t len)
> @@ -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 [*] ?

> +        qemu_hexdump_line(str, bufptr + b, len);
>           asciidump_line(ascii, bufptr + b, len);
>   
> -        fprintf(fp, "%s: %04x: %-*s %s\n",
> -                prefix, b, QEMU_HEXDUMP_LINE_WIDTH, line, ascii);
> +        fprintf(fp, "%s: %04zx: %-*s %s\n",
> +                prefix, b, QEMU_HEXDUMP_LINE_WIDTH, str->str, ascii);
>       }
>   
>   }
Richard Henderson April 12, 2024, 6:59 p.m. UTC | #2
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.


r~
Philippe Mathieu-Daudé April 13, 2024, 9:45 a.m. UTC | #3
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.

Regards,

Phil.
Stefan Hajnoczi April 13, 2024, 12:02 p.m. UTC | #4
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
diff mbox series

Patch

diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index d0c5386e6c..7311fb36ca 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -252,12 +252,17 @@  static inline const char *yes_no(bool b)
  */
 int parse_debug_env(const char *name, int max, int initial);
 
-/*
- * Hexdump a line of a byte buffer into a hexadecimal/ASCII buffer
+/**
+ * qemu_hexdump_line:
+ * @str: GString into which to append
+ * @buf: buffer to dump
+ * @len: number of bytes to dump
+ *
+ * Append @len bytes of @buf as hexadecimal into @str.
+ * If @str is NULL, allocate a new string and return it;
+ * otherwise return @str.
  */
-#define QEMU_HEXDUMP_LINE_BYTES 16 /* Number of bytes to dump */
-#define QEMU_HEXDUMP_LINE_LEN 75   /* Number of characters in line */
-void qemu_hexdump_line(char *line, const void *bufptr, size_t len);
+GString *qemu_hexdump_line(GString *str, const void *buf, size_t len);
 
 /*
  * Hexdump a buffer to a file. An optional string prefix is added to every line
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 32bad5ce68..ab047d8ee0 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -941,13 +941,15 @@  static int vhost_vdpa_set_config_call(struct vhost_dev *dev,
 static void vhost_vdpa_dump_config(struct vhost_dev *dev, const uint8_t *config,
                                    uint32_t config_len)
 {
-    int b, len;
-    char line[QEMU_HEXDUMP_LINE_LEN];
+    g_autoptr(GString) str = g_string_sized_new(4 * 16);
+    size_t b, len;
 
-    for (b = 0; b < config_len; b += 16) {
-        len = config_len - b;
-        qemu_hexdump_line(line, config + b, len);
-        trace_vhost_vdpa_dump_config(dev, b, line);
+    for (b = 0; b < config_len; b += len) {
+        len = MIN(config_len - b, 16);
+
+        g_string_truncate(str, 0);
+        qemu_hexdump_line(str, config + b, len);
+        trace_vhost_vdpa_dump_config(dev, b, str->str);
     }
 }
 
diff --git a/util/hexdump.c b/util/hexdump.c
index dbc536fe84..521e346bc6 100644
--- a/util/hexdump.c
+++ b/util/hexdump.c
@@ -16,22 +16,25 @@ 
 #include "qemu/osdep.h"
 #include "qemu/cutils.h"
 
-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);
     }
 
     for (i = 0; i < len; i++) {
         if (i != 0 && (i % 4) == 0) {
-            *line++ = ' ';
+            g_string_append_c(str, ' ');
         }
-        line += sprintf(line, " %02x", (unsigned char)buf[i]);
+        g_string_append_printf(str, " %02x", buf[i]);
     }
-    *line = '\0';
+
+    return str;
 }
 
 static void asciidump_line(char *line, const void *bufptr, size_t len)
@@ -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);
+        qemu_hexdump_line(str, bufptr + b, len);
         asciidump_line(ascii, bufptr + b, len);
 
-        fprintf(fp, "%s: %04x: %-*s %s\n",
-                prefix, b, QEMU_HEXDUMP_LINE_WIDTH, line, ascii);
+        fprintf(fp, "%s: %04zx: %-*s %s\n",
+                prefix, b, QEMU_HEXDUMP_LINE_WIDTH, str->str, ascii);
     }
 
 }