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

[BALATON Zoltan <balaton@eik.bme.hu>]

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@redhat.com>
>> ---
>
> Nice cleanup
>
>
> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>
>>   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