Re: [PATCH v2] hw/ppc/pnv: Improve kernel/initrd load failure error messages

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

Hello Daniel,

Thank you for the review!

On Wed, Oct 15, 2025 at 03:02:27PM +0100, Daniel P. Berrangé wrote:
> On Wed, Oct 15, 2025 at 07:17:17PM +0530, Vishal Chourasia wrote:
> > diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
> > index e72bbde2a2..8cf5aadf1f 100644
> > --- a/hw/core/generic-loader.c
> > +++ b/hw/core/generic-loader.c
> > @@ -148,13 +148,14 @@ static void generic_loader_realize(DeviceState *dev, Error **errp)
> >  
> >          if (size < 0 || s->force_raw) {
> >              /* Default to the maximum size being the machine's ram size */
> > -            size = load_image_targphys_as(s->file, s->addr, current_machine->ram_size, as);
> > +            size = load_image_targphys_as(s->file, s->addr,
> > +                    current_machine->ram_size, as, errp);
> >          } else {
> >              s->addr = entry;
> >          }
> >  
> > -        if (size < 0) {
> > -            error_setg(errp, "Cannot load specified image %s", s->file);
> > +        if (*errp || size < 0) {
> 
> We should not have to check both *errp and 'size < 0'.
Sure, I will remove the `*errp`

> We must ensure that every code path in 'load_image_targphys_as' that can
> return -1, will *always* fills in 'errp', so that callers can be sure
> that *errp is always non-NULL on failure.
> 

> > +            error_reportf_err(*errp, "Cannot load specified image %s", s->file);
> 
> This method is propagating the error to the caller in its 'errp'
> parameter, so it is wrong to call error_reportf_err. The latter
> should only be used at the final point in the callstack which
> owns the 'Error' parameter.
> 
> The only change in this method should be to remove the existing
> error_setg call.
Agreed. Removing it altogether.

> > diff --git a/hw/core/guest-loader.c b/hw/core/guest-loader.c
> > index 3db89d7a2e..d4f749fd6e 100644
> > --- a/hw/core/guest-loader.c
> > +++ b/hw/core/guest-loader.c
> > @@ -101,9 +101,9 @@ static void guest_loader_realize(DeviceState *dev, Error **errp)
> >  
> >      /* Default to the maximum size being the machine's ram size */
> >      size = load_image_targphys_as(file, s->addr, current_machine->ram_size,
> > -                                  NULL);
> > -    if (size < 0) {
> > -        error_setg(errp, "Cannot load specified image %s", file);
> > +                                  NULL, errp);
> > +    if (*errp || size < 0) {
> > +        error_reportf_err(*errp, "Cannot load specified image %s", file);
> >          return;
> 
> Again must not be calling error_reportf_err nor chcking *errp,
> just remove error_setg().
Will remove.

> >  
> > diff --git a/hw/core/loader.c b/hw/core/loader.c
> > index 477661a025..d8c02786d2 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;
> > +    }
> 
> This perhaps ought to be changed to call 'qemu_open' which
> already fills in an Error object, and additionally protects
> the fd with O_CLOEXEC and handles FD passing with /dev/fdset
> 
Make sense, thanks for the suggestion. 

> >      size = lseek(fd, 0, SEEK_END);
> > +    if (size < 0) {
> > +        error_setg_errno(errp, errno, "lseek failure");
> > +        return -1;
> > +    }
> >      close(fd);
> >      return size;
> >  }
> > @@ -118,21 +127,28 @@ 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)
> >  {
> >      ssize_t size;
> >  
> > -    size = get_image_size(filename);
> > -    if (size < 0 || size > max_sz) {
> > +    size = get_image_size(filename, errp);
> > +    if (*errp || size < 0) {
> 
> Must not chck *errp, only 'size < 0'.
Sure. I will modify accordingly.

> 
> >          return -1;
> >      }
> > +
> > +    if (size > max_sz) {
> > +        error_setg(errp, "Exceeds maximum image size (%lu MiB)", max_sz / MiB);
> > +        return -1;
> > +    }
> > +
> >      if (size > 0) {
> >          if (rom_add_file_fixed_as(filename, addr, -1, as) < 0) {
> >              return -1;
> > @@ -150,7 +166,7 @@ ssize_t load_image_mr(const char *filename, MemoryRegion *mr)
> >          return -1;
> >      }
> >  
> > -    size = get_image_size(filename);
> > +    size = get_image_size(filename, NULL);
> >  
> >      if (size < 0 || size > memory_region_size(mr)) {
> >          return -1;
> 
> 
> I'd suggest that we add the Error parameter in one patch, making every
> caller pass NULL. Then a second patch update the callers to pass a
> non-NULL errp and use error_report_err to print details, ideally  for
> more than just the 1 ppc source file. 

sure.

Planning to make following changes as per your review.

diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
index 8cf5aadf1f..cd636d9d89 100644
--- a/hw/core/generic-loader.c
+++ b/hw/core/generic-loader.c
@@ -154,8 +154,7 @@ static void generic_loader_realize(DeviceState *dev, Error **errp)
             s->addr = entry;
         }

-        if (*errp || size < 0) {
-            error_reportf_err(*errp, "Cannot load specified image %s", s->file);
+        if (size < 0) {
             return;
         }
     }
diff --git a/hw/core/guest-loader.c b/hw/core/guest-loader.c
index d4f749fd6e..9722474480 100644
--- a/hw/core/guest-loader.c
+++ b/hw/core/guest-loader.c
@@ -102,8 +102,7 @@ static void guest_loader_realize(DeviceState *dev, Error **errp)
     /* Default to the maximum size being the machine's ram size */
     size = load_image_targphys_as(file, s->addr, current_machine->ram_size,
                                   NULL, errp);
-    if (*errp || size < 0) {
-        error_reportf_err(*errp, "Cannot load specified image %s", file);
+    if (size < 0) {
         return;
     }

diff --git a/hw/core/loader.c b/hw/core/loader.c
index d8c02786d2..68cf982efe 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -77,14 +77,13 @@ int64_t get_image_size(const char *filename, Error **errp)
 {
     int fd;
     int64_t size;
-    fd = open(filename, O_RDONLY | O_BINARY);
+    fd = qemu_open(filename, O_RDONLY | O_BINARY, errp);
     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");
+        error_setg_errno(errp, errno, "lseek failure: %s", filename);
         return -1;
     }
     close(fd);
@@ -140,12 +139,12 @@ ssize_t load_image_targphys_as(const char *filename,
     ssize_t size;

     size = get_image_size(filename, errp);
-    if (*errp || size < 0) {
+    if (size < 0) {
         return -1;
     }

     if (size > max_sz) {
-        error_setg(errp, "Exceeds maximum image size (%lu MiB)", max_sz / MiB);
+        error_setg(errp, "%s exceeds maximum image size (%lu MiB)", filename, max_sz / MiB);
         return -1;
     }

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index e293d2ef35..16f3802717 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1072,8 +1072,7 @@ static void pnv_init(MachineState *machine)
     fw_size = load_image_targphys(fw_filename, pnv->fw_load_addr, FW_MAX_SIZE,
                                     &errp);
     if (fw_size < 0) {
-        error_reportf_err(errp, "Could not load OPAL firmware '%s': ",
-                fw_filename);
+        error_report_err(errp);
         exit(1);
     }
     g_free(fw_filename);
@@ -1086,8 +1085,7 @@ static void pnv_init(MachineState *machine)
                                           KERNEL_LOAD_ADDR, KERNEL_MAX_SIZE,
                                           &errp);
         if (kernel_size < 0) {
-            error_reportf_err(errp, "Could not load kernel '%s': ",
-                    machine->kernel_filename);
+            error_report_err(errp);
             exit(1);
         }
     }
@@ -1098,8 +1096,7 @@ static void pnv_init(MachineState *machine)
         pnv->initrd_size = load_image_targphys(machine->initrd_filename,
                                   pnv->initrd_base, INITRD_MAX_SIZE, &errp);
         if (pnv->initrd_size < 0) {
-            error_reportf_err(errp, "Could not load initial ram disk '%s': ",
-                    machine->initrd_filename);
+            error_report_err(errp);
             exit(1);
         }
     }


- vishalc