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

2016-03-11 Thread Daniel P. Berrange 
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 
> > ---
> >  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 _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 _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),

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

2016-03-10 Thread Eric Blake 
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 
> ---
>  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 _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 _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  instead of fprintf().


-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

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

2016-03-10 Thread Daniel P. Berrange 
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 
---
 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