[Qemu-devel] [PATCH v2 11/35] smbios: Clean up error handling in smbios_add()

[Markus Armbruster <armbru@redhat.com>]

Calling error_report() in a function that takes an Error ** argument
is suspicious.  smbios_entry_add() does that, and then exit()s.  It
also passes &error_fatal to qemu_opts_validate().  Both wrong, but
currently harmless, as its only caller passes &error_fatal.  Messed up
in commit 1007a37e208.  Clean it up.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/smbios/smbios.c | 90 +++++++++++++++++++++++++++++++---------------
 1 file changed, 62 insertions(+), 28 deletions(-)

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index a27e54b2fa..920939454e 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -950,6 +950,7 @@ static void save_opt_list(size_t *ndest, const char ***dest,
 
 void smbios_entry_add(QemuOpts *opts, Error **errp)
 {
+    Error *err = NULL;
     const char *val;
 
     assert(!smbios_immutable);
@@ -960,12 +961,16 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
         int size;
         struct smbios_table *table; /* legacy mode only */
 
-        qemu_opts_validate(opts, qemu_smbios_file_opts, &error_fatal);
+        qemu_opts_validate(opts, qemu_smbios_file_opts, &err);
+        if (err) {
+            error_propagate(errp, err);
+            return;
+        }
 
         size = get_image_size(val);
         if (size == -1 || size < sizeof(struct smbios_structure_header)) {
-            error_report("Cannot read SMBIOS file %s", val);
-            exit(1);
+            error_setg(errp, "Cannot read SMBIOS file %s", val);
+            return;
         }
 
         /*
@@ -978,14 +983,15 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
                                                     smbios_tables_len);
 
         if (load_image(val, (uint8_t *)header) != size) {
-            error_report("Failed to load SMBIOS file %s", val);
-            exit(1);
+            error_setg(errp, "Failed to load SMBIOS file %s", val);
+            return;
         }
 
         if (test_bit(header->type, have_fields_bitmap)) {
-            error_report("can't load type %d struct, fields already specified!",
-                         header->type);
-            exit(1);
+            error_setg(errp,
+                       "can't load type %d struct, fields already specified!",
+                       header->type);
+            return;
         }
         set_bit(header->type, have_binfile_bitmap);
 
@@ -1030,19 +1036,23 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
         unsigned long type = strtoul(val, NULL, 0);
 
         if (type > SMBIOS_MAX_TYPE) {
-            error_report("out of range!");
-            exit(1);
+            error_setg(errp, "out of range!");
+            return;
         }
 
         if (test_bit(type, have_binfile_bitmap)) {
-            error_report("can't add fields, binary file already loaded!");
-            exit(1);
+            error_setg(errp, "can't add fields, binary file already loaded!");
+            return;
         }
         set_bit(type, have_fields_bitmap);
 
         switch (type) {
         case 0:
-            qemu_opts_validate(opts, qemu_smbios_type0_opts, &error_fatal);
+            qemu_opts_validate(opts, qemu_smbios_type0_opts, &err);
+            if (err) {
+                error_propagate(errp, err);
+                return;
+            }
             save_opt(&type0.vendor, opts, "vendor");
             save_opt(&type0.version, opts, "version");
             save_opt(&type0.date, opts, "date");
@@ -1051,14 +1061,18 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
             val = qemu_opt_get(opts, "release");
             if (val) {
                 if (sscanf(val, "%hhu.%hhu", &type0.major, &type0.minor) != 2) {
-                    error_report("Invalid release");
-                    exit(1);
+                    error_setg(errp, "Invalid release");
+                    return;
                 }
                 type0.have_major_minor = true;
             }
             return;
         case 1:
-            qemu_opts_validate(opts, qemu_smbios_type1_opts, &error_fatal);
+            qemu_opts_validate(opts, qemu_smbios_type1_opts, &err);
+            if (err) {
+                error_propagate(errp, err);
+                return;
+            }
             save_opt(&type1.manufacturer, opts, "manufacturer");
             save_opt(&type1.product, opts, "product");
             save_opt(&type1.version, opts, "version");
@@ -1069,14 +1083,18 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
             val = qemu_opt_get(opts, "uuid");
             if (val) {
                 if (qemu_uuid_parse(val, &qemu_uuid) != 0) {
-                    error_report("Invalid UUID");
-                    exit(1);
+                    error_setg(errp, "Invalid UUID");
+                    return;
                 }
                 qemu_uuid_set = true;
             }
             return;
         case 2:
-            qemu_opts_validate(opts, qemu_smbios_type2_opts, &error_fatal);
+            qemu_opts_validate(opts, qemu_smbios_type2_opts, &err);
+            if (err) {
+                error_propagate(errp, err);
+                return;
+            }
             save_opt(&type2.manufacturer, opts, "manufacturer");
             save_opt(&type2.product, opts, "product");
             save_opt(&type2.version, opts, "version");
@@ -1085,7 +1103,11 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
             save_opt(&type2.location, opts, "location");
             return;
         case 3:
-            qemu_opts_validate(opts, qemu_smbios_type3_opts, &error_fatal);
+            qemu_opts_validate(opts, qemu_smbios_type3_opts, &err);
+            if (err) {
+                error_propagate(errp, err);
+                return;
+            }
             save_opt(&type3.manufacturer, opts, "manufacturer");
             save_opt(&type3.version, opts, "version");
             save_opt(&type3.serial, opts, "serial");
@@ -1093,7 +1115,11 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
             save_opt(&type3.sku, opts, "sku");
             return;
         case 4:
-            qemu_opts_validate(opts, qemu_smbios_type4_opts, &error_fatal);
+            qemu_opts_validate(opts, qemu_smbios_type4_opts, &err);
+            if (err) {
+                error_propagate(errp, err);
+                return;
+            }
             save_opt(&type4.sock_pfx, opts, "sock_pfx");
             save_opt(&type4.manufacturer, opts, "manufacturer");
             save_opt(&type4.version, opts, "version");
@@ -1102,11 +1128,19 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
             save_opt(&type4.part, opts, "part");
             return;
         case 11:
-            qemu_opts_validate(opts, qemu_smbios_type11_opts, &error_fatal);
+            qemu_opts_validate(opts, qemu_smbios_type11_opts, &err);
+            if (err) {
+                error_propagate(errp, err);
+                return;
+            }
             save_opt_list(&type11.nvalues, &type11.values, opts, "value");
             return;
         case 17:
-            qemu_opts_validate(opts, qemu_smbios_type17_opts, &error_fatal);
+            qemu_opts_validate(opts, qemu_smbios_type17_opts, &err);
+            if (err) {
+                error_propagate(errp, err);
+                return;
+            }
             save_opt(&type17.loc_pfx, opts, "loc_pfx");
             save_opt(&type17.bank, opts, "bank");
             save_opt(&type17.manufacturer, opts, "manufacturer");
@@ -1116,12 +1150,12 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
             type17.speed = qemu_opt_get_number(opts, "speed", 0);
             return;
         default:
-            error_report("Don't know how to build fields for SMBIOS type %ld",
-                         type);
-            exit(1);
+            error_setg(errp,
+                       "Don't know how to build fields for SMBIOS type %ld",
+                       type);
+            return;
         }
     }
 
-    error_report("Must specify type= or file=");
-    exit(1);
+    error_setg(errp, "Must specify type= or file=");
 }
-- 
2.17.1