diff mbox series

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

Message ID 20240411101550.99392-5-philmd@linaro.org
State New
Headers show
Series misc: Remove sprintf() due to macOS deprecation | expand

Commit Message

Philippe Mathieu-Daudé April 11, 2024, 10:15 a.m. UTC 
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(-)

Comments

Richard Henderson April 11, 2024, 8:07 p.m. UTC | #1 
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~
Richard Henderson April 11, 2024, 8:31 p.m. UTC | #2 
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~
diff mbox series

Patch

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;