[Qemu-devel] [PATCH 0/7] Some -smbios work

[Qemu-devel] [PATCH 0/7] Some -smbios work

[Markus Armbruster]

Better error messages, a bit of code cleanup, and a big endian fix.

Not addressed: qemu_uuid_parse() sets an SMBIOS field by side effect.
Gross!

Markus Armbruster (7):
  error-report.h: Supply missing include
  log.h: Supply missing includes
  smbios: Convert to error_report()
  Use sizeof(qemu_uuid) instead of literal 16
  smbios: Clean up smbios_add_field() parameters
  smbios: Fix -smbios type=0,release=... for big endian hosts
  smbios: Check R in -smbios type=0,release=R parses okay

 arch_init.c                 |  4 ++--
 hw/i386/smbios.c            | 57 ++++++++++++++++++++++++---------------------
 hw/nvram/fw_cfg.c           |  2 +-
 include/hw/i386/smbios.h    |  2 +-
 include/qemu/error-report.h |  1 +
 include/qemu/log.h          |  3 +++
 include/sysemu/sysemu.h     |  2 +-
 7 files changed, 40 insertions(+), 31 deletions(-)

-- 
1.7.11.7

[Qemu-devel] [PATCH 1/7] error-report.h: Supply missing include

[Markus Armbruster]

Missed in commit e5924d8.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qemu/error-report.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index c902cc1..14c1719 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -14,6 +14,7 @@
 #define QEMU_ERROR_H
 
 #include <stdarg.h>
+#include "qemu/compiler.h"
 
 typedef struct Location {
     /* all members are private to qemu-error.c */
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 1/7] error-report.h: Supply missing include

[Laszlo Ersek]

On 06/06/13 18:27, Markus Armbruster wrote:
> Missed in commit e5924d8.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/qemu/error-report.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
> index c902cc1..14c1719 100644
> --- a/include/qemu/error-report.h
> +++ b/include/qemu/error-report.h
> @@ -14,6 +14,7 @@
>  #define QEMU_ERROR_H
>  
>  #include <stdarg.h>
> +#include "qemu/compiler.h"
>  
>  typedef struct Location {
>      /* all members are private to qemu-error.c */
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

[Qemu-devel] [PATCH 2/7] log.h: Supply missing includes

[Markus Armbruster]

<stdio.h> has always been missing.  Rest missed in commit eeacee4.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qemu/log.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/qemu/log.h b/include/qemu/log.h
index 6b0db02..fd76f91 100644
--- a/include/qemu/log.h
+++ b/include/qemu/log.h
@@ -2,6 +2,9 @@
 #define QEMU_LOG_H
 
 #include <stdarg.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include "qemu/compiler.h"
 #ifdef NEED_CPU_H
 #include "disas/disas.h"
 #endif
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 2/7] log.h: Supply missing includes

[Laszlo Ersek]

On 06/06/13 18:27, Markus Armbruster wrote:
> <stdio.h> has always been missing.  Rest missed in commit eeacee4.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/qemu/log.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/qemu/log.h b/include/qemu/log.h
> index 6b0db02..fd76f91 100644
> --- a/include/qemu/log.h
> +++ b/include/qemu/log.h
> @@ -2,6 +2,9 @@
>  #define QEMU_LOG_H
>  
>  #include <stdarg.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include "qemu/compiler.h"
>  #ifdef NEED_CPU_H
>  #include "disas/disas.h"
>  #endif
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

[Qemu-devel] [PATCH 3/7] smbios: Convert to error_report()

[Markus Armbruster]

Improves diagnistics from ad hoc messages like

    Invalid SMBIOS UUID string

to

    qemu-system-x86_64: -smbios type=1,uuid=gaga: Invalid UUID

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 arch_init.c      |  1 -
 hw/i386/smbios.c | 24 ++++++++++++------------
 2 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 5d32ecf..5d71870 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -1053,7 +1053,6 @@ void do_smbios_option(const char *optarg)
 {
 #ifdef TARGET_I386
     if (smbios_entry_add(optarg) < 0) {
-        fprintf(stderr, "Wrong smbios provided\n");
         exit(1);
     }
 #endif
diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index c00bb2f..a67a328 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -13,6 +13,7 @@
  * GNU GPL, version 2 or (at your option) any later version.
  */
 
+#include "qemu/error-report.h"
 #include "sysemu/sysemu.h"
 #include "hw/i386/smbios.h"
 #include "hw/loader.h"
@@ -48,8 +49,7 @@ static int smbios_type4_count = 0;
 static void smbios_validate_table(void)
 {
     if (smbios_type4_count && smbios_type4_count != smp_cpus) {
-         fprintf(stderr,
-                 "Number of SMBIOS Type 4 tables must match cpu count.\n");
+        error_report("Number of SMBIOS Type 4 tables must match cpu count");
         exit(1);
     }
 }
@@ -82,16 +82,16 @@ static void smbios_check_collision(int type, int entry)
         if (entry == SMBIOS_TABLE_ENTRY && header->type == SMBIOS_FIELD_ENTRY) {
             struct smbios_field *field = (void *)header;
             if (type == field->type) {
-                fprintf(stderr, "SMBIOS type %d field already defined, "
-                                "cannot add table\n", type);
+                error_report("SMBIOS type %d field already defined, "
+                             "cannot add table", type);
                 exit(1);
             }
         } else if (entry == SMBIOS_FIELD_ENTRY &&
                    header->type == SMBIOS_TABLE_ENTRY) {
             struct smbios_structure_header *table = (void *)(header + 1);
             if (type == table->type) {
-                fprintf(stderr, "SMBIOS type %d table already defined, "
-                                "cannot add field\n", type);
+                error_report("SMBIOS type %d table already defined, "
+                             "cannot add field", type);
                 exit(1);
             }
         }
@@ -166,7 +166,7 @@ static void smbios_build_type_1_fields(const char *t)
                          strlen(buf) + 1, buf);
     if (get_param_value(buf, sizeof(buf), "uuid", t)) {
         if (qemu_uuid_parse(buf, qemu_uuid) != 0) {
-            fprintf(stderr, "Invalid SMBIOS UUID string\n");
+            error_report("Invalid UUID");
             exit(1);
         }
     }
@@ -188,7 +188,7 @@ int smbios_entry_add(const char *t)
         int size = get_image_size(buf);
 
         if (size == -1 || size < sizeof(struct smbios_structure_header)) {
-            fprintf(stderr, "Cannot read smbios file %s\n", buf);
+            error_report("Cannot read SMBIOS file %s", buf);
             exit(1);
         }
 
@@ -204,7 +204,7 @@ int smbios_entry_add(const char *t)
         table->header.length = cpu_to_le16(sizeof(*table) + size);
 
         if (load_image(buf, table->data) != size) {
-            fprintf(stderr, "Failed to load smbios file %s", buf);
+            error_report("Failed to load SMBIOS file %s", buf);
             exit(1);
         }
 
@@ -230,12 +230,12 @@ int smbios_entry_add(const char *t)
             smbios_build_type_1_fields(t);
             return 0;
         default:
-            fprintf(stderr, "Don't know how to build fields for SMBIOS type "
-                    "%ld\n", type);
+            error_report("Don't know how to build fields for SMBIOS type %ld",
+                         type);
             exit(1);
         }
     }
 
-    fprintf(stderr, "smbios: must specify type= or file=\n");
+    error_report("Must specify type= or file=");
     return -1;
 }
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 3/7] smbios: Convert to error_report()

[Laszlo Ersek]

On 06/06/13 18:27, Markus Armbruster wrote:
> Improves diagnistics from ad hoc messages like
> 
>     Invalid SMBIOS UUID string
> 
> to
> 
>     qemu-system-x86_64: -smbios type=1,uuid=gaga: Invalid UUID
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  arch_init.c      |  1 -
>  hw/i386/smbios.c | 24 ++++++++++++------------
>  2 files changed, 12 insertions(+), 13 deletions(-)

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

[Qemu-devel] [PATCH 4/7] Use sizeof(qemu_uuid) instead of literal 16

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 arch_init.c             | 3 ++-
 hw/nvram/fw_cfg.c       | 2 +-
 include/sysemu/sysemu.h | 2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 5d71870..aa24660 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -1029,7 +1029,8 @@ int qemu_uuid_parse(const char *str, uint8_t *uuid)
         return -1;
     }
 #ifdef TARGET_I386
-    smbios_add_field(1, offsetof(struct smbios_type_1, uuid), 16, uuid);
+    smbios_add_field(1, offsetof(struct smbios_type_1, uuid),
+                     sizeof(uuid), uuid);
 #endif
     return 0;
 }
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 3c255ce..f1d3861 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -509,7 +509,7 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
         sysbus_mmio_map(d, 1, data_addr);
     }
     fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
-    fw_cfg_add_bytes(s, FW_CFG_UUID, qemu_uuid, 16);
+    fw_cfg_add_bytes(s, FW_CFG_UUID, qemu_uuid, sizeof(qemu_uuid));
     fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)(display_type == DT_NOGRAPHIC));
     fw_cfg_add_i16(s, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
     fw_cfg_add_i16(s, FW_CFG_BOOT_MENU, (uint16_t)boot_menu);
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 2fb71af..b969e56 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -15,7 +15,7 @@
 extern const char *bios_name;
 
 extern const char *qemu_name;
-extern uint8_t qemu_uuid[];
+extern uint8_t qemu_uuid[16];
 int qemu_uuid_parse(const char *str, uint8_t *uuid);
 #define UUID_FMT "%02hhx%02hhx%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx"
 
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 4/7] Use sizeof(qemu_uuid) instead of literal 16

[Laszlo Ersek]

On 06/06/13 18:27, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  arch_init.c             | 3 ++-
>  hw/nvram/fw_cfg.c       | 2 +-
>  include/sysemu/sysemu.h | 2 +-
>  3 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 5d71870..aa24660 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -1029,7 +1029,8 @@ int qemu_uuid_parse(const char *str, uint8_t *uuid)
>          return -1;
>      }
>  #ifdef TARGET_I386
> -    smbios_add_field(1, offsetof(struct smbios_type_1, uuid), 16, uuid);
> +    smbios_add_field(1, offsetof(struct smbios_type_1, uuid),
> +                     sizeof(uuid), uuid);
>  #endif
>      return 0;
>  }

I believe this is wrong, "uuid" is not an array here but a pointer. I
guess you mistyped "sizeof(qemu_uuid)" as "sizeof(uuid)" in the third arg.

> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 3c255ce..f1d3861 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -509,7 +509,7 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
>          sysbus_mmio_map(d, 1, data_addr);
>      }
>      fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
> -    fw_cfg_add_bytes(s, FW_CFG_UUID, qemu_uuid, 16);
> +    fw_cfg_add_bytes(s, FW_CFG_UUID, qemu_uuid, sizeof(qemu_uuid));
>      fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)(display_type == DT_NOGRAPHIC));
>      fw_cfg_add_i16(s, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
>      fw_cfg_add_i16(s, FW_CFG_BOOT_MENU, (uint16_t)boot_menu);
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 2fb71af..b969e56 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -15,7 +15,7 @@
>  extern const char *bios_name;
>  
>  extern const char *qemu_name;
> -extern uint8_t qemu_uuid[];
> +extern uint8_t qemu_uuid[16];
>  int qemu_uuid_parse(const char *str, uint8_t *uuid);
>  #define UUID_FMT "%02hhx%02hhx%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx"
>  
> 

Rest seems OK.

Laszlo

Re: [Qemu-devel] [PATCH 4/7] Use sizeof(qemu_uuid) instead of literal 16

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> On 06/06/13 18:27, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  arch_init.c             | 3 ++-
>>  hw/nvram/fw_cfg.c       | 2 +-
>>  include/sysemu/sysemu.h | 2 +-
>>  3 files changed, 4 insertions(+), 3 deletions(-)
>> 
>> diff --git a/arch_init.c b/arch_init.c
>> index 5d71870..aa24660 100644
>> --- a/arch_init.c
>> +++ b/arch_init.c
>> @@ -1029,7 +1029,8 @@ int qemu_uuid_parse(const char *str, uint8_t *uuid)
>>          return -1;
>>      }
>>  #ifdef TARGET_I386
>> -    smbios_add_field(1, offsetof(struct smbios_type_1, uuid), 16, uuid);
>> +    smbios_add_field(1, offsetof(struct smbios_type_1, uuid),
>> +                     sizeof(uuid), uuid);
>>  #endif
>>      return 0;
>>  }
>
> I believe this is wrong, "uuid" is not an array here but a pointer. I
> guess you mistyped "sizeof(qemu_uuid)" as "sizeof(uuid)" in the third arg.

Rats!  You're right.  Thanks!

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

[Markus Armbruster]

Having size preceed the associated pointer is odd.  Swap them, and fix
up the types.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 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));
 #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);
 
 /*
-- 
1.7.11.7

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 <armbru@redhat.com>
> ---
>  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

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

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> 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"? :)

Sure.

[Qemu-devel] [PATCH 6/7] smbios: Fix -smbios type=0, release=... for big endian hosts

[Markus Armbruster]

Classic endianness bug due to careless dirty coding: assuming reading
a byte from an int variable gets the least significant byte.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/i386/smbios.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index 322f0a0..68bd6d0 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -127,6 +127,7 @@ void smbios_add_field(int type, int offset, const void *data, size_t len)
 static void smbios_build_type_0_fields(const char *t)
 {
     char buf[1024];
+    unsigned char major, minor;
 
     if (get_param_value(buf, sizeof(buf), "vendor", t))
         smbios_add_field(0, offsetof(struct smbios_type_0, vendor_str),
@@ -139,8 +140,7 @@ static void smbios_build_type_0_fields(const char *t)
                                      bios_release_date_str),
                          buf, strlen(buf) + 1);
     if (get_param_value(buf, sizeof(buf), "release", t)) {
-        int major, minor;
-        sscanf(buf, "%d.%d", &major, &minor);
+        sscanf(buf, "%hhd.%hhd", &major, &minor);
         smbios_add_field(0, offsetof(struct smbios_type_0,
                                      system_bios_major_release),
                          &major, 1);
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 6/7] smbios: Fix -smbios type=0, release=... for big endian hosts

[Laszlo Ersek]

On 06/06/13 18:27, Markus Armbruster wrote:
> Classic endianness bug due to careless dirty coding: assuming reading
> a byte from an int variable gets the least significant byte.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/i386/smbios.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
> index 322f0a0..68bd6d0 100644
> --- a/hw/i386/smbios.c
> +++ b/hw/i386/smbios.c
> @@ -127,6 +127,7 @@ void smbios_add_field(int type, int offset, const void *data, size_t len)
>  static void smbios_build_type_0_fields(const char *t)
>  {
>      char buf[1024];
> +    unsigned char major, minor;
>  
>      if (get_param_value(buf, sizeof(buf), "vendor", t))
>          smbios_add_field(0, offsetof(struct smbios_type_0, vendor_str),
> @@ -139,8 +140,7 @@ static void smbios_build_type_0_fields(const char *t)
>                                       bios_release_date_str),
>                           buf, strlen(buf) + 1);
>      if (get_param_value(buf, sizeof(buf), "release", t)) {
> -        int major, minor;
> -        sscanf(buf, "%d.%d", &major, &minor);
> +        sscanf(buf, "%hhd.%hhd", &major, &minor);
>          smbios_add_field(0, offsetof(struct smbios_type_0,
>                                       system_bios_major_release),
>                           &major, 1);
> 

Strictly speaking these should be %hhu, if it's not much of a bother.
Otherwise:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

(BTW what was wrong with the definitions being in the narrowest scope? :))

Re: [Qemu-devel] [PATCH 6/7] smbios: Fix -smbios type=0, release=... for big endian hosts

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> On 06/06/13 18:27, Markus Armbruster wrote:
>> Classic endianness bug due to careless dirty coding: assuming reading
>> a byte from an int variable gets the least significant byte.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/i386/smbios.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
>> index 322f0a0..68bd6d0 100644
>> --- a/hw/i386/smbios.c
>> +++ b/hw/i386/smbios.c
>> @@ -127,6 +127,7 @@ void smbios_add_field(int type, int offset, const void *data, size_t len)
>>  static void smbios_build_type_0_fields(const char *t)
>>  {
>>      char buf[1024];
>> +    unsigned char major, minor;
>>  
>>      if (get_param_value(buf, sizeof(buf), "vendor", t))
>>          smbios_add_field(0, offsetof(struct smbios_type_0, vendor_str),
>> @@ -139,8 +140,7 @@ static void smbios_build_type_0_fields(const char *t)
>>                                       bios_release_date_str),
>>                           buf, strlen(buf) + 1);
>>      if (get_param_value(buf, sizeof(buf), "release", t)) {
>> -        int major, minor;
>> -        sscanf(buf, "%d.%d", &major, &minor);
>> +        sscanf(buf, "%hhd.%hhd", &major, &minor);
>>          smbios_add_field(0, offsetof(struct smbios_type_0,
>>                                       system_bios_major_release),
>>                           &major, 1);
>> 
>
> Strictly speaking these should be %hhu, if it's not much of a bother.

It's not.

> Otherwise:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> (BTW what was wrong with the definitions being in the narrowest scope? :))

I don't like spreading declarations all over the function in C.  Not
entirely sure why.  Perhaps it's just habit.  Perhaps it's because
declarations don't stand out visually, unlike in some other languages.

[Qemu-devel] [PATCH 7/7] smbios: Check R in -smbios type=0, release=R parses okay

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/i386/smbios.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index 68bd6d0..88a1360 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -140,7 +140,10 @@ static void smbios_build_type_0_fields(const char *t)
                                      bios_release_date_str),
                          buf, strlen(buf) + 1);
     if (get_param_value(buf, sizeof(buf), "release", t)) {
-        sscanf(buf, "%hhd.%hhd", &major, &minor);
+        if (sscanf(buf, "%hhd.%hhd", &major, &minor) != 2) {
+            error_report("Invalid release");
+            exit(1);
+        }
         smbios_add_field(0, offsetof(struct smbios_type_0,
                                      system_bios_major_release),
                          &major, 1);
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 7/7] smbios: Check R in -smbios type=0, release=R parses okay

[Laszlo Ersek]

On 06/06/13 18:27, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/i386/smbios.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
> index 68bd6d0..88a1360 100644
> --- a/hw/i386/smbios.c
> +++ b/hw/i386/smbios.c
> @@ -140,7 +140,10 @@ static void smbios_build_type_0_fields(const char *t)
>                                       bios_release_date_str),
>                           buf, strlen(buf) + 1);
>      if (get_param_value(buf, sizeof(buf), "release", t)) {
> -        sscanf(buf, "%hhd.%hhd", &major, &minor);
> +        if (sscanf(buf, "%hhd.%hhd", &major, &minor) != 2) {
> +            error_report("Invalid release");
> +            exit(1);
> +        }
>          smbios_add_field(0, offsetof(struct smbios_type_0,
>                                       system_bios_major_release),
>                           &major, 1);
> 

Right. OTOH if any of the decimal strings provided doesn't fit into the
space provided (eg. you pass "256" for an "unsigned char" which happens
to be uint8_t), the behavior is undefined anyway. sscanf() cannot be
used with "untrusted" data. ("... if the result of the conversion cannot
be represented in the space provided, the behavior is undefined.")

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [Qemu-devel] [PATCH 7/7] smbios: Check R in -smbios type=0, release=R parses okay

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> On 06/06/13 18:27, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/i386/smbios.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
>> index 68bd6d0..88a1360 100644
>> --- a/hw/i386/smbios.c
>> +++ b/hw/i386/smbios.c
>> @@ -140,7 +140,10 @@ static void smbios_build_type_0_fields(const char *t)
>>                                       bios_release_date_str),
>>                           buf, strlen(buf) + 1);
>>      if (get_param_value(buf, sizeof(buf), "release", t)) {
>> -        sscanf(buf, "%hhd.%hhd", &major, &minor);
>> +        if (sscanf(buf, "%hhd.%hhd", &major, &minor) != 2) {
>> +            error_report("Invalid release");
>> +            exit(1);
>> +        }
>>          smbios_add_field(0, offsetof(struct smbios_type_0,
>>                                       system_bios_major_release),
>>                           &major, 1);
>> 
>
> Right. OTOH if any of the decimal strings provided doesn't fit into the
> space provided (eg. you pass "256" for an "unsigned char" which happens
> to be uint8_t), the behavior is undefined anyway. sscanf() cannot be
> used with "untrusted" data. ("... if the result of the conversion cannot
> be represented in the space provided, the behavior is undefined.")

Yes, this isn't rigorous parsing.  It improves the code from "brazenly
careless" to the more common (in QEMU) "quick but sloppy".

> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks for the review!