[PATCH v2 00/13] misc: Remove sprintf() due to macOS deprecation

[PATCH v2 00/13] misc: Remove sprintf() due to macOS deprecation

[Philippe Mathieu-Daudé]

Since v1:
- Use snprintf() in patches 1-5

Hi,

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience.

Suggestion to avoid the super-noisy warning on macOS forum are [*]:

* use -Wno-deprecated-declarations on the whole build
* surgically add #pragma clang diagnostic around each use.

None of these options seem reasonable, so we are somehow forced to
spend time converting each sprintf() call, even if they are safe
enough.

I'm so tired of seeing them than I started the conversion. This is
the first part. Help for the rest is welcomed.

Regards,

Phil.

[*] https://forums.developer.apple.com/forums/thread/714675

Philippe Mathieu-Daudé (13):
  ui/console-vc: Replace sprintf() by snprintf()
  hw/vfio/pci: Replace sprintf() by snprintf()
  hw/ppc/spapr: Replace sprintf() by snprintf()
  hw/mips/malta: Add re-usable rng_seed_hex_new() method
  hw/mips/malta: Replace sprintf() by snprintf()
  system/qtest: Replace sprintf() by g_string_append_printf()
  util/hexdump: Rename @offset argument in qemu_hexdump_line()
  util/hexdump: Have qemu_hexdump_line() return heap allocated buffer
  util/hexdump: Replace sprintf() by g_string_append_printf()
  hw/scsi/scsi-disk: Use qemu_hexdump_line() to avoid sprintf()
  hw/ide/atapi: Use qemu_hexdump_line() to avoid sprintf()
  hw/dma/pl330: Use qemu_hexdump_line() to avoid sprintf()
  backends/tpm: Use qemu_hexdump_line() to avoid sprintf()

 include/qemu/cutils.h   | 17 ++++++++++++++---
 backends/tpm/tpm_util.c | 24 ++++++++----------------
 hw/dma/pl330.c          | 12 +++---------
 hw/ide/atapi.c          |  8 ++------
 hw/mips/malta.c         | 23 ++++++++++++++---------
 hw/ppc/spapr.c          |  2 +-
 hw/scsi/scsi-disk.c     |  8 ++------
 hw/vfio/pci.c           |  2 +-
 hw/virtio/vhost-vdpa.c  | 11 ++++++-----
 system/qtest.c          |  8 +++-----
 ui/console-vc.c         |  2 +-
 util/hexdump.c          | 33 ++++++++++++++++++---------------
 12 files changed, 73 insertions(+), 77 deletions(-)

-- 
2.41.0

[PATCH v2 01/13] ui/console-vc: Replace sprintf() by snprintf()

[Philippe Mathieu-Daudé]

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience.

Replace sprintf() by snprintf() in order to avoid:

  [702/1310] Compiling C object libcommon.fa.p/ui_console-vc.c.o
  ui/console-vc.c:824:21: warning: 'sprintf' is deprecated:
    This function is provided for compatibility reasons only.
    Due to security concerns inherent in the design of sprintf(3),
    it is highly recommended that you use snprintf(3) instead.
    [-Wdeprecated-declarations]
                    sprintf(response, "\033[%d;%dR",
                    ^
  1 warning generated.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 ui/console-vc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ui/console-vc.c b/ui/console-vc.c
index 899fa11c94..847d5fb174 100644
--- a/ui/console-vc.c
+++ b/ui/console-vc.c
@@ -821,7 +821,7 @@ static void vc_putchar(VCChardev *vc, int ch)
                     break;
                 case 6:
                     /* report cursor position */
-                    sprintf(response, "\033[%d;%dR",
+                    snprintf(response, sizeof(response), "\033[%d;%dR",
                            (s->y_base + s->y) % s->total_height + 1,
                             s->x + 1);
                     vc_respond_str(vc, response);
-- 
2.41.0

Re: [PATCH v2 01/13] ui/console-vc: Replace sprintf() by snprintf()

[Marc-André Lureau]

On Thu, Apr 11, 2024 at 2:16 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
> resulting in painful developper experience.
>
> Replace sprintf() by snprintf() in order to avoid:
>
>   [702/1310] Compiling C object libcommon.fa.p/ui_console-vc.c.o
>   ui/console-vc.c:824:21: warning: 'sprintf' is deprecated:
>     This function is provided for compatibility reasons only.
>     Due to security concerns inherent in the design of sprintf(3),
>     it is highly recommended that you use snprintf(3) instead.
>     [-Wdeprecated-declarations]
>                     sprintf(response, "\033[%d;%dR",
>                     ^
>   1 warning generated.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  ui/console-vc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ui/console-vc.c b/ui/console-vc.c
> index 899fa11c94..847d5fb174 100644
> --- a/ui/console-vc.c
> +++ b/ui/console-vc.c
> @@ -821,7 +821,7 @@ static void vc_putchar(VCChardev *vc, int ch)
>                      break;
>                  case 6:
>                      /* report cursor position */
> -                    sprintf(response, "\033[%d;%dR",
> +                    snprintf(response, sizeof(response), "\033[%d;%dR",
>                             (s->y_base + s->y) % s->total_height + 1,
>                              s->x + 1);
>                      vc_respond_str(vc, response);
> --
> 2.41.0
>

Re: [PATCH v2 01/13] ui/console-vc: Replace sprintf() by snprintf()

[Richard Henderson]

On 4/11/24 03:15, Philippe Mathieu-Daudé wrote:
> sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
> resulting in painful developper experience.
> 
> Replace sprintf() by snprintf() in order to avoid:
> 
>    [702/1310] Compiling C object libcommon.fa.p/ui_console-vc.c.o
>    ui/console-vc.c:824:21: warning: 'sprintf' is deprecated:
>      This function is provided for compatibility reasons only.
>      Due to security concerns inherent in the design of sprintf(3),
>      it is highly recommended that you use snprintf(3) instead.
>      [-Wdeprecated-declarations]
>                      sprintf(response, "\033[%d;%dR",
>                      ^
>    1 warning generated.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   ui/console-vc.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH v2 02/13] hw/vfio/pci: Replace sprintf() by snprintf()

[Philippe Mathieu-Daudé]

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience. Use snprintf() instead.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/vfio/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 64780d1b79..cc545bcfbe 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2444,7 +2444,7 @@ bool vfio_pci_host_match(PCIHostDeviceAddress *addr, const char *name)
 {
     char tmp[13];
 
-    sprintf(tmp, "%04x:%02x:%02x.%1x", addr->domain,
+    snprintf(tmp, sizeof(tmp), "%04x:%02x:%02x.%1x", addr->domain,
             addr->bus, addr->slot, addr->function);
 
     return (strcmp(tmp, name) == 0);
-- 
2.41.0

Re: [PATCH v2 02/13] hw/vfio/pci: Replace sprintf() by snprintf()

[Richard Henderson]

On 4/11/24 03:15, Philippe Mathieu-Daudé wrote:
> sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
> resulting in painful developper experience. Use snprintf() instead.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/vfio/pci.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH v2 03/13] hw/ppc/spapr: Replace sprintf() by snprintf()

[Philippe Mathieu-Daudé]

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience.

Replace sprintf() by snprintf() in order to avoid:

  hw/ppc/spapr.c:385:5: warning: 'sprintf' is deprecated:
    This function is provided for compatibility reasons only.
    Due to security concerns inherent in the design of sprintf(3),
    it is highly recommended that you use snprintf(3) instead.
    [-Wdeprecated-declarations]
      sprintf(mem_name, "memory@%" HWADDR_PRIx, start);
      ^
  1 warning generated.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ppc/spapr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index e9bc97fee0..9e97992c79 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -382,7 +382,7 @@ static int spapr_dt_memory_node(SpaprMachineState *spapr, void *fdt, int nodeid,
     mem_reg_property[0] = cpu_to_be64(start);
     mem_reg_property[1] = cpu_to_be64(size);
 
-    sprintf(mem_name, "memory@%" HWADDR_PRIx, start);
+    snprintf(mem_name, sizeof(mem_name), "memory@%" HWADDR_PRIx, start);
     off = fdt_add_subnode(fdt, 0, mem_name);
     _FDT(off);
     _FDT((fdt_setprop_string(fdt, off, "device_type", "memory")));
-- 
2.41.0

Re: [PATCH v2 03/13] hw/ppc/spapr: Replace sprintf() by snprintf()

[Richard Henderson]

On 4/11/24 03:15, Philippe Mathieu-Daudé wrote:
> sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
> resulting in painful developper experience.
> 
> Replace sprintf() by snprintf() in order to avoid:
> 
>    hw/ppc/spapr.c:385:5: warning: 'sprintf' is deprecated:
>      This function is provided for compatibility reasons only.
>      Due to security concerns inherent in the design of sprintf(3),
>      it is highly recommended that you use snprintf(3) instead.
>      [-Wdeprecated-declarations]
>        sprintf(mem_name, "memory@%" HWADDR_PRIx, start);
>        ^
>    1 warning generated.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/ppc/spapr.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 03/13] hw/ppc/spapr: Replace sprintf() by snprintf()

[Harsh Prateek Bora]

On 4/11/24 15:45, Philippe Mathieu-Daudé wrote:
> sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
> resulting in painful developper experience.

s/developper/developer ?

> 
> Replace sprintf() by snprintf() in order to avoid:
> 
>    hw/ppc/spapr.c:385:5: warning: 'sprintf' is deprecated:
>      This function is provided for compatibility reasons only.
>      Due to security concerns inherent in the design of sprintf(3),
>      it is highly recommended that you use snprintf(3) instead.
>      [-Wdeprecated-declarations]
>        sprintf(mem_name, "memory@%" HWADDR_PRIx, start);
>        ^
>    1 warning generated.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

With the typo fixed,

Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

> ---
>   hw/ppc/spapr.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index e9bc97fee0..9e97992c79 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -382,7 +382,7 @@ static int spapr_dt_memory_node(SpaprMachineState *spapr, void *fdt, int nodeid,
>       mem_reg_property[0] = cpu_to_be64(start);
>       mem_reg_property[1] = cpu_to_be64(size);
>   
> -    sprintf(mem_name, "memory@%" HWADDR_PRIx, start);
> +    snprintf(mem_name, sizeof(mem_name), "memory@%" HWADDR_PRIx, start);
>       off = fdt_add_subnode(fdt, 0, mem_name);
>       _FDT(off);
>       _FDT((fdt_setprop_string(fdt, off, "device_type", "memory")));

[PATCH v2 04/13] hw/mips/malta: Add re-usable rng_seed_hex_new() method

[Philippe Mathieu-Daudé]

Extract common code from reinitialize_rng_seed() and
load_kernel() to rng_seed_hex_new().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/mips/malta.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index af74008c82..9fc6a7d313 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -850,15 +850,24 @@ static void G_GNUC_PRINTF(3, 4) prom_set(uint32_t *prom_buf, int index,
     va_end(ap);
 }
 
-static void reinitialize_rng_seed(void *opaque)
+static char *rng_seed_hex_new(void)
 {
-    char *rng_seed_hex = opaque;
     uint8_t rng_seed[32];
+    char rng_seed_hex[sizeof(rng_seed) * 2 + 1];
 
     qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
     for (size_t i = 0; i < sizeof(rng_seed); ++i) {
         sprintf(rng_seed_hex + i * 2, "%02x", rng_seed[i]);
     }
+
+    return g_strdup(rng_seed_hex);
+}
+
+static void reinitialize_rng_seed(void *opaque)
+{
+    g_autofree char *rng_seed_hex = rng_seed_hex_new();
+
+    strcpy(opaque, rng_seed_hex);
 }
 
 /* Kernel */
@@ -870,8 +879,7 @@ static uint64_t load_kernel(void)
     uint32_t *prom_buf;
     long prom_size;
     int prom_index = 0;
-    uint8_t rng_seed[32];
-    char rng_seed_hex[sizeof(rng_seed) * 2 + 1];
+    g_autofree char *rng_seed_hex = rng_seed_hex_new();
     size_t rng_seed_prom_offset;
 
     kernel_size = load_elf(loaderparams.kernel_filename, NULL,
@@ -946,10 +954,6 @@ static uint64_t load_kernel(void)
     prom_set(prom_buf, prom_index++, "modetty0");
     prom_set(prom_buf, prom_index++, "38400n8r");
 
-    qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
-    for (size_t i = 0; i < sizeof(rng_seed); ++i) {
-        sprintf(rng_seed_hex + i * 2, "%02x", rng_seed[i]);
-    }
     prom_set(prom_buf, prom_index++, "rngseed");
     rng_seed_prom_offset = prom_index * ENVP_ENTRY_SIZE +
                            sizeof(uint32_t) * ENVP_NB_ENTRIES;
-- 
2.41.0

Re: [PATCH v2 04/13] hw/mips/malta: Add re-usable rng_seed_hex_new() method

[Richard Henderson]

On 4/11/24 03:15, Philippe Mathieu-Daudé wrote:
> Extract common code from reinitialize_rng_seed() and
> load_kernel() to rng_seed_hex_new().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/mips/malta.c | 20 ++++++++++++--------
>   1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/mips/malta.c b/hw/mips/malta.c
> index af74008c82..9fc6a7d313 100644
> --- a/hw/mips/malta.c
> +++ b/hw/mips/malta.c
> @@ -850,15 +850,24 @@ static void G_GNUC_PRINTF(3, 4) prom_set(uint32_t *prom_buf, int index,
>       va_end(ap);
>   }
>   
> -static void reinitialize_rng_seed(void *opaque)
> +static char *rng_seed_hex_new(void)
>   {
> -    char *rng_seed_hex = opaque;
>       uint8_t rng_seed[32];
> +    char rng_seed_hex[sizeof(rng_seed) * 2 + 1];
>   
>       qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
>       for (size_t i = 0; i < sizeof(rng_seed); ++i) {
>           sprintf(rng_seed_hex + i * 2, "%02x", rng_seed[i]);
>       }
> +
> +    return g_strdup(rng_seed_hex);
> +}
> +
> +static void reinitialize_rng_seed(void *opaque)
> +{
> +    g_autofree char *rng_seed_hex = rng_seed_hex_new();
> +
> +    strcpy(opaque, rng_seed_hex);
>   }

Though it isn't deprecated, strcpy isn't really any safer than sprintf.
We don't need to be copying text around quite as much as this.

How about:

#define RNG_SEED_SIZE 32

static void rng_seed_hex_new(char buf[2 * RNG_SEED_SIZE + 1])
{
     static const char hex = "0123456789abcdef";
     uint8_t rng_seed[RNG_SEED_SIZE];

     qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
     for (int i = 0; i < RNG_SEED_SIZE; ++i) {
         buf[i * 2 + 0] = hex[rng_seed[i] / 16];
         buf[i * 2 + 1] = hex[rng_seed[i] % 16];
     }
     buf[RNG_SEED_SIZE * 2] = '\0';
}

static void reinitialize_rng_seed(void *opaque)
{
     rng_seed_hex_new(opaque);
}

with little change in load_kernel.


r~

Re: [PATCH v2 04/13] hw/mips/malta: Add re-usable rng_seed_hex_new() method

[Richard Henderson]

On 4/11/24 13:07, Richard Henderson wrote:
> On 4/11/24 03:15, Philippe Mathieu-Daudé wrote:
>> Extract common code from reinitialize_rng_seed() and
>> load_kernel() to rng_seed_hex_new().
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/mips/malta.c | 20 ++++++++++++--------
>>   1 file changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/mips/malta.c b/hw/mips/malta.c
>> index af74008c82..9fc6a7d313 100644
>> --- a/hw/mips/malta.c
>> +++ b/hw/mips/malta.c
>> @@ -850,15 +850,24 @@ static void G_GNUC_PRINTF(3, 4) prom_set(uint32_t *prom_buf, int 
>> index,
>>       va_end(ap);
>>   }
>> -static void reinitialize_rng_seed(void *opaque)
>> +static char *rng_seed_hex_new(void)
>>   {
>> -    char *rng_seed_hex = opaque;
>>       uint8_t rng_seed[32];
>> +    char rng_seed_hex[sizeof(rng_seed) * 2 + 1];
>>       qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
>>       for (size_t i = 0; i < sizeof(rng_seed); ++i) {
>>           sprintf(rng_seed_hex + i * 2, "%02x", rng_seed[i]);
>>       }
>> +
>> +    return g_strdup(rng_seed_hex);
>> +}
>> +
>> +static void reinitialize_rng_seed(void *opaque)
>> +{
>> +    g_autofree char *rng_seed_hex = rng_seed_hex_new();
>> +
>> +    strcpy(opaque, rng_seed_hex);
>>   }
> 
> Though it isn't deprecated, strcpy isn't really any safer than sprintf.
> We don't need to be copying text around quite as much as this.
> 
> How about:
> 
> #define RNG_SEED_SIZE 32
> 
> static void rng_seed_hex_new(char buf[2 * RNG_SEED_SIZE + 1])
> {
>      static const char hex = "0123456789abcdef";
>      uint8_t rng_seed[RNG_SEED_SIZE];
> 
>      qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
>      for (int i = 0; i < RNG_SEED_SIZE; ++i) {
>          buf[i * 2 + 0] = hex[rng_seed[i] / 16];
>          buf[i * 2 + 1] = hex[rng_seed[i] % 16];

Hmm.  Maybe a

static inline char hexdump_nibble(unsigned val)
{
     return (val < 10 ? '0' : 'a') + val;
}

static inline void hexdump_byte(char *out, uint8_t byte)
{
     out[0] = hexdump_nibble(byte >> 4);
     out[1] = hexdump_nibble(byte & 15);
}

in "qemu/cutils.h", for use elsewhere including util/hexdump.c.


r~

[PATCH v2 05/13] hw/mips/malta: Replace sprintf() by snprintf()

[Philippe Mathieu-Daudé]

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience.

Replace sprintf() by snprintf() in order to avoid:

  [120/169] Compiling C object libcommon.fa.p/system_qtest.c.o
  hw/mips/malta.c:860:9: warning: 'sprintf' is deprecated:
          sprintf(rng_seed_hex + i * 2, "%02x", rng_seed[i]);
          ^
    This function is provided for compatibility reasons only.
    Due to security concerns inherent in the design of sprintf(3),
    it is highly recommended that you use snprintf(3) instead.
    [-Wdeprecated-declarations]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/mips/malta.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index 9fc6a7d313..5d33aa5123 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -857,7 +857,8 @@ static char *rng_seed_hex_new(void)
 
     qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
     for (size_t i = 0; i < sizeof(rng_seed); ++i) {
-        sprintf(rng_seed_hex + i * 2, "%02x", rng_seed[i]);
+        snprintf(rng_seed_hex + i * 2, sizeof(rng_seed_hex) - i * 2,
+                 "%02x", rng_seed[i]);
     }
 
     return g_strdup(rng_seed_hex);
-- 
2.41.0

[PATCH v2 06/13] system/qtest: Replace sprintf() by g_string_append_printf()

[Philippe Mathieu-Daudé]

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience.

Replace sprintf() by GString API uses in order to avoid:

  [120/169] Compiling C object libcommon.fa.p/system_qtest.c.o
  system/qtest.c:623:13: warning: 'sprintf' is deprecated:
    This function is provided for compatibility reasons only.
    Due to security concerns inherent in the design of sprintf(3),
    it is highly recommended that you use snprintf(3) instead.
    [-Wdeprecated-declarations]
            sprintf(&enc[i * 2], "%02x", data[i]);
            ^
  1 warning generated.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 system/qtest.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/system/qtest.c b/system/qtest.c
index 6da58b3874..22bf1a33dc 100644
--- a/system/qtest.c
+++ b/system/qtest.c
@@ -601,9 +601,9 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
         qtest_send_prefix(chr);
         qtest_sendf(chr, "OK 0x%016" PRIx64 "\n", value);
     } else if (strcmp(words[0], "read") == 0) {
+        g_autoptr(GString) enc = g_string_new("");
         uint64_t addr, len, i;
         uint8_t *data;
-        char *enc;
         int ret;
 
         g_assert(words[1] && words[2]);
@@ -618,16 +618,14 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
         address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
                            len);
 
-        enc = g_malloc(2 * len + 1);
         for (i = 0; i < len; i++) {
-            sprintf(&enc[i * 2], "%02x", data[i]);
+            g_string_append_printf(enc, "%02x", data[i]);
         }
 
         qtest_send_prefix(chr);
-        qtest_sendf(chr, "OK 0x%s\n", enc);
+        qtest_sendf(chr, "OK 0x%s\n", enc->str);
 
         g_free(data);
-        g_free(enc);
     } else if (strcmp(words[0], "b64read") == 0) {
         uint64_t addr, len;
         uint8_t *data;
-- 
2.41.0

Re: [PATCH v2 06/13] system/qtest: Replace sprintf() by g_string_append_printf()

[Richard Henderson]

On 4/11/24 03:15, Philippe Mathieu-Daudé wrote:
> sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
> resulting in painful developper experience.
> 
> Replace sprintf() by GString API uses in order to avoid:
> 
>    [120/169] Compiling C object libcommon.fa.p/system_qtest.c.o
>    system/qtest.c:623:13: warning: 'sprintf' is deprecated:
>      This function is provided for compatibility reasons only.
>      Due to security concerns inherent in the design of sprintf(3),
>      it is highly recommended that you use snprintf(3) instead.
>      [-Wdeprecated-declarations]
>              sprintf(&enc[i * 2], "%02x", data[i]);
>              ^
>    1 warning generated.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> Reviewed-by: Thomas Huth<thuth@redhat.com>
> ---
>   system/qtest.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH v2 07/13] util/hexdump: Rename @offset argument in qemu_hexdump_line()

[Philippe Mathieu-Daudé]

@offset argument is more descriptive than @b.

Inverse @bufptr <-> @offset arguments order.

Document qemu_hexdump_line().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/qemu/cutils.h  | 11 +++++++++--
 hw/virtio/vhost-vdpa.c |  8 ++++----
 util/hexdump.c         | 16 ++++++++--------
 3 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index 92c927a6a3..70ca4b876b 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -252,12 +252,19 @@ static inline const char *yes_no(bool b)
  */
 int parse_debug_env(const char *name, int max, int initial);
 
-/*
+/**
+ * qemu_hexdump_line:
+ * @line: Buffer to be filled by the hexadecimal/ASCII dump
+ * @bufptr: Buffer to dump
+ * @offset: Offset within @bufptr to start the dump
+ * @len: Length of the bytes do dump
+ * @ascii: Replace non-ASCII characters by the dot symbol
+ *
  * Hexdump a line of a byte buffer into a hexadecimal/ASCII buffer
  */
 #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, unsigned int b, const void *bufptr,
+void qemu_hexdump_line(char *line, const void *bufptr, unsigned offset,
                        unsigned int len, bool ascii);
 
 /*
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index e827b9175f..cf7cfa3f16 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -941,12 +941,12 @@ 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;
+    int ofs, len;
     char line[QEMU_HEXDUMP_LINE_LEN];
 
-    for (b = 0; b < config_len; b += 16) {
-        len = config_len - b;
-        qemu_hexdump_line(line, b, config, len, false);
+    for (ofs = 0; ofs < config_len; ofs += 16) {
+        len = config_len - ofs;
+        qemu_hexdump_line(line, config, ofs, len, false);
         trace_vhost_vdpa_dump_config(dev, line);
     }
 }
diff --git a/util/hexdump.c b/util/hexdump.c
index 9921114b3c..469083d8c0 100644
--- a/util/hexdump.c
+++ b/util/hexdump.c
@@ -16,7 +16,7 @@
 #include "qemu/osdep.h"
 #include "qemu/cutils.h"
 
-void qemu_hexdump_line(char *line, unsigned int b, const void *bufptr,
+void qemu_hexdump_line(char *line, const void *bufptr, unsigned offset,
                        unsigned int len, bool ascii)
 {
     const char *buf = bufptr;
@@ -26,13 +26,13 @@ void qemu_hexdump_line(char *line, unsigned int b, const void *bufptr,
         len = QEMU_HEXDUMP_LINE_BYTES;
     }
 
-    line += snprintf(line, 6, "%04x:", b);
+    line += snprintf(line, 6, "%04x:", offset);
     for (i = 0; i < QEMU_HEXDUMP_LINE_BYTES; i++) {
         if ((i % 4) == 0) {
             *line++ = ' ';
         }
         if (i < len) {
-            line += sprintf(line, " %02x", (unsigned char)buf[b + i]);
+            line += sprintf(line, " %02x", (unsigned char)buf[offset + i]);
         } else {
             line += sprintf(line, "   ");
         }
@@ -40,7 +40,7 @@ void qemu_hexdump_line(char *line, unsigned int b, const void *bufptr,
     if (ascii) {
         *line++ = ' ';
         for (i = 0; i < len; i++) {
-            c = buf[b + i];
+            c = buf[offset + i];
             if (c < ' ' || c > '~') {
                 c = '.';
             }
@@ -53,12 +53,12 @@ void qemu_hexdump_line(char *line, unsigned int b, const void *bufptr,
 void qemu_hexdump(FILE *fp, const char *prefix,
                   const void *bufptr, size_t size)
 {
-    unsigned int b, len;
+    unsigned int ofs, len;
     char line[QEMU_HEXDUMP_LINE_LEN];
 
-    for (b = 0; b < size; b += QEMU_HEXDUMP_LINE_BYTES) {
-        len = size - b;
-        qemu_hexdump_line(line, b, bufptr, len, true);
+    for (ofs = 0; ofs < size; ofs += QEMU_HEXDUMP_LINE_BYTES) {
+        len = size - ofs;
+        qemu_hexdump_line(line, bufptr, ofs, len, true);
         fprintf(fp, "%s: %s\n", prefix, line);
     }
 
-- 
2.41.0

Re: [PATCH v2 07/13] util/hexdump: Rename @offset argument in qemu_hexdump_line()

[Richard Henderson]

On 4/11/24 03:15, Philippe Mathieu-Daudé wrote:
> @offset argument is more descriptive than @b.
> 
> Inverse @bufptr <-> @offset arguments order.
> 
> Document qemu_hexdump_line().
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   include/qemu/cutils.h  | 11 +++++++++--
>   hw/virtio/vhost-vdpa.c |  8 ++++----
>   util/hexdump.c         | 16 ++++++++--------
>   3 files changed, 21 insertions(+), 14 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH v2 08/13] util/hexdump: Have qemu_hexdump_line() return heap allocated buffer

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/qemu/cutils.h  | 10 +++++++---
 hw/virtio/vhost-vdpa.c |  5 +++--
 util/hexdump.c         | 12 ++++++++----
 3 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index 70ca4b876b..e8d6b86098 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -254,18 +254,22 @@ int parse_debug_env(const char *name, int max, int initial);
 
 /**
  * qemu_hexdump_line:
- * @line: Buffer to be filled by the hexadecimal/ASCII dump
  * @bufptr: Buffer to dump
  * @offset: Offset within @bufptr to start the dump
  * @len: Length of the bytes do dump
  * @ascii: Replace non-ASCII characters by the dot symbol
  *
  * Hexdump a line of a byte buffer into a hexadecimal/ASCII buffer
+ *
+ * The caller must use g_free() to free the returned data when it is
+ * no longer required.
+ *
+ * Returns: Hexadecimal/ASCII dump
  */
 #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 offset,
-                       unsigned int len, bool ascii);
+char *qemu_hexdump_line(const void *bufptr, unsigned offset,
+                        unsigned int len, bool ascii);
 
 /*
  * 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 cf7cfa3f16..e61af86d9d 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -942,12 +942,13 @@ static void vhost_vdpa_dump_config(struct vhost_dev *dev, const uint8_t *config,
                                    uint32_t config_len)
 {
     int ofs, len;
-    char line[QEMU_HEXDUMP_LINE_LEN];
+    char *line;
 
     for (ofs = 0; ofs < config_len; ofs += 16) {
         len = config_len - ofs;
-        qemu_hexdump_line(line, config, ofs, len, false);
+        line = qemu_hexdump_line(config, ofs, len, false);
         trace_vhost_vdpa_dump_config(dev, line);
+        g_free(line);
     }
 }
 
diff --git a/util/hexdump.c b/util/hexdump.c
index 469083d8c0..b6f70e93bb 100644
--- a/util/hexdump.c
+++ b/util/hexdump.c
@@ -16,9 +16,10 @@
 #include "qemu/osdep.h"
 #include "qemu/cutils.h"
 
-void qemu_hexdump_line(char *line, const void *bufptr, unsigned offset,
-                       unsigned int len, bool ascii)
+char *qemu_hexdump_line(const void *bufptr, unsigned offset,
+                        unsigned int len, bool ascii)
 {
+    char linebuf[QEMU_HEXDUMP_LINE_BYTES], *line = linebuf;
     const char *buf = bufptr;
     int i, c;
 
@@ -48,18 +49,21 @@ void qemu_hexdump_line(char *line, const void *bufptr, unsigned offset,
         }
     }
     *line = '\0';
+
+    return g_strdup(linebuf);
 }
 
 void qemu_hexdump(FILE *fp, const char *prefix,
                   const void *bufptr, size_t size)
 {
     unsigned int ofs, len;
-    char line[QEMU_HEXDUMP_LINE_LEN];
+    char *line;
 
     for (ofs = 0; ofs < size; ofs += QEMU_HEXDUMP_LINE_BYTES) {
         len = size - ofs;
-        qemu_hexdump_line(line, bufptr, ofs, len, true);
+        line = qemu_hexdump_line(bufptr, ofs, len, true);
         fprintf(fp, "%s: %s\n", prefix, line);
+        g_free(line);
     }
 
 }
-- 
2.41.0

Re: [PATCH v2 08/13] util/hexdump: Have qemu_hexdump_line() return heap allocated buffer

[Richard Henderson]

On 4/11/24 03:15, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/qemu/cutils.h  | 10 +++++++---
>   hw/virtio/vhost-vdpa.c |  5 +++--
>   util/hexdump.c         | 12 ++++++++----
>   3 files changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
> index 70ca4b876b..e8d6b86098 100644
> --- a/include/qemu/cutils.h
> +++ b/include/qemu/cutils.h
> @@ -254,18 +254,22 @@ int parse_debug_env(const char *name, int max, int initial);
>   
>   /**
>    * qemu_hexdump_line:
> - * @line: Buffer to be filled by the hexadecimal/ASCII dump
>    * @bufptr: Buffer to dump
>    * @offset: Offset within @bufptr to start the dump
>    * @len: Length of the bytes do dump
>    * @ascii: Replace non-ASCII characters by the dot symbol
>    *
>    * Hexdump a line of a byte buffer into a hexadecimal/ASCII buffer
> + *
> + * The caller must use g_free() to free the returned data when it is
> + * no longer required.
> + *
> + * Returns: Hexadecimal/ASCII dump
>    */
>   #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 offset,
> -                       unsigned int len, bool ascii);
> +char *qemu_hexdump_line(const void *bufptr, unsigned offset,
> +                        unsigned int len, bool ascii);
>   
>   /*
>    * 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 cf7cfa3f16..e61af86d9d 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -942,12 +942,13 @@ static void vhost_vdpa_dump_config(struct vhost_dev *dev, const uint8_t *config,
>                                      uint32_t config_len)
>   {
>       int ofs, len;
> -    char line[QEMU_HEXDUMP_LINE_LEN];
> +    char *line;
>   
>       for (ofs = 0; ofs < config_len; ofs += 16) {
>           len = config_len - ofs;
> -        qemu_hexdump_line(line, config, ofs, len, false);
> +        line = qemu_hexdump_line(config, ofs, len, false);
>           trace_vhost_vdpa_dump_config(dev, line);
> +        g_free(line);
>       }
>   }
>   
> diff --git a/util/hexdump.c b/util/hexdump.c
> index 469083d8c0..b6f70e93bb 100644
> --- a/util/hexdump.c
> +++ b/util/hexdump.c
> @@ -16,9 +16,10 @@
>   #include "qemu/osdep.h"
>   #include "qemu/cutils.h"
>   
> -void qemu_hexdump_line(char *line, const void *bufptr, unsigned offset,
> -                       unsigned int len, bool ascii)
> +char *qemu_hexdump_line(const void *bufptr, unsigned offset,
> +                        unsigned int len, bool ascii)
>   {
> +    char linebuf[QEMU_HEXDUMP_LINE_BYTES], *line = linebuf;
>       const char *buf = bufptr;
>       int i, c;
>   
> @@ -48,18 +49,21 @@ void qemu_hexdump_line(char *line, const void *bufptr, unsigned offset,
>           }
>       }
>       *line = '\0';
> +
> +    return g_strdup(linebuf);
>   }
>   
>   void qemu_hexdump(FILE *fp, const char *prefix,
>                     const void *bufptr, size_t size)
>   {
>       unsigned int ofs, len;
> -    char line[QEMU_HEXDUMP_LINE_LEN];
> +    char *line;
>   
>       for (ofs = 0; ofs < size; ofs += QEMU_HEXDUMP_LINE_BYTES) {
>           len = size - ofs;
> -        qemu_hexdump_line(line, bufptr, ofs, len, true);
> +        line = qemu_hexdump_line(bufptr, ofs, len, true);
>           fprintf(fp, "%s: %s\n", prefix, line);
> +        g_free(line);
>       }
>   
>   }

Not especially efficient, re-allocating for each line.

How about

GString *qemu_hexdump_line(GString *str, buf, offset, len, ascii)
{
     if (str) {
         g_string_truncate(str, 0);
     } else {
         str = g_string_sized_new(QEMU_HEXDUMP_LINE_LEN);
     }
     ...
     return str;
}

void qemu_hexdump(FILE *fp, ...)
{
     g_autoptr(GString) str = g_string_sized_new(QEMU_HEXDUMP_LINE_LEN);

     for (...) {
         qemu_hexdump_line(str, ...);
         fprintf(fp, "%s: %s\n", prefix, str->str);
     }
}

So that we reuse the one allocation across the whole loop.

r~

[PATCH v2 09/13] util/hexdump: Replace sprintf() by g_string_append_printf()

[Philippe Mathieu-Daudé]

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience.

Replace sprintf() by GString API in order to avoid:

  [426/1310] Compiling C object libqemuutil.a.p/util_hexdump.c.o
  util/hexdump.c:35:21: warning: 'sprintf' is deprecated:
    This function is provided for compatibility reasons only.
    Due to security concerns inherent in the design of sprintf(3),
    it is highly recommended that you use snprintf(3) instead.
    [-Wdeprecated-declarations]
            line += sprintf(line, " %02x", (unsigned char)buf[b + i]);
                    ^
  util/hexdump.c:37:21: warning: 'sprintf' is deprecated:
            line += sprintf(line, "   ");
                    ^
  2 warnings generated.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 util/hexdump.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/util/hexdump.c b/util/hexdump.c
index b6f70e93bb..2ec1171de3 100644
--- a/util/hexdump.c
+++ b/util/hexdump.c
@@ -19,7 +19,7 @@
 char *qemu_hexdump_line(const void *bufptr, unsigned offset,
                         unsigned int len, bool ascii)
 {
-    char linebuf[QEMU_HEXDUMP_LINE_BYTES], *line = linebuf;
+    g_autoptr(GString) gs = g_string_sized_new(QEMU_HEXDUMP_LINE_BYTES);
     const char *buf = bufptr;
     int i, c;
 
@@ -27,30 +27,29 @@ char *qemu_hexdump_line(const void *bufptr, unsigned offset,
         len = QEMU_HEXDUMP_LINE_BYTES;
     }
 
-    line += snprintf(line, 6, "%04x:", offset);
+    g_string_append_printf(gs, "%04x:", offset);
     for (i = 0; i < QEMU_HEXDUMP_LINE_BYTES; i++) {
         if ((i % 4) == 0) {
-            *line++ = ' ';
+            g_string_append_c(gs, ' ');
         }
         if (i < len) {
-            line += sprintf(line, " %02x", (unsigned char)buf[offset + i]);
+            g_string_append_printf(gs, " %02x", (unsigned char)buf[offset + i]);
         } else {
-            line += sprintf(line, "   ");
+            g_string_append(gs, "   ");
         }
     }
     if (ascii) {
-        *line++ = ' ';
+        g_string_append_c(gs, ' ');
         for (i = 0; i < len; i++) {
             c = buf[offset + i];
             if (c < ' ' || c > '~') {
                 c = '.';
             }
-            *line++ = c;
+            g_string_append_c(gs, c);
         }
     }
-    *line = '\0';
 
-    return g_strdup(linebuf);
+    return g_strdup(gs->str);
 }
 
 void qemu_hexdump(FILE *fp, const char *prefix,
-- 
2.41.0

Re: [PATCH v2 09/13] util/hexdump: Replace sprintf() by g_string_append_printf()

[Philippe Mathieu-Daudé]

On 11/4/24 12:15, Philippe Mathieu-Daudé wrote:
> sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
> resulting in painful developper experience.
> 
> Replace sprintf() by GString API in order to avoid:
> 
>    [426/1310] Compiling C object libqemuutil.a.p/util_hexdump.c.o
>    util/hexdump.c:35:21: warning: 'sprintf' is deprecated:
>      This function is provided for compatibility reasons only.
>      Due to security concerns inherent in the design of sprintf(3),
>      it is highly recommended that you use snprintf(3) instead.
>      [-Wdeprecated-declarations]
>              line += sprintf(line, " %02x", (unsigned char)buf[b + i]);
>                      ^
>    util/hexdump.c:37:21: warning: 'sprintf' is deprecated:
>              line += sprintf(line, "   ");
>                      ^
>    2 warnings generated.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   util/hexdump.c | 17 ++++++++---------
>   1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/util/hexdump.c b/util/hexdump.c
> index b6f70e93bb..2ec1171de3 100644
> --- a/util/hexdump.c
> +++ b/util/hexdump.c
> @@ -19,7 +19,7 @@
>   char *qemu_hexdump_line(const void *bufptr, unsigned offset,
>                           unsigned int len, bool ascii)
>   {
> -    char linebuf[QEMU_HEXDUMP_LINE_BYTES], *line = linebuf;
> +    g_autoptr(GString) gs = g_string_sized_new(QEMU_HEXDUMP_LINE_BYTES);
>       const char *buf = bufptr;
>       int i, c;
>   
> @@ -27,30 +27,29 @@ char *qemu_hexdump_line(const void *bufptr, unsigned offset,
>           len = QEMU_HEXDUMP_LINE_BYTES;
>       }
>   
> -    line += snprintf(line, 6, "%04x:", offset);
> +    g_string_append_printf(gs, "%04x:", offset);
>       for (i = 0; i < QEMU_HEXDUMP_LINE_BYTES; i++) {
>           if ((i % 4) == 0) {
> -            *line++ = ' ';
> +            g_string_append_c(gs, ' ');
>           }
>           if (i < len) {
> -            line += sprintf(line, " %02x", (unsigned char)buf[offset + i]);
> +            g_string_append_printf(gs, " %02x", (unsigned char)buf[offset + i]);

I find using g_string_append_printf() simpler than checking snprintf()
return value, and don't expect this function to be in hot path, but if
preferred I can try to not use the GString API.

>           } else {
> -            line += sprintf(line, "   ");
> +            g_string_append(gs, "   ");
>           }
>       }

Re: [PATCH v2 09/13] util/hexdump: Replace sprintf() by g_string_append_printf()

[Richard Henderson]

On 4/11/24 13:43, Philippe Mathieu-Daudé wrote:
> On 11/4/24 12:15, Philippe Mathieu-Daudé wrote:
>> sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
>> resulting in painful developper experience.
>>
>> Replace sprintf() by GString API in order to avoid:
>>
>>    [426/1310] Compiling C object libqemuutil.a.p/util_hexdump.c.o
>>    util/hexdump.c:35:21: warning: 'sprintf' is deprecated:
>>      This function is provided for compatibility reasons only.
>>      Due to security concerns inherent in the design of sprintf(3),
>>      it is highly recommended that you use snprintf(3) instead.
>>      [-Wdeprecated-declarations]
>>              line += sprintf(line, " %02x", (unsigned char)buf[b + i]);
>>                      ^
>>    util/hexdump.c:37:21: warning: 'sprintf' is deprecated:
>>              line += sprintf(line, "   ");
>>                      ^
>>    2 warnings generated.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   util/hexdump.c | 17 ++++++++---------
>>   1 file changed, 8 insertions(+), 9 deletions(-)
>>
>> diff --git a/util/hexdump.c b/util/hexdump.c
>> index b6f70e93bb..2ec1171de3 100644
>> --- a/util/hexdump.c
>> +++ b/util/hexdump.c
>> @@ -19,7 +19,7 @@
>>   char *qemu_hexdump_line(const void *bufptr, unsigned offset,
>>                           unsigned int len, bool ascii)
>>   {
>> -    char linebuf[QEMU_HEXDUMP_LINE_BYTES], *line = linebuf;
>> +    g_autoptr(GString) gs = g_string_sized_new(QEMU_HEXDUMP_LINE_BYTES);
>>       const char *buf = bufptr;
>>       int i, c;
>> @@ -27,30 +27,29 @@ char *qemu_hexdump_line(const void *bufptr, unsigned offset,
>>           len = QEMU_HEXDUMP_LINE_BYTES;
>>       }
>> -    line += snprintf(line, 6, "%04x:", offset);
>> +    g_string_append_printf(gs, "%04x:", offset);
>>       for (i = 0; i < QEMU_HEXDUMP_LINE_BYTES; i++) {
>>           if ((i % 4) == 0) {
>> -            *line++ = ' ';
>> +            g_string_append_c(gs, ' ');
>>           }
>>           if (i < len) {
>> -            line += sprintf(line, " %02x", (unsigned char)buf[offset + i]);
>> +            g_string_append_printf(gs, " %02x", (unsigned char)buf[offset + i]);
> 
> I find using g_string_append_printf() simpler than checking snprintf()
> return value, and don't expect this function to be in hot path, but if
> preferred I can try to not use the GString API.

GString api is pretty good.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[PATCH v2 10/13] hw/scsi/scsi-disk: Use qemu_hexdump_line() to avoid sprintf()

[Philippe Mathieu-Daudé]

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience.

Use qemu_hexdump_line() to avoid sprintf() calls, silencing:

  [105/169] Compiling C object libcommon.fa.p/hw_scsi_scsi-disk.c.o
  hw/scsi/scsi-disk.c:2659:14: warning: 'sprintf' is deprecated:
    This function is provided for compatibility reasons only.
    Due to security concerns inherent in the design of sprintf(3),
    it is highly recommended that you use snprintf(3) instead.
    [-Wdeprecated-declarations]
        p += sprintf(p, " 0x%02x", buf[i]);
             ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/scsi/scsi-disk.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 4bd7af9d0c..4f914df5c2 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2648,16 +2648,12 @@ static const SCSIReqOps *const scsi_disk_reqops_dispatch[256] = {
 
 static void scsi_disk_new_request_dump(uint32_t lun, uint32_t tag, uint8_t *buf)
 {
-    int i;
     int len = scsi_cdb_length(buf);
-    char *line_buffer, *p;
+    char *line_buffer;
 
     assert(len > 0 && len <= 16);
-    line_buffer = g_malloc(len * 5 + 1);
 
-    for (i = 0, p = line_buffer; i < len; i++) {
-        p += sprintf(p, " 0x%02x", buf[i]);
-    }
+    line_buffer = qemu_hexdump_line(buf, 0, len, false);
     trace_scsi_disk_new_request(lun, tag, line_buffer);
 
     g_free(line_buffer);
-- 
2.41.0

Re: [PATCH v2 10/13] hw/scsi/scsi-disk: Use qemu_hexdump_line() to avoid sprintf()

[Richard Henderson]

On 4/11/24 03:15, Philippe Mathieu-Daudé wrote:
> sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
> resulting in painful developper experience.
> 
> Use qemu_hexdump_line() to avoid sprintf() calls, silencing:
> 
>    [105/169] Compiling C object libcommon.fa.p/hw_scsi_scsi-disk.c.o
>    hw/scsi/scsi-disk.c:2659:14: warning: 'sprintf' is deprecated:
>      This function is provided for compatibility reasons only.
>      Due to security concerns inherent in the design of sprintf(3),
>      it is highly recommended that you use snprintf(3) instead.
>      [-Wdeprecated-declarations]
>          p += sprintf(p, " 0x%02x", buf[i]);
>               ^
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/scsi/scsi-disk.c | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 4bd7af9d0c..4f914df5c2 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -2648,16 +2648,12 @@ static const SCSIReqOps *const scsi_disk_reqops_dispatch[256] = {
>   
>   static void scsi_disk_new_request_dump(uint32_t lun, uint32_t tag, uint8_t *buf)
>   {
> -    int i;
>       int len = scsi_cdb_length(buf);
> -    char *line_buffer, *p;
> +    char *line_buffer;
>   
>       assert(len > 0 && len <= 16);
> -    line_buffer = g_malloc(len * 5 + 1);
>   
> -    for (i = 0, p = line_buffer; i < len; i++) {
> -        p += sprintf(p, " 0x%02x", buf[i]);
> -    }
> +    line_buffer = qemu_hexdump_line(buf, 0, len, false);

This is adding "0000: " as an unnecessary prefix, because it's added by qemu_hexdump_line.
I think having qemu_hexdump_line as a primitive is good, but probably the offset argument 
should be dropped and printed by the two callers that need it (mostly qemu_hexdump).


r~

[PATCH v2 11/13] hw/ide/atapi: Use qemu_hexdump_line() to avoid sprintf()

[Philippe Mathieu-Daudé]

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience.

Use qemu_hexdump_line() to avoid sprintf() calls, silencing:

  [1367/1604] Compiling C object libcommon.fa.p/backends_tpm_tpm_util.c.o
  backends/tpm/tpm_util.c:355:18: warning: 'sprintf' is deprecated:
    This function is provided for compatibility reasons only.
    Due to security concerns inherent in the design of sprintf(3),
    it is highly recommended that you use snprintf(3) instead.
    [-Wdeprecated-declarations]
            p += sprintf(p, "\n");
                 ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ide/atapi.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 73ec373184..27b6aa59fd 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -24,6 +24,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/cutils.h"
 #include "hw/scsi/scsi.h"
 #include "sysemu/block-backend.h"
 #include "scsi/constants.h"
@@ -1309,12 +1310,7 @@ void ide_atapi_cmd(IDEState *s)
     trace_ide_atapi_cmd(s, s->io_buffer[0]);
 
     if (trace_event_get_state_backends(TRACE_IDE_ATAPI_CMD_PACKET)) {
-        /* Each pretty-printed byte needs two bytes and a space; */
-        char *ppacket = g_malloc(ATAPI_PACKET_SIZE * 3 + 1);
-        int i;
-        for (i = 0; i < ATAPI_PACKET_SIZE; i++) {
-            sprintf(ppacket + (i * 3), "%02x ", buf[i]);
-        }
+        char *ppacket = qemu_hexdump_line(buf, 0, ATAPI_PACKET_SIZE, false);
         trace_ide_atapi_cmd_packet(s, s->lcyl | (s->hcyl << 8), ppacket);
         g_free(ppacket);
     }
-- 
2.41.0

[PATCH v2 12/13] hw/dma/pl330: Use qemu_hexdump_line() to avoid sprintf()

[Philippe Mathieu-Daudé]

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience.

Use qemu_hexdump_line() to avoid sprintf() calls, silencing:

  [5/8] Compiling C object libcommon.fa.p/hw_dma_pl330.c.o
  hw/dma/pl330.c:333:13: warning: 'sprintf' is deprecated:
    This function is provided for compatibility reasons only.
    Due to security concerns inherent in the design of sprintf(3),
    it is highly recommended that you use snprintf(3) instead.
    [-Wdeprecated-declarations]
            sprintf(tmpbuf + strlen(tmpbuf), " %02x", buf[b + i]);
            ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/dma/pl330.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/hw/dma/pl330.c b/hw/dma/pl330.c
index 70a502d245..0435378b7e 100644
--- a/hw/dma/pl330.c
+++ b/hw/dma/pl330.c
@@ -15,6 +15,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/cutils.h"
 #include "hw/irq.h"
 #include "hw/qdev-properties.h"
 #include "hw/sysbus.h"
@@ -317,21 +318,14 @@ typedef struct PL330InsnDesc {
 
 static void pl330_hexdump(uint8_t *buf, size_t size)
 {
-    unsigned int b, i, len;
-    char tmpbuf[80];
+    unsigned int b, len;
 
     for (b = 0; b < size; b += 16) {
         len = size - b;
         if (len > 16) {
             len = 16;
         }
-        tmpbuf[0] = '\0';
-        for (i = 0; i < len; i++) {
-            if ((i % 4) == 0) {
-                strcat(tmpbuf, " ");
-            }
-            sprintf(tmpbuf + strlen(tmpbuf), " %02x", buf[b + i]);
-        }
+        g_autofree char *tmpbuf = qemu_hexdump_line(buf, b, len, false);
         trace_pl330_hexdump(b, tmpbuf);
     }
 }
-- 
2.41.0

[PATCH v2 13/13] backends/tpm: Use qemu_hexdump_line() to avoid sprintf()

[Philippe Mathieu-Daudé]

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience.

Use qemu_hexdump_line() to avoid sprintf() calls, silencing:

  backends/tpm/tpm_util.c:357:14: warning: 'sprintf' is deprecated:
    This function is provided for compatibility reasons only.
    Due to security concerns inherent in the design of sprintf(3),
    it is highly recommended that you use snprintf(3) instead.
    [-Wdeprecated-declarations]
        p += sprintf(p, "%.2X ", buffer[i]);
             ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
---
 backends/tpm/tpm_util.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/backends/tpm/tpm_util.c b/backends/tpm/tpm_util.c
index 1856589c3b..0747af2d1c 100644
--- a/backends/tpm/tpm_util.c
+++ b/backends/tpm/tpm_util.c
@@ -21,6 +21,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu/error-report.h"
+#include "qemu/cutils.h"
 #include "qapi/error.h"
 #include "qapi/visitor.h"
 #include "tpm_int.h"
@@ -336,27 +337,18 @@ void tpm_sized_buffer_reset(TPMSizedBuffer *tsb)
 void tpm_util_show_buffer(const unsigned char *buffer,
                           size_t buffer_size, const char *string)
 {
-    size_t len, i;
-    char *line_buffer, *p;
+    size_t len;
+    char *line, *lineup;
 
     if (!trace_event_get_state_backends(TRACE_TPM_UTIL_SHOW_BUFFER)) {
         return;
     }
     len = MIN(tpm_cmd_get_size(buffer), buffer_size);
 
-    /*
-     * allocate enough room for 3 chars per buffer entry plus a
-     * newline after every 16 chars and a final null terminator.
-     */
-    line_buffer = g_malloc(len * 3 + (len / 16) + 1);
+    line = qemu_hexdump_line(buffer, 0, len, false);
+    lineup = g_ascii_strup(line, -1);
+    trace_tpm_util_show_buffer(string, len, lineup);
 
-    for (i = 0, p = line_buffer; i < len; i++) {
-        if (i && !(i % 16)) {
-            p += sprintf(p, "\n");
-        }
-        p += sprintf(p, "%.2X ", buffer[i]);
-    }
-    trace_tpm_util_show_buffer(string, len, line_buffer);
-
-    g_free(line_buffer);
+    g_free(line);
+    g_free(lineup);
 }
-- 
2.41.0