Re: [Qemu-devel] [PATCH 5/7] smbios: Clean up smbios_add_field() parameters

[Laszlo Ersek]

On 06/06/13 18:27, Markus Armbruster wrote:
> Having size preceed the associated pointer is odd.  Swap them, and fix
> up the types.

Can you proceed to fix the spelling of "precede"? :)

> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  arch_init.c              |  2 +-
>  hw/i386/smbios.c         | 26 ++++++++++++++------------
>  include/hw/i386/smbios.h |  2 +-
>  3 files changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index aa24660..06b65a2 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -1030,7 +1030,7 @@ int qemu_uuid_parse(const char *str, uint8_t *uuid)
>      }
>  #ifdef TARGET_I386
>      smbios_add_field(1, offsetof(struct smbios_type_1, uuid),
> -                     sizeof(uuid), uuid);
> +                     uuid, sizeof(uuid));

Still wrong I think.

>  #endif
>      return 0;
>  }
> diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
> index a67a328..322f0a0 100644
> --- a/hw/i386/smbios.c
> +++ b/hw/i386/smbios.c
> @@ -99,7 +99,7 @@ static void smbios_check_collision(int type, int entry)
>      }
>  }
>  
> -void smbios_add_field(int type, int offset, int len, void *data)
> +void smbios_add_field(int type, int offset, const void *data, size_t len)
>  {
>      struct smbios_field *field;
>  
> @@ -130,21 +130,23 @@ static void smbios_build_type_0_fields(const char *t)
>  
>      if (get_param_value(buf, sizeof(buf), "vendor", t))
>          smbios_add_field(0, offsetof(struct smbios_type_0, vendor_str),
> -                         strlen(buf) + 1, buf);
> +                         buf, strlen(buf) + 1);
>      if (get_param_value(buf, sizeof(buf), "version", t))
>          smbios_add_field(0, offsetof(struct smbios_type_0, bios_version_str),
> -                         strlen(buf) + 1, buf);
> +                         buf, strlen(buf) + 1);
>      if (get_param_value(buf, sizeof(buf), "date", t))
>          smbios_add_field(0, offsetof(struct smbios_type_0,
>                                       bios_release_date_str),
> -                                     strlen(buf) + 1, buf);
> +                         buf, strlen(buf) + 1);
>      if (get_param_value(buf, sizeof(buf), "release", t)) {
>          int major, minor;
>          sscanf(buf, "%d.%d", &major, &minor);
>          smbios_add_field(0, offsetof(struct smbios_type_0,
> -                                     system_bios_major_release), 1, &major);
> +                                     system_bios_major_release),
> +                         &major, 1);
>          smbios_add_field(0, offsetof(struct smbios_type_0,
> -                                     system_bios_minor_release), 1, &minor);
> +                                     system_bios_minor_release),
> +                         &minor, 1);
>      }
>  }
>  
> @@ -154,16 +156,16 @@ static void smbios_build_type_1_fields(const char *t)
>  
>      if (get_param_value(buf, sizeof(buf), "manufacturer", t))
>          smbios_add_field(1, offsetof(struct smbios_type_1, manufacturer_str),
> -                         strlen(buf) + 1, buf);
> +                         buf, strlen(buf) + 1);
>      if (get_param_value(buf, sizeof(buf), "product", t))
>          smbios_add_field(1, offsetof(struct smbios_type_1, product_name_str),
> -                         strlen(buf) + 1, buf);
> +                         buf, strlen(buf) + 1);
>      if (get_param_value(buf, sizeof(buf), "version", t))
>          smbios_add_field(1, offsetof(struct smbios_type_1, version_str),
> -                         strlen(buf) + 1, buf);
> +                         buf, strlen(buf) + 1);
>      if (get_param_value(buf, sizeof(buf), "serial", t))
>          smbios_add_field(1, offsetof(struct smbios_type_1, 
> serial_number_str),
> -                         strlen(buf) + 1, buf);
> +                         buf, strlen(buf) + 1);
>      if (get_param_value(buf, sizeof(buf), "uuid", t)) {
>          if (qemu_uuid_parse(buf, qemu_uuid) != 0) {
>              error_report("Invalid UUID");
> @@ -172,10 +174,10 @@ static void smbios_build_type_1_fields(const char *t)
>      }
>      if (get_param_value(buf, sizeof(buf), "sku", t))
>          smbios_add_field(1, offsetof(struct smbios_type_1, sku_number_str),
> -                         strlen(buf) + 1, buf);
> +                         buf, strlen(buf) + 1);
>      if (get_param_value(buf, sizeof(buf), "family", t))
>          smbios_add_field(1, offsetof(struct smbios_type_1, family_str),
> -                         strlen(buf) + 1, buf);
> +                         buf, strlen(buf) + 1);
>  }
>  
>  int smbios_entry_add(const char *t)
> diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h
> index 94e3641..9babeaf 100644
> --- a/include/hw/i386/smbios.h
> +++ b/include/hw/i386/smbios.h
> @@ -14,7 +14,7 @@
>   */
>  
>  int smbios_entry_add(const char *t);
> -void smbios_add_field(int type, int offset, int len, void *data);
> +void smbios_add_field(int type, int offset, const void *data, size_t len);
>  uint8_t *smbios_get_table(size_t *length);
>  
>  /*
> 

I trust gcc would have caught any missed calls ("creates pointer from
integer without cast" or some such). So the only problem (I think) is in
qemu_uuid_parse() which should be semi-auto-fixed when you rebase.

Laszlo