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

[Eric Blake <eblake@redhat.com>]

[-- 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 --]