[Qemu-devel] [PATCH v2 0/6] Some -smbios work

[Qemu-devel] [PATCH v2 0/6] 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!

Testing:
* Verify error messages improve for
    -smbios gaga
    -smbios file=gaga
    -smbios type=42
    -smbios type=1,uuid=gaga
    -smbios type=0,release=gaga
* Verify SMBIOS table remains unchanged
    -smbios type=0,vendor=me,version=42,date=today,release=1.2 -smbios type=1,manufacturer=me,product=crap,version=6,serial=77,uuid=988bc9dd-0986-440f-ac24-cf9626c5aa88,sku=888,family=flintstones
  by sticking
      qemu_hexdump(smbios_entries, stdout, "SMBIOS", smbios_entries_len);
  into smbios_get_table()

v2: Address "Hawkeye" Laszlo's review
* 1-3/7 unchanged
* Drop 4/7 because it's buggy, and the fixed version isn't worthwhile
* Spelling fix in commit message of 5/7
* Correct scanf format in 5-6/7

Markus Armbruster (6):
  error-report.h: Supply missing include
  log.h: Supply missing includes
  smbios: Convert to error_report()
  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                 |  3 +--
 hw/i386/smbios.c            | 57 ++++++++++++++++++++++++---------------------
 include/hw/i386/smbios.h    |  2 +-
 include/qemu/error-report.h |  1 +
 include/qemu/log.h          |  3 +++
 5 files changed, 37 insertions(+), 29 deletions(-)

-- 
1.7.11.7

[Qemu-devel] [PATCH v2 1/6] 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

[Qemu-devel] [PATCH v2 2/6] 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

[Qemu-devel] [PATCH v2 3/6] 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

[Qemu-devel] [PATCH v2 4/6] smbios: Clean up smbios_add_field() parameters

[Markus Armbruster]

Having size precede 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 5d71870..872020e 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -1029,7 +1029,7 @@ 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), uuid, 16);
 #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

[Qemu-devel] [PATCH v2 5/6] 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..6431dd4 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, "%hhu.%hhu", &major, &minor);
         smbios_add_field(0, offsetof(struct smbios_type_0,
                                      system_bios_major_release),
                          &major, 1);
-- 
1.7.11.7

[Qemu-devel] [PATCH v2 6/6] 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 6431dd4..e708cb8 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, "%hhu.%hhu", &major, &minor);
+        if (sscanf(buf, "%hhu.%hhu", &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 v2 0/6] Some -smbios work

[Laszlo Ersek]

On 06/07/13 15:00, Markus Armbruster wrote:

> v2: Address "Hawkeye" Laszlo's review

You're too kind, but it did crack me up :)

(Next time I'll miss something I'll have to hang my head in shame all
the more!)

> * 1-3/7 unchanged
> * Drop 4/7 because it's buggy, and the fixed version isn't worthwhile
> * Spelling fix in commit message of 5/7
> * Correct scanf format in 5-6/7

series
Reviewed-by: Laszlo "ever the optimist" Ersek <lersek@redhat.com>

Re: [Qemu-devel] [PATCH v2 0/6] Some -smbios work

[Markus Armbruster]

Would this make sense for -stable?  Impact is modest: fix a rather
obscure feature of x86 targets on bigendian hosts, and improve error
messages around it).  On the other hand, the patches look pretty safe to
me.

Markus Armbruster <armbru@redhat.com> writes:

> 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!
>
> Testing:
> * Verify error messages improve for
>     -smbios gaga
>     -smbios file=gaga
>     -smbios type=42
>     -smbios type=1,uuid=gaga
>     -smbios type=0,release=gaga
> * Verify SMBIOS table remains unchanged
>     -smbios type=0,vendor=me,version=42,date=today,release=1.2 -smbios type=1,manufacturer=me,product=crap,version=6,serial=77,uuid=988bc9dd-0986-440f-ac24-cf9626c5aa88,sku=888,family=flintstones
>   by sticking
>       qemu_hexdump(smbios_entries, stdout, "SMBIOS", smbios_entries_len);
>   into smbios_get_table()
>
> v2: Address "Hawkeye" Laszlo's review
> * 1-3/7 unchanged
> * Drop 4/7 because it's buggy, and the fixed version isn't worthwhile
> * Spelling fix in commit message of 5/7
> * Correct scanf format in 5-6/7
>
> Markus Armbruster (6):
>   error-report.h: Supply missing include
>   log.h: Supply missing includes
>   smbios: Convert to error_report()
>   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                 |  3 +--
>  hw/i386/smbios.c            | 57 ++++++++++++++++++++++++---------------------
>  include/hw/i386/smbios.h    |  2 +-
>  include/qemu/error-report.h |  1 +
>  include/qemu/log.h          |  3 +++
>  5 files changed, 37 insertions(+), 29 deletions(-)

Re: [Qemu-devel] [PATCH v2 0/6] Some -smbios work

[Anthony Liguori]

Applied.  Thanks.

Regards,

Anthony Liguori

Re: [Qemu-devel] [Qemu-stable] [PATCH v2 0/6] Some -smbios work

[mdroth]

On Mon, Jun 17, 2013 at 01:36:04PM +0200, Markus Armbruster wrote:
> Would this make sense for -stable?  Impact is modest: fix a rather
> obscure feature of x86 targets on bigendian hosts, and improve error
> messages around it).  On the other hand, the patches look pretty safe to
> me.

5/6 was certainly a good candidate. I was going to cherry-pick it but
I agree the other patches looked safe to I went ahead and applied the
series:

https://github.com/mdroth/qemu/commits/stable-1.5-staging

> 
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > 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!
> >
> > Testing:
> > * Verify error messages improve for
> >     -smbios gaga
> >     -smbios file=gaga
> >     -smbios type=42
> >     -smbios type=1,uuid=gaga
> >     -smbios type=0,release=gaga
> > * Verify SMBIOS table remains unchanged
> >     -smbios type=0,vendor=me,version=42,date=today,release=1.2 -smbios type=1,manufacturer=me,product=crap,version=6,serial=77,uuid=988bc9dd-0986-440f-ac24-cf9626c5aa88,sku=888,family=flintstones
> >   by sticking
> >       qemu_hexdump(smbios_entries, stdout, "SMBIOS", smbios_entries_len);
> >   into smbios_get_table()
> >
> > v2: Address "Hawkeye" Laszlo's review
> > * 1-3/7 unchanged
> > * Drop 4/7 because it's buggy, and the fixed version isn't worthwhile
> > * Spelling fix in commit message of 5/7
> > * Correct scanf format in 5-6/7
> >
> > Markus Armbruster (6):
> >   error-report.h: Supply missing include
> >   log.h: Supply missing includes
> >   smbios: Convert to error_report()
> >   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                 |  3 +--
> >  hw/i386/smbios.c            | 57 ++++++++++++++++++++++++---------------------
> >  include/hw/i386/smbios.h    |  2 +-
> >  include/qemu/error-report.h |  1 +
> >  include/qemu/log.h          |  3 +++
> >  5 files changed, 37 insertions(+), 29 deletions(-)
> 

Re: [Qemu-devel] [Qemu-stable] [PATCH v2 0/6] Some -smbios work

[Markus Armbruster]

mdroth <mdroth@linux.vnet.ibm.com> writes:

> On Mon, Jun 17, 2013 at 01:36:04PM +0200, Markus Armbruster wrote:
>> Would this make sense for -stable?  Impact is modest: fix a rather
>> obscure feature of x86 targets on bigendian hosts, and improve error
>> messages around it).  On the other hand, the patches look pretty safe to
>> me.
>
> 5/6 was certainly a good candidate. I was going to cherry-pick it but
> I agree the other patches looked safe to I went ahead and applied the
> series:
>
> https://github.com/mdroth/qemu/commits/stable-1.5-staging

Thanks!