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
