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

[Eduardo Habkost]

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 <address@hidden>
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
>  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