Re: [PATCH 1/5] hw/core/loader: Make load_elf_hdr() return bool, simplify caller

[BALATON Zoltan <balaton@xxxxxxxxxx>]

On Thu, 20 Nov 2025, Daniel Henrique Barboza wrote:
> On 11/19/25 10:08 AM, Markus Armbruster wrote:
> > Signed-off-by: Markus Armbruster <armbru@xxxxxxxxxx>
> > ---
> Nice cleanup
>
>
> Reviewed-by: Daniel Henrique Barboza <dbarboza@xxxxxxxxxxxxxxxx>
>
> >   include/hw/loader.h |  4 +++-
> >   hw/arm/boot.c       |  6 +-----
> >   hw/core/loader.c    |  8 ++++++--
> >   hw/riscv/spike.c    | 10 +---------
> >   4 files changed, 11 insertions(+), 17 deletions(-)
> >
> > diff --git a/include/hw/loader.h b/include/hw/loader.h
> > index d035e72748..6f91703503 100644
> > --- a/include/hw/loader.h
> > +++ b/include/hw/loader.h
> > @@ -188,8 +188,10 @@ ssize_t load_elf(const char *filename,
> >    *
> >    * Inspect an ELF file's header. Read its full header contents into a
> >    * buffer and/or determine if the ELF is 64bit.
> > + *
> > + * Returns true on success, false on failure.
> >    */
> > -void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error 
> > **errp);
> > +bool load_elf_hdr(const char *filename, void *hdr, bool *is64, Error 
> > **errp);
> >     ssize_t load_aout(const char *filename, hwaddr addr, int max_sz,
> >                     bool big_endian, hwaddr target_page_size);
> > diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> > index b91660208f..06b303aab8 100644
> > --- a/hw/arm/boot.c
> > +++ b/hw/arm/boot.c
> > @@ -766,16 +766,12 @@ static ssize_t arm_load_elf(struct arm_boot_info 
> > *info, uint64_t *pentry,
> >       int data_swab = 0;
> >       int elf_data_order;
> >       ssize_t ret;
> > -    Error *err = NULL;
> >   -
> > -    load_elf_hdr(info->kernel_filename, &elf_header, &elf_is64, &err);
> > -    if (err) {
> > +    if (!load_elf_hdr(info->kernel_filename, &elf_header, &elf_is64, 
> > NULL)) {
> >           /*
> >            * If the file is not an ELF file we silently return.
> >            * The caller will fall back to try other formats.
> >            */
> > -        error_free(err);
> >           return -1;
> >       }
> >   diff --git a/hw/core/loader.c b/hw/core/loader.c
> > index 590c5b02aa..10687fe1c8 100644
> > --- a/hw/core/loader.c
> > +++ b/hw/core/loader.c
> > @@ -364,8 +364,9 @@ const char *load_elf_strerror(ssize_t error)
> >       }
> >   }
> >   -void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error 
> > **errp)
> > +bool load_elf_hdr(const char *filename, void *hdr, bool *is64, Error 
> > **errp)
> >   {
> > +    bool ok = false;
> >       int fd;
> >       uint8_t e_ident_local[EI_NIDENT];
> >       uint8_t *e_ident;
> > @@ -380,7 +381,7 @@ void load_elf_hdr(const char *filename, void *hdr, bool 
> > *is64, Error **errp)
> >       fd = open(filename, O_RDONLY | O_BINARY);
> >       if (fd < 0) {
> >           error_setg_errno(errp, errno, "Failed to open file: %s", 
> > filename);
> > -        return;
> > +        return false;
> >       }
> >       if (read(fd, hdr, EI_NIDENT) != EI_NIDENT) {
> >           error_setg_errno(errp, errno, "Failed to read file: %s", 
> > filename);
> > @@ -415,8 +416,11 @@ void load_elf_hdr(const char *filename, void *hdr, 
> > bool *is64, Error **errp)
> >           off += br;
> >       }
> >   +    ok = true;
> > +
> >   fail:
> >       close(fd);
> > +    return ok;
> >   }
> >     /* return < 0 if error, otherwise the number of bytes loaded in memory 
> > */
> > diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> > index b0bab3fe00..8531e1d121 100644
> > --- a/hw/riscv/spike.c
> > +++ b/hw/riscv/spike.c
> > @@ -180,15 +180,7 @@ static void create_fdt(SpikeState *s, const 
> > MemMapEntry *memmap,
> >     static bool spike_test_elf_image(char *filename)
> >   {
> > -    Error *err = NULL;
> > -
> > -    load_elf_hdr(filename, NULL, NULL, &err);
> > -    if (err) {
> > -        error_free(err);
> > -        return false;
> > -    } else {
> > -        return true;
> > -    }
> > +    return load_elf_hdr(filename, NULL, NULL, NULL);
Does it worth to keep this function or could just be inlined at the two 
callers now that it's equivalent with load_elf_hdr?
Regards,
BALATON Zoltan