diff mbox

[v3] hw: fix error reporting for missing option ROMs

Message ID 1457716462-25568-1-git-send-email-berrange@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel P. Berrangé March 11, 2016, 5:14 p.m. UTC 
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.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 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      |  6 ++++--
 hw/misc/sga.c           |  4 +++-
 hw/pci/pci.c            |  8 ++++++--
 include/hw/loader.h     | 16 +++++++++-------
 8 files changed, 55 insertions(+), 31 deletions(-)

Comments

Eduardo Habkost April 4, 2016, 8:23 p.m. UTC | #1 
Hi,

Sorry for the long delay in reviewing this.

On Fri, Mar 11, 2016 at 05:14:22PM +0000, Daniel P. Berrange wrote:
> 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.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  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      |  6 ++++--
>  hw/misc/sga.c           |  4 +++-
>  hw/pci/pci.c            |  8 ++++++--
>  include/hw/loader.h     | 16 +++++++++-------
>  8 files changed, 55 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);

Currently, rom_add_file() errors here are ignored, but not silent
(rom_add_file() still reported them to stderr). Now they are
ignored silently.

>      }
>      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) {

No error details will be printed to stderr anymore, and the user
won't know why image loading failed.

>              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);

read() won't set errno if it just read a smaller number of bytes
than request, will it?

>          goto err;
>      }
>      close(fd);
[...]
diff mbox

Patch

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..091a288 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) {
@@ -191,6 +192,7 @@  static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
     }
     if (bios_size <= 0 ||
         (bios_size % 65536) != 0) {
+        error_setg(&err, "BIOS size %d not a multiple of 65536", bios_size);
         goto bios_error;
     }
     bios = g_malloc(sizeof(*bios));
@@ -199,10 +201,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