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

[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 <address@hidden>
> > ---
> >  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 :|