Re: [PATCH v3 02/27] util/hexdump: Remove ascii parameter from qemu_hexdump_line

[Philippe Mathieu-Daudé]

On 12/4/24 09:33, Richard Henderson wrote:
> Split out asciidump_line as a separate function, local to hexdump.c,
> for use by qemu_hexdump.  Use "%-*s" to generate the alignment
> between the hex and the ascii, rather than explicit spaces.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   include/qemu/cutils.h  |  3 +--
>   hw/virtio/vhost-vdpa.c |  2 +-
>   util/hexdump.c         | 54 ++++++++++++++++++++++++------------------
>   3 files changed, 33 insertions(+), 26 deletions(-)
>
> diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
> index 3415f5d249..d0c5386e6c 100644
> --- a/include/qemu/cutils.h
> +++ b/include/qemu/cutils.h
> @@ -257,8 +257,7 @@ int parse_debug_env(const char *name, int max, int initial);
>    */
>   #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,
> -                       unsigned int len, bool ascii);
> +void qemu_hexdump_line(char *line, const void *bufptr, 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 b4afa142f6..32bad5ce68 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -946,7 +946,7 @@ static void vhost_vdpa_dump_config(struct vhost_dev *dev, 
> const uint8_t *config,
>   
>       for (b = 0; b < config_len; b += 16) {
>           len = config_len - b;
> -        qemu_hexdump_line(line, config + b, len, false);
> +        qemu_hexdump_line(line, config + b, len);
>           trace_vhost_vdpa_dump_config(dev, b, line);
>       }
>   }
> diff --git a/util/hexdump.c b/util/hexdump.c
> index 7324e7b126..dbc536fe84 100644
> --- a/util/hexdump.c
> +++ b/util/hexdump.c
> @@ -16,8 +16,7 @@
>   #include "qemu/osdep.h"
>   #include "qemu/cutils.h"
>   
> -void qemu_hexdump_line(char *line, const void *bufptr,
> -                       unsigned int len, bool ascii)
> +void qemu_hexdump_line(char *line, const void *bufptr, size_t len)
>   {
>       const char *buf = bufptr;
>       int i, c;

'c' ...

> @@ -26,39 +25,48 @@ void qemu_hexdump_line(char *line, const void *bufptr,
>           len = QEMU_HEXDUMP_LINE_BYTES;
>       }
>   
> -    for (i = 0; i < QEMU_HEXDUMP_LINE_BYTES; i++) {
> +    for (i = 0; i < len; i++) {
>           if (i != 0 && (i % 4) == 0) {
>               *line++ = ' ';
>           }
> -        if (i < len) {
> -            line += sprintf(line, " %02x", (unsigned char)buf[i]);
> -        } else {
> -            line += sprintf(line, "   ");
> -        }
> -    }
> -    if (ascii) {
> -        *line++ = ' ';
> -        for (i = 0; i < len; i++) {
> -            c = buf[i];
> -            if (c < ' ' || c > '~') {
> -                c = '.';
> -            }
> -            *line++ = c;

... is no more used;

> -        }
> +        line += sprintf(line, " %02x", (unsigned char)buf[i]);
>       }
>       *line = '\0';
>   }
>   
> +static void asciidump_line(char *line, const void *bufptr, size_t len)
> +{
> +    const char *buf = bufptr;
> +
> +    for (size_t i = 0; i < len; i++) {
> +        char c = buf[i];
> +
> +        if (c < ' ' || c > '~') {
> +            c = '.';
> +        }
> +        *line++ = c;
> +    }
> +    *line = '\0';
> +}
> +
> +#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)
>   {
> -    unsigned int b, len;
>       char line[QEMU_HEXDUMP_LINE_LEN];
> +    char ascii[QEMU_HEXDUMP_LINE_BYTES + 1];
> +    size_t b, len;
>   
> -    for (b = 0; b < size; b += QEMU_HEXDUMP_LINE_BYTES) {
> -        len = size - b;
> -        qemu_hexdump_line(line, bufptr + b, len, true);
> -        fprintf(fp, "%s: %04x: %s\n", prefix, b, line);
> +    for (b = 0; b < size; b += len) {
> +        len = MIN(size - b, QEMU_HEXDUMP_LINE_BYTES);
> +
> +        qemu_hexdump_line(line, bufptr + b, len);
> +        asciidump_line(ascii, bufptr + b, len);
> +
> +        fprintf(fp, "%s: %04x: %-*s %s\n",

'b' is size_t.

> +                prefix, b, QEMU_HEXDUMP_LINE_WIDTH, line, ascii);
>       }
>   
>   }

Queued squashing:

-- >8 --
diff --git a/util/hexdump.c b/util/hexdump.c
index dbc536fe84..0f943e31e5 100644
--- a/util/hexdump.c
+++ b/util/hexdump.c
@@ -19,7 +19,7 @@
 void qemu_hexdump_line(char *line, const void *bufptr, size_t len)
 {
     const char *buf = bufptr;
-    int i, c;
+    int i;

     if (len > QEMU_HEXDUMP_LINE_BYTES) {
         len = QEMU_HEXDUMP_LINE_BYTES;
@@ -65,7 +65,7 @@ void qemu_hexdump(FILE *fp, const char *prefix,
         qemu_hexdump_line(line, bufptr + b, len);
         asciidump_line(ascii, bufptr + b, len);

-        fprintf(fp, "%s: %04x: %-*s %s\n",
+        fprintf(fp, "%s: %04zx: %-*s %s\n",
                 prefix, b, QEMU_HEXDUMP_LINE_WIDTH, line, ascii);
     }

---