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

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

Hi Alistair,



On Fri, Oct 17, 2025 at 08:58:58AM +1000, Alistair Francis wrote:
> On Fri, Oct 17, 2025 at 4:01 AM Vishal Chourasia <vishalc@linux.ibm.com> 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>
> 
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> 
> Alistair
> 
> > ---
> >  hw/core/loader.c         | 33 +++++++++++++++++++++++++--------
<snipped>
> > diff --git a/hw/core/loader.c b/hw/core/loader.c
> > index 477661a025..63bb0578b1 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,21 +127,29 @@ 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 > max_sz) {
> > +        error_setg(errp, "%s exceeds maximum image size (%lu MiB)", filename, max_sz / MiB);
> > +        return -1;
> > +    }
> > +
> >      if (size > 0) {
> >          if (rom_add_file_fixed_as(filename, addr, -1, as) < 0) {
> >              return -1;


There was a case where load_image_targphys_as() can return -1 but errp
was not set. Following change addresses this issue.
I will incorporate this in the next version (v4).

 /* 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 > max_sz) {
+        error_setg(errp, "%s exceeds maximum image size (%lu MiB)",
+                   filename, 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;
         }
     }

Thanks,
vishalc