Re: [Patch v4 1/5] hw/core/loader: capture Error from load_image_targphys

[Vishal Chourasia <vishalc@linux.ibm.com>]

On Fri, Oct 17, 2025 at 09:25:41PM +0200, BALATON Zoltan wrote:
> On Fri, 17 Oct 2025, Vishal Chourasia wrote:
> > Add Error **errp parameter to load_image_targphys(),
> > load_image_targphys_as(), and get_image_size() to enable better
> > error reporting when image loading fails.
> > 
> > Pass NULL for errp in all existing call sites to maintain current
> > behavior. No functional change intended in this patch.
> > 
> > Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Vishal Chourasia <vishalc@linux.ibm.com>
> > ---
> > hw/alpha/dp264.c         |  4 ++--
> > hw/arm/armv7m.c          |  2 +-
> > hw/arm/boot.c            |  5 +++--
> > hw/arm/digic_boards.c    |  2 +-
> > hw/arm/highbank.c        |  3 ++-
> > hw/arm/raspi.c           |  2 +-
> > hw/arm/vexpress.c        |  2 +-
> > hw/core/generic-loader.c |  3 ++-
> > hw/core/guest-loader.c   |  2 +-
> > hw/core/loader.c         | 40 ++++++++++++++++++++++++++++++++--------
> > hw/hppa/machine.c        |  5 +++--
> > hw/i386/multiboot.c      |  2 +-
> > hw/i386/x86-common.c     |  4 ++--
> > hw/ipmi/ipmi_bmc_sim.c   |  2 +-
> > hw/loongarch/boot.c      |  5 ++---
> > hw/m68k/an5206.c         |  2 +-
> > hw/m68k/mcf5208.c        |  4 ++--
> > hw/m68k/next-cube.c      |  2 +-
> > hw/m68k/q800.c           |  7 ++++---
> > hw/m68k/virt.c           |  4 ++--
> > hw/microblaze/boot.c     |  5 +++--
> > hw/mips/boston.c         |  2 +-
> > hw/mips/fuloong2e.c      |  9 +++++----
> > hw/mips/jazz.c           |  2 +-
> > hw/mips/loongson3_virt.c | 10 ++++++----
> > hw/mips/malta.c          |  9 +++++----
> > hw/nubus/nubus-device.c  |  2 +-
> > hw/openrisc/boot.c       |  5 +++--
> > hw/pci/pci.c             |  2 +-
> > hw/ppc/amigaone.c        |  4 ++--
> > hw/ppc/e500.c            |  5 +++--
> > hw/ppc/mac_newworld.c    |  9 ++++++---
> > hw/ppc/mac_oldworld.c    |  9 ++++++---
> > hw/ppc/pegasos2.c        |  5 +++--
> > hw/ppc/pnv.c             |  9 ++++++---
> > hw/ppc/ppc440_bamboo.c   |  3 ++-
> > hw/ppc/prep.c            |  8 +++++---
> > hw/ppc/sam460ex.c        |  3 ++-
> > hw/ppc/spapr.c           |  8 ++++----
> > hw/ppc/virtex_ml507.c    |  5 +++--
> > hw/riscv/boot.c          |  7 ++++---
> > hw/rx/rx-gdbsim.c        |  2 +-
> > hw/s390x/ipl.c           |  8 +++++---
> > hw/sh4/r2d.c             |  8 +++++---
> > hw/smbios/smbios.c       |  2 +-
> > hw/sparc/leon3.c         |  4 ++--
> > hw/sparc/sun4m.c         |  8 +++++---
> > hw/sparc64/sun4u.c       |  7 ++++---
> > hw/xtensa/xtfpga.c       |  3 ++-
> > include/hw/loader.h      |  8 +++++---
> > system/device_tree.c     |  2 +-
> > 51 files changed, 168 insertions(+), 107 deletions(-)
> [...]
> 
> > diff --git a/hw/core/loader.c b/hw/core/loader.c
> > index 477661a025..e40cba1b9f 100644
> > --- a/hw/core/loader.c
> > +++ b/hw/core/loader.c
> > @@ -48,6 +48,7 @@
> > #include "qapi/error.h"
> > #include "qapi/qapi-commands-machine.h"
> > #include "qapi/type-helpers.h"
> > +#include "qemu/units.h"
> > #include "trace.h"
> > #include "hw/hw.h"
> > #include "disas/disas.h"
> > @@ -61,23 +62,31 @@
> > #include "hw/nvram/fw_cfg.h"
> > #include "system/memory.h"
> > #include "hw/boards.h"
> > +#include "qapi/error.h"
> > #include "qemu/cutils.h"
> > #include "system/runstate.h"
> > #include "tcg/debuginfo.h"
> > 
> > +#include <errno.h>
> > #include <zlib.h>
> > 
> > static int roms_loaded;
> > 
> > /* return the size or -1 if error */
> > -int64_t get_image_size(const char *filename)
> > +int64_t get_image_size(const char *filename, Error **errp)
> > {
> >     int fd;
> >     int64_t size;
> >     fd = open(filename, O_RDONLY | O_BINARY);
> > -    if (fd < 0)
> > +    if (fd < 0) {
> > +        error_setg_file_open(errp, errno, filename);
> >         return -1;
> > +    }
> >     size = lseek(fd, 0, SEEK_END);
> > +    if (size < 0) {
> > +        error_setg_errno(errp, errno, "lseek failure: %s", filename);
> > +        return -1;
> > +    }
> >     close(fd);
> >     return size;
> > }
> > @@ -118,23 +127,38 @@ ssize_t read_targphys(const char *name,
> > }
> > 
> > ssize_t load_image_targphys(const char *filename,
> > -                            hwaddr addr, uint64_t max_sz)
> > +                            hwaddr addr, uint64_t max_sz, Error **errp)
> > {
> > -    return load_image_targphys_as(filename, addr, max_sz, NULL);
> > +    return load_image_targphys_as(filename, addr, max_sz, NULL, errp);
> > }
> > 
> > /* return the size or -1 if error */
> > ssize_t load_image_targphys_as(const char *filename,
> > -                               hwaddr addr, uint64_t max_sz, AddressSpace *as)
> > +                               hwaddr addr, uint64_t max_sz, AddressSpace *as,
> > +                               Error **errp)
> > {
> > +    ERRP_GUARD();
> >     ssize_t size;
> > 
> > -    size = get_image_size(filename);
> > -    if (size < 0 || size > max_sz) {
> > +    size = get_image_size(filename, errp);
> > +    if (*errp) {
> >         return -1;
> >     }
> > +
> > +    if (size == 0) {
> > +        error_setg(errp, "empty file");
> > +        return -1;
> > +    }
> > +
> > +    if (size > max_sz) {
> > +        error_setg(errp, "%s exceeds maximum image size (%" PRIu64 " MiB)",
> > +                   filename, (long unsigned int) max_sz / MiB);
> > +        return -1;
> > +    }
> > +
> >     if (size > 0) {
> >         if (rom_add_file_fixed_as(filename, addr, -1, as) < 0) {
> > +            error_setg(errp, "failed to add file as ROM");
> >             return -1;
> 
> These in this file are functional changes adding new messages so that
> contradicts the commmit message and maybe should be in its own patch for
> easier review.
Sure, I will split it into separate patch.

Thanks
vishalc