* [Qemu-devel] [PATCH v1] hw: fix error reporting for missing option ROMs
@ 2016-03-10 17:28 Daniel P. Berrange
2016-03-10 20:25 ` Eric Blake
0 siblings, 1 reply; 3+ messages in thread
From: Daniel P. Berrange @ 2016-03-10 17:28 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Richard Henderson, Eduardo Habkost,
Michael S. Tsirkin
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
rom_add_file() method 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.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
hw/core/loader.c | 40 +++++++++++++++++++++++++---------------
hw/display/cirrus_vga.c | 4 +++-
hw/display/vga-isa.c | 4 +++-
hw/i386/pc.c | 4 ++--
hw/i386/pc_sysfw.c | 2 +-
hw/misc/sga.c | 4 +++-
hw/pci/pci.c | 8 ++++++--
include/hw/loader.h | 16 +++++++++-------
8 files changed, 52 insertions(+), 30 deletions(-)
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 8e8031c..010e442 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,9 @@ 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_errno(errp, errno,
+ "Could not open option rom '%s'",
+ rom->path);
goto err;
}
@@ -859,8 +862,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 +872,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 +985,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..ba4fa67 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1264,7 +1264,7 @@ 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, NULL);
}
pcms->fw_cfg = fw_cfg;
}
@@ -1392,7 +1392,7 @@ 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, NULL);
}
pcms->fw_cfg = fw_cfg;
}
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 2324e70..e2c12a7 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -199,7 +199,7 @@ 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, NULL);
if (ret != 0) {
bios_error:
fprintf(stderr, "qemu: could not load PC BIOS '%s'\n", bios_name);
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH v1] hw: fix error reporting for missing option ROMs
2016-03-10 17:28 [Qemu-devel] [PATCH v1] hw: fix error reporting for missing option ROMs Daniel P. Berrange
@ 2016-03-10 20:25 ` Eric Blake
2016-03-11 9:27 ` Daniel P. Berrange
0 siblings, 1 reply; 3+ messages in thread
From: Eric Blake @ 2016-03-10 20:25 UTC (permalink / raw)
To: Daniel P. Berrange, qemu-devel
Cc: Paolo Bonzini, Michael S. Tsirkin, Eduardo Habkost,
Richard Henderson
[-- Attachment #1: Type: text/plain, Size: 3693 bytes --]
On 03/10/2016 10:28 AM, 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
> rom_add_file() method 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.
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
> hw/core/loader.c | 40 +++++++++++++++++++++++++---------------
> hw/display/cirrus_vga.c | 4 +++-
> hw/display/vga-isa.c | 4 +++-
> hw/i386/pc.c | 4 ++--
> hw/i386/pc_sysfw.c | 2 +-
> hw/misc/sga.c | 4 +++-
> hw/pci/pci.c | 8 ++++++--
> include/hw/loader.h | 16 +++++++++-------
> 8 files changed, 52 insertions(+), 30 deletions(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 8e8031c..010e442 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);
> }
Why is this one ignoring the error? Would &error_abort be better if we
know it can't fail?
> 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;
This one still detects and passes on failure, but loses the error
message. I guess that's okay, as long as this patch is incrementally
better somewhere else.
> @@ -847,8 +849,9 @@ 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_errno(errp, errno,
> + "Could not open option rom '%s'",
> + rom->path);
would error_setg_file_open() be any better here, for consistency?
> +++ b/hw/i386/pc.c
> @@ -1264,7 +1264,7 @@ 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, NULL);
Another place that blindly ignores things; should we use &error_abort?
> +++ b/hw/i386/pc_sysfw.c
> @@ -199,7 +199,7 @@ 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, NULL);
> if (ret != 0) {
> bios_error:
> fprintf(stderr, "qemu: could not load PC BIOS '%s'\n", bios_name);
This one makes sense - you are incrementally improving the interface,
and not all callers; this caller was already reporting errors and could
be cleaned up in a later commit to use &err instead of fprintf().
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH v1] hw: fix error reporting for missing option ROMs
2016-03-10 20:25 ` Eric Blake
@ 2016-03-11 9:27 ` Daniel P. Berrange
0 siblings, 0 replies; 3+ messages in thread
From: Daniel P. Berrange @ 2016-03-11 9:27 UTC (permalink / raw)
To: Eric Blake
Cc: Paolo Bonzini, Michael S. Tsirkin, qemu-devel, Eduardo Habkost,
Richard Henderson
On Thu, Mar 10, 2016 at 01:25:08PM -0700, Eric Blake wrote:
> On 03/10/2016 10:28 AM, 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
> > rom_add_file() method 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.
> >
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> > hw/core/loader.c | 40 +++++++++++++++++++++++++---------------
> > hw/display/cirrus_vga.c | 4 +++-
> > hw/display/vga-isa.c | 4 +++-
> > hw/i386/pc.c | 4 ++--
> > hw/i386/pc_sysfw.c | 2 +-
> > hw/misc/sga.c | 4 +++-
> > hw/pci/pci.c | 8 ++++++--
> > include/hw/loader.h | 16 +++++++++-------
> > 8 files changed, 52 insertions(+), 30 deletions(-)
> >
> > diff --git a/hw/core/loader.c b/hw/core/loader.c
> > index 8e8031c..010e442 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);
> > }
>
> Why is this one ignoring the error? Would &error_abort be better if we
> know it can't fail?
The loader.h header file has a tonne of different APIs which load files.
int get_image_size(const char *filename);
int load_image(const char *filename, uint8_t *addr); /* deprecated */
ssize_t load_image_size(const char *filename, void *addr, size_t size);
int load_image_targphys(const char *filename, hwaddr,
uint64_t max_sz);
int load_image_mr(const char *filename, MemoryRegion *mr);
int load_image_gzipped_buffer(const char *filename, uint64_t max_sz,
uint8_t **buffer);
int load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz);
void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp);
int load_aout(const char *filename, hwaddr addr, int max_sz,
int bswap_needed, hwaddr target_page_size);
int load_uimage(const char *filename, hwaddr *ep,
hwaddr *loadaddr, int *is_linux,
uint64_t (*translate_fn)(void *, uint64_t),
void *translate_opaque);
int load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz);
If I change all those to add an Error **errp, the ripple effect across the
rest of QEMU is pretty huge. So I decided it would be better to incrementally
convert stuff, starting with the rom_add_* functions first. Converting more
of the load_* functions can be a separate followup patch, since this one does
not make the situation worse for those.
> > @@ -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;
>
> This one still detects and passes on failure, but loses the error
> message. I guess that's okay, as long as this patch is incrementally
> better somewhere else.
See previous explanation.
> > @@ -847,8 +849,9 @@ 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_errno(errp, errno,
> > + "Could not open option rom '%s'",
> > + rom->path);
>
> would error_setg_file_open() be any better here, for consistency?
Sure.
>
> > +++ b/hw/i386/pc.c
> > @@ -1264,7 +1264,7 @@ 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, NULL);
>
> Another place that blindly ignores things; should we use &error_abort?
The xen_load_linux() function doesn't have any Error **Errp to propagate
the errors back up, so I left that unset. I guess error_abort is a valid
alternative, since this is in startup path, not hotplug.
> > +++ b/hw/i386/pc_sysfw.c
> > @@ -199,7 +199,7 @@ 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, NULL);
> > if (ret != 0) {
> > bios_error:
> > fprintf(stderr, "qemu: could not load PC BIOS '%s'\n", bios_name);
>
> This one makes sense - you are incrementally improving the interface,
> and not all callers; this caller was already reporting errors and could
> be cleaned up in a later commit to use &err instead of fprintf().
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-03-11 9:27 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-03-10 17:28 [Qemu-devel] [PATCH v1] hw: fix error reporting for missing option ROMs Daniel P. Berrange
2016-03-10 20:25 ` Eric Blake
2016-03-11 9:27 ` Daniel P. Berrange
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).