A bit of cleanup around Error

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

Posted by Markus Armbruster 2 months, 3 weeks ago

Signed-off-by: Markus Armbruster <armbru@redhat.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);
 }
 
 static void spike_board_init(MachineState *machine)
-- 
2.49.0

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

Posted by Zhao Liu 2 months, 2 weeks ago

On Wed, Nov 19, 2025 at 02:08:51PM +0100, Markus Armbruster wrote:
> Date: Wed, 19 Nov 2025 14:08:51 +0100
> From: Markus Armbruster <armbru@redhat.com>
> Subject: [PATCH 1/5] hw/core/loader: Make load_elf_hdr() return bool,
>  simplify caller
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.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(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

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

Posted by Daniel Henrique Barboza 2 months, 3 weeks ago


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);
>   }
>   
>   static void spike_board_init(MachineState *machine)

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

Posted by BALATON Zoltan 2 months, 2 weeks ago

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

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

Posted by Markus Armbruster 2 months, 2 weeks ago

BALATON Zoltan <balaton@eik.bme.hu> writes:

> 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>

[...]

>>> 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?

Palmer, Alistair, Daniel, got a preference?

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

Posted by Peter Xu 2 months, 3 weeks ago

On Wed, Nov 19, 2025 at 02:08:51PM +0100, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

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

Posted by Vladimir Sementsov-Ogievskiy 2 months, 3 weeks ago

On 19.11.25 16:08, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

> ---
>   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.

I don't really care, but IMO, it's obvious contract for bool+errp functions, not worth a comment.

>    */
> -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);

-- 
Best regards,
Vladimir

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

Posted by Markus Armbruster 2 months, 3 weeks ago

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> On 19.11.25 16:08, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>
>> ---
>>   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.
>
> I don't really care, but IMO, it's obvious contract for bool+errp functions, not worth a comment.

Nearby function comments all have a "Returns" sentence.  I try to blend
in :)

>>    */
>> -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);

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

Posted by Philippe Mathieu-Daudé 2 months, 3 weeks ago

On 19/11/25 20:12, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> On 19.11.25 16:08, Markus Armbruster wrote:
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>
>>> ---
>>>    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.
>>
>> I don't really care, but IMO, it's obvious contract for bool+errp functions, not worth a comment.
> 
> Nearby function comments all have a "Returns" sentence.  I try to blend
> in :)

New developers might just have to look at a particular API such this
loader one, without knowing the global errp contract. With that in
mind, I'll documents @return everywhere.

> 
>>>     */
>>> -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);
>

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

Posted by Richard Henderson 2 months, 3 weeks ago

On 11/19/25 14:08, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster<armbru@redhat.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(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~