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 <ebl...@redhat.com>
> Signed-off-by: Daniel P. Berrange <berra...@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);
[...]
--
Eduardo
