[Qemu-devel] [PATCH v2] hw: fix error reporting for missing option ROMs

["Daniel P. Berrange" <berrange@redhat.com>]

If QEMU fails to load any of the VGA ROMs, it prints a message
to stderr and then carries on as if everything was fine, despite
the VGA interface not being functional. This extends the the
various rom_add_*() methods in loader.h to accept a 'Error **errp'
parameter. The VGA device realizefn() impls can now pass in the
errp they already have and get errors reported as fatal problems.

Addition of 'Error **errp' to the load_*() methods in loader.h is
left as an exercise for future interested developers, since it will
require fixing up a great many callers to propagate errors correctly.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---

Changed in v2:

 - Use error_fatal instead of NULL in places lacking an
   Error **errp to propagate to
 - Use error_setg_file_open instead of error_setg_errno
 - Mention that load_*() methods are intentionally not converted

 hw/core/loader.c        | 38 +++++++++++++++++++++++---------------
 hw/display/cirrus_vga.c |  4 +++-
 hw/display/vga-isa.c    |  4 +++-
 hw/i386/pc.c            |  6 ++++--
 hw/i386/pc_sysfw.c      |  5 +++--
 hw/misc/sga.c           |  4 +++-
 hw/pci/pci.c            |  8 ++++++--
 include/hw/loader.h     | 16 +++++++++-------
 8 files changed, 54 insertions(+), 31 deletions(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 8e8031c..2c9be4e 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -142,7 +142,7 @@ int load_image_targphys(const char *filename,
         return -1;
     }
     if (size > 0) {
-        rom_add_file_fixed(filename, addr, -1);
+        rom_add_file_fixed(filename, addr, -1, NULL);
     }
     return size;
 }
@@ -162,7 +162,7 @@ int load_image_mr(const char *filename, MemoryRegion *mr)
         return -1;
     }
     if (size > 0) {
-        if (rom_add_file_mr(filename, mr, -1) < 0) {
+        if (rom_add_file_mr(filename, mr, -1, NULL) < 0) {
             return -1;
         }
     }
@@ -831,11 +831,13 @@ static void *rom_set_mr(Rom *rom, Object *owner, const char *name)
 
 int rom_add_file(const char *file, const char *fw_dir,
                  hwaddr addr, int32_t bootindex,
-                 bool option_rom, MemoryRegion *mr)
+                 bool option_rom, MemoryRegion *mr,
+                 Error **errp)
 {
     MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
     Rom *rom;
-    int rc, fd = -1;
+    int fd = -1;
+    ssize_t rc;
     char devpath[100];
 
     rom = g_malloc0(sizeof(*rom));
@@ -847,8 +849,7 @@ int rom_add_file(const char *file, const char *fw_dir,
 
     fd = open(rom->path, O_RDONLY | O_BINARY);
     if (fd == -1) {
-        fprintf(stderr, "Could not open option rom '%s': %s\n",
-                rom->path, strerror(errno));
+        error_setg_file_open(errp, errno, rom->path);
         goto err;
     }
 
@@ -859,8 +860,9 @@ int rom_add_file(const char *file, const char *fw_dir,
     rom->addr     = addr;
     rom->romsize  = lseek(fd, 0, SEEK_END);
     if (rom->romsize == -1) {
-        fprintf(stderr, "rom: file %-20s: get size error: %s\n",
-                rom->name, strerror(errno));
+        error_setg_errno(errp, errno,
+                         "Could not get size of option rom '%s'",
+                         rom->path);
         goto err;
     }
 
@@ -868,9 +870,15 @@ int rom_add_file(const char *file, const char *fw_dir,
     rom->data     = g_malloc0(rom->datasize);
     lseek(fd, 0, SEEK_SET);
     rc = read(fd, rom->data, rom->datasize);
-    if (rc != rom->datasize) {
-        fprintf(stderr, "rom: file %-20s: read error: rc=%d (expected %zd)\n",
-                rom->name, rc, rom->datasize);
+    if (rc < 0) {
+        error_setg_errno(errp, errno,
+                         "Could not read option rom '%s'",
+                         rom->path);
+        goto err;
+    } else if (rc != rom->datasize) {
+        error_setg_errno(errp, errno,
+                         "Short read on option rom '%s' %zd vs %zd",
+                         rom->path, rc, rom->datasize);
         goto err;
     }
     close(fd);
@@ -975,14 +983,14 @@ int rom_add_elf_program(const char *name, void *data, size_t datasize,
     return 0;
 }
 
-int rom_add_vga(const char *file)
+int rom_add_vga(const char *file, Error **errp)
 {
-    return rom_add_file(file, "vgaroms", 0, -1, true, NULL);
+    return rom_add_file(file, "vgaroms", 0, -1, true, NULL, errp);
 }
 
-int rom_add_option(const char *file, int32_t bootindex)
+int rom_add_option(const char *file, int32_t bootindex, Error **errp)
 {
-    return rom_add_file(file, "genroms", 0, bootindex, true, NULL);
+    return rom_add_file(file, "genroms", 0, bootindex, true, NULL, errp);
 }
 
 static void rom_reset(void *unused)
diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index 57b91a7..7fbb2b0 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -2977,7 +2977,9 @@ static void isa_cirrus_vga_realizefn(DeviceState *dev, Error **errp)
                        isa_address_space(isadev),
                        isa_address_space_io(isadev));
     s->con = graphic_console_init(dev, 0, s->hw_ops, s);
-    rom_add_vga(VGABIOS_CIRRUS_FILENAME);
+    if (rom_add_vga(VGABIOS_CIRRUS_FILENAME, errp) < 0) {
+        return;
+    }
     /* XXX ISA-LFB support */
     /* FIXME not qdev yet */
 }
diff --git a/hw/display/vga-isa.c b/hw/display/vga-isa.c
index f5aff1c..4309ae1 100644
--- a/hw/display/vga-isa.c
+++ b/hw/display/vga-isa.c
@@ -72,7 +72,9 @@ static void vga_isa_realizefn(DeviceState *dev, Error **errp)
 
     vga_init_vbe(s, OBJECT(dev), isa_address_space(isadev));
     /* ROM BIOS */
-    rom_add_vga(VGABIOS_FILENAME);
+    if (rom_add_vga(VGABIOS_FILENAME, errp) < 0) {
+        return;
+    }
 }
 
 static Property vga_isa_properties[] = {
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 56ec6cd..471dcb4 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1264,7 +1264,8 @@ void xen_load_linux(PCMachineState *pcms)
     for (i = 0; i < nb_option_roms; i++) {
         assert(!strcmp(option_rom[i].name, "linuxboot.bin") ||
                !strcmp(option_rom[i].name, "multiboot.bin"));
-        rom_add_option(option_rom[i].name, option_rom[i].bootindex);
+        rom_add_option(option_rom[i].name, option_rom[i].bootindex,
+                       &error_fatal);
     }
     pcms->fw_cfg = fw_cfg;
 }
@@ -1392,7 +1393,8 @@ void pc_memory_init(PCMachineState *pcms,
     }
 
     for (i = 0; i < nb_option_roms; i++) {
-        rom_add_option(option_rom[i].name, option_rom[i].bootindex);
+        rom_add_option(option_rom[i].name, option_rom[i].bootindex,
+                       &error_fatal);
     }
     pcms->fw_cfg = fw_cfg;
 }
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 2324e70..d8dea64 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -178,6 +178,7 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
     MemoryRegion *bios, *isa_bios;
     int bios_size, isa_bios_size;
     int ret;
+    Error *err = NULL;
 
     /* BIOS load */
     if (bios_name == NULL) {
@@ -199,10 +200,10 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
     if (!isapc_ram_fw) {
         memory_region_set_readonly(bios, true);
     }
-    ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), -1);
+    ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), -1, &err);
     if (ret != 0) {
     bios_error:
-        fprintf(stderr, "qemu: could not load PC BIOS '%s'\n", bios_name);
+        error_report_err(err);
         exit(1);
     }
     g_free(filename);
diff --git a/hw/misc/sga.c b/hw/misc/sga.c
index 03b006d..f90ad1f 100644
--- a/hw/misc/sga.c
+++ b/hw/misc/sga.c
@@ -41,7 +41,9 @@ typedef struct ISASGAState {
 
 static void sga_realizefn(DeviceState *dev, Error **errp)
 {
-    rom_add_vga(SGABIOS_FILENAME);
+    if (rom_add_vga(SGABIOS_FILENAME, errp) < 0) {
+        return;
+    }
 }
 
 static void sga_class_initfn(ObjectClass *klass, void *data)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index e67664d..728a9ec 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2065,9 +2065,13 @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom,
         }
 
         if (class == 0x0300) {
-            rom_add_vga(pdev->romfile);
+            if (rom_add_vga(pdev->romfile, errp) < 0) {
+                return;
+            }
         } else {
-            rom_add_option(pdev->romfile, -1);
+            if (rom_add_option(pdev->romfile, -1, errp) < 0) {
+                return;
+            }
         }
         return;
     }
diff --git a/include/hw/loader.h b/include/hw/loader.h
index 0ba7808..dc9951d 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -2,6 +2,7 @@
 #define LOADER_H
 #include "qapi/qmp/qdict.h"
 #include "hw/nvram/fw_cfg.h"
+#include "qapi/error.h"
 
 /* loader.c */
 /**
@@ -118,7 +119,8 @@ extern bool rom_file_has_mr;
 
 int rom_add_file(const char *file, const char *fw_dir,
                  hwaddr addr, int32_t bootindex,
-                 bool option_rom, MemoryRegion *mr);
+                 bool option_rom, MemoryRegion *mr,
+                 Error **errp);
 MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
                            size_t max_len, hwaddr addr,
                            const char *fw_file_name,
@@ -132,12 +134,12 @@ int rom_copy(uint8_t *dest, hwaddr addr, size_t size);
 void *rom_ptr(hwaddr addr);
 void hmp_info_roms(Monitor *mon, const QDict *qdict);
 
-#define rom_add_file_fixed(_f, _a, _i)          \
-    rom_add_file(_f, NULL, _a, _i, false, NULL)
+#define rom_add_file_fixed(_f, _a, _i, e)       \
+    rom_add_file(_f, NULL, _a, _i, false, NULL, e)
 #define rom_add_blob_fixed(_f, _b, _l, _a)      \
     rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL)
-#define rom_add_file_mr(_f, _mr, _i)            \
-    rom_add_file(_f, NULL, 0, _i, false, mr)
+#define rom_add_file_mr(_f, _mr, _i, e)           \
+    rom_add_file(_f, NULL, 0, _i, false, mr, e)
 
 #define PC_ROM_MIN_VGA     0xc0000
 #define PC_ROM_MIN_OPTION  0xc8000
@@ -145,7 +147,7 @@ void hmp_info_roms(Monitor *mon, const QDict *qdict);
 #define PC_ROM_ALIGN       0x800
 #define PC_ROM_SIZE        (PC_ROM_MAX - PC_ROM_MIN_VGA)
 
-int rom_add_vga(const char *file);
-int rom_add_option(const char *file, int32_t bootindex);
+int rom_add_vga(const char *file, Error **errp);
+int rom_add_option(const char *file, int32_t bootindex, Error **errp);
 
 #endif
-- 
2.5.0