Add error checking for lseek() failure and provide better error
messages when image loading fails, including filenames and addresses.
Signed-off-by: Vishal Chourasia <vishalc@linux.ibm.com>
---
hw/core/loader.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 7aca4989ef..48dd4e7b33 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -79,6 +79,10 @@ int64_t get_image_size(const char *filename, Error **errp)
if (fd < 0)
return -1;
size = lseek(fd, 0, SEEK_END);
+ if (size < 0) {
+ error_setg_errno(errp, errno, "lseek failure: %s", filename);
+ return -1;
+ }
close(fd);
return size;
}
@@ -129,14 +133,24 @@ ssize_t load_image_targphys_as(const char *filename,
hwaddr addr, uint64_t max_sz, AddressSpace *as,
Error **errp)
{
+ ERRP_GUARD();
ssize_t size;
size = get_image_size(filename, errp);
- if (size < 0 || size > max_sz) {
+ if (*errp) {
+ return -1;
+ }
+
+ if (size > max_sz) {
+ error_setg(errp, "%s exceeds maximum image size (%" PRIu64 " MiB)",
+ filename, max_sz / MiB);
return -1;
}
+
if (size > 0) {
if (rom_add_file_fixed_as(filename, addr, -1, as) < 0) {
+ error_setg(errp, "could not load '%s' at %" HWADDR_PRIx,
+ filename, addr);
return -1;
}
}
--
2.51.0On Fri, 24 Oct 2025, Vishal Chourasia wrote:
> Add error checking for lseek() failure and provide better error
> messages when image loading fails, including filenames and addresses.
>
> Signed-off-by: Vishal Chourasia <vishalc@linux.ibm.com>
> ---
> hw/core/loader.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 7aca4989ef..48dd4e7b33 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -79,6 +79,10 @@ int64_t get_image_size(const char *filename, Error **errp)
> if (fd < 0)
> return -1;
> size = lseek(fd, 0, SEEK_END);
> + if (size < 0) {
> + error_setg_errno(errp, errno, "lseek failure: %s", filename);
> + return -1;
> + }
> close(fd);
> return size;
> }
> @@ -129,14 +133,24 @@ ssize_t load_image_targphys_as(const char *filename,
> hwaddr addr, uint64_t max_sz, AddressSpace *as,
> Error **errp)
> {
> + ERRP_GUARD();
> ssize_t size;
>
> size = get_image_size(filename, errp);
> - if (size < 0 || size > max_sz) {
> + if (*errp) {
> + return -1;
> + }
> +
> + if (size > max_sz) {
> + error_setg(errp, "%s exceeds maximum image size (%" PRIu64 " MiB)",
> + filename, max_sz / MiB);
MiB is arbitrary here. This function is used to load all kinds of images
such as ROMs which may be 64k-2MB or even executables in generic loader
that can be a few kilobytes. This might result in errors saying max size
is 0 MiB if the allowed size is less than a MiB (e.g. amigaone PROM_SIZE =
512 KiB) and integer division discards fractions. Do we have a function to
pretty print sizes or maybe this should be left as bytes or at most
kilobytes?
Regards,
BALATON Zoltan
> return -1;
> }
> +
> if (size > 0) {
> if (rom_add_file_fixed_as(filename, addr, -1, as) < 0) {
> + error_setg(errp, "could not load '%s' at %" HWADDR_PRIx,
> + filename, addr);
> return -1;
> }
> }
>
On Fri, 24 Oct 2025 at 12:25, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> On Fri, 24 Oct 2025, Vishal Chourasia wrote:
> > Add error checking for lseek() failure and provide better error
> > messages when image loading fails, including filenames and addresses.
> >
> > Signed-off-by: Vishal Chourasia <vishalc@linux.ibm.com>
> > + if (size > max_sz) {
> > + error_setg(errp, "%s exceeds maximum image size (%" PRIu64 " MiB)",
> > + filename, max_sz / MiB);
>
> MiB is arbitrary here. This function is used to load all kinds of images
> such as ROMs which may be 64k-2MB or even executables in generic loader
> that can be a few kilobytes. This might result in errors saying max size
> is 0 MiB if the allowed size is less than a MiB (e.g. amigaone PROM_SIZE =
> 512 KiB) and integer division discards fractions. Do we have a function to
> pretty print sizes or maybe this should be left as bytes or at most
> kilobytes?
Yes, we have size_to_str() for this purpose.
thanks
-- PMM
On 24/10/25 17:48, Peter Maydell wrote:
> On Fri, 24 Oct 2025 at 12:25, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>> On Fri, 24 Oct 2025, Vishal Chourasia wrote:
>>> Add error checking for lseek() failure and provide better error
>>> messages when image loading fails, including filenames and addresses.
>>>
>>> Signed-off-by: Vishal Chourasia <vishalc@linux.ibm.com>
>>> + if (size > max_sz) {
>>> + error_setg(errp, "%s exceeds maximum image size (%" PRIu64 " MiB)",
>>> + filename, max_sz / MiB);
>> MiB is arbitrary here. This function is used to load all kinds of images
>> such as ROMs which may be 64k-2MB or even executables in generic loader
>> that can be a few kilobytes. This might result in errors saying max size
>> is 0 MiB if the allowed size is less than a MiB (e.g. amigaone PROM_SIZE =
>> 512 KiB) and integer division discards fractions. Do we have a function to
>> pretty print sizes or maybe this should be left as bytes or at most
>> kilobytes?
> Yes, we have size_to_str() for this purpose.
Thanks Peter, I will add it in next version.
vishalc
Hi Balaton, Philippe
On 24/10/25 16:55, BALATON Zoltan wrote:
> On Fri, 24 Oct 2025, Vishal Chourasia wrote:
>> Add error checking for lseek() failure and provide better error
>> messages when image loading fails, including filenames and addresses.
>>
>> Signed-off-by: Vishal Chourasia <vishalc@linux.ibm.com>
>> ---
>> hw/core/loader.c | 16 +++++++++++++++-
>> 1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/core/loader.c b/hw/core/loader.c
>> index 7aca4989ef..48dd4e7b33 100644
>> --- a/hw/core/loader.c
>> +++ b/hw/core/loader.c
>> @@ -79,6 +79,10 @@ int64_t get_image_size(const char *filename, Error
>> **errp)
>> if (fd < 0)
>> return -1;
>> size = lseek(fd, 0, SEEK_END);
>> + if (size < 0) {
>> + error_setg_errno(errp, errno, "lseek failure: %s", filename);
>> + return -1;
>> + }
>> close(fd);
>> return size;
>> }
>> @@ -129,14 +133,24 @@ ssize_t load_image_targphys_as(const char
>> *filename,
>> hwaddr addr, uint64_t max_sz,
>> AddressSpace *as,
>> Error **errp)
>> {
>> + ERRP_GUARD();
>> ssize_t size;
>>
>> size = get_image_size(filename, errp);
>> - if (size < 0 || size > max_sz) {
>> + if (*errp) {
>> + return -1;
>> + }
>> +
>> + if (size > max_sz) {
>> + error_setg(errp, "%s exceeds maximum image size (%" PRIu64 "
>> MiB)",
>> + filename, max_sz / MiB);
>
> MiB is arbitrary here. This function is used to load all kinds of
> images such as ROMs which may be 64k-2MB or even executables in
> generic loader that can be a few kilobytes. This might result in
> errors saying max size is 0 MiB if the allowed size is less than a MiB
> (e.g. amigaone PROM_SIZE = 512 KiB) and integer division discards
> fractions. Do we have a function to pretty print sizes or maybe this
> should be left as bytes or at most kilobytes?
>
Yes, for images sizes less than a megabyte.
Perhaps we can leave it at Kilobytes
I will send out another version.
I will also address the *errp vs size<0 comment from Philippe.
Thanks,
vishalc
> Regards,
> BALATON Zoltan
>
>> return -1;
>> }
>> +
>> if (size > 0) {
>> if (rom_add_file_fixed_as(filename, addr, -1, as) < 0) {
>> + error_setg(errp, "could not load '%s' at %" HWADDR_PRIx,
>> + filename, addr);
>> return -1;
>> }
>> }
>>
On 24/10/25 14:11, Vishal Chourasia wrote:
> Hi Balaton, Philippe
>
> On 24/10/25 16:55, BALATON Zoltan wrote:
>> On Fri, 24 Oct 2025, Vishal Chourasia wrote:
>>> Add error checking for lseek() failure and provide better error
>>> messages when image loading fails, including filenames and addresses.
>>>
>>> Signed-off-by: Vishal Chourasia <vishalc@linux.ibm.com>
>>> ---
>>> hw/core/loader.c | 16 +++++++++++++++-
>>> 1 file changed, 15 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/core/loader.c b/hw/core/loader.c
>>> index 7aca4989ef..48dd4e7b33 100644
>>> --- a/hw/core/loader.c
>>> +++ b/hw/core/loader.c
>>> @@ -79,6 +79,10 @@ int64_t get_image_size(const char *filename, Error
>>> **errp)
>>> if (fd < 0)
>>> return -1;
>>> size = lseek(fd, 0, SEEK_END);
>>> + if (size < 0) {
>>> + error_setg_errno(errp, errno, "lseek failure: %s", filename);
>>> + return -1;
>>> + }
>>> close(fd);
>>> return size;
>>> }
>>> @@ -129,14 +133,24 @@ ssize_t load_image_targphys_as(const char
>>> *filename,
>>> hwaddr addr, uint64_t max_sz,
>>> AddressSpace *as,
>>> Error **errp)
>>> {
>>> + ERRP_GUARD();
>>> ssize_t size;
>>>
>>> size = get_image_size(filename, errp);
>>> - if (size < 0 || size > max_sz) {
>>> + if (*errp) {
>>> + return -1;
>>> + }
>>> +
>>> + if (size > max_sz) {
>>> + error_setg(errp, "%s exceeds maximum image size (%" PRIu64 "
>>> MiB)",
>>> + filename, max_sz / MiB);
>>
>> MiB is arbitrary here. This function is used to load all kinds of
>> images such as ROMs which may be 64k-2MB or even executables in
>> generic loader that can be a few kilobytes. This might result in
>> errors saying max size is 0 MiB if the allowed size is less than a MiB
>> (e.g. amigaone PROM_SIZE = 512 KiB) and integer division discards
>> fractions. Do we have a function to pretty print sizes or maybe this
>> should be left as bytes or at most kilobytes?
>>
> Yes, for images sizes less than a megabyte.
> Perhaps we can leave it at Kilobytes
>
> I will send out another version.
If so, then please use qemu_strtosz().
> I will also address the *errp vs size<0 comment from Philippe.
>
> Thanks,
> vishalc
>
>> Regards,
>> BALATON Zoltan
>>
>>> return -1;
>>> }
>>> +
>>> if (size > 0) {
>>> if (rom_add_file_fixed_as(filename, addr, -1, as) < 0) {
>>> + error_setg(errp, "could not load '%s' at %" HWADDR_PRIx,
>>> + filename, addr);
>>> return -1;
>>> }
>>> }
>>>
Hi Philippe,
On 24/10/25 18:17, Philippe Mathieu-Daudé wrote:
> On 24/10/25 14:11, Vishal Chourasia wrote:
>> Hi Balaton, Philippe
>>
>> On 24/10/25 16:55, BALATON Zoltan wrote:
>>> On Fri, 24 Oct 2025, Vishal Chourasia wrote:
>>>> Add error checking for lseek() failure and provide better error
>>>> messages when image loading fails, including filenames and addresses.
>>>>
>>>> Signed-off-by: Vishal Chourasia <vishalc@linux.ibm.com>
>>>> ---
>>>> hw/core/loader.c | 16 +++++++++++++++-
>>>> 1 file changed, 15 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/core/loader.c b/hw/core/loader.c
>>>> index 7aca4989ef..48dd4e7b33 100644
>>>> --- a/hw/core/loader.c
>>>> +++ b/hw/core/loader.c
>>>> @@ -79,6 +79,10 @@ int64_t get_image_size(const char *filename,
>>>> Error **errp)
>>>> if (fd < 0)
>>>> return -1;
>>>> size = lseek(fd, 0, SEEK_END);
>>>> + if (size < 0) {
>>>> + error_setg_errno(errp, errno, "lseek failure: %s", filename);
>>>> + return -1;
>>>> + }
>>>> close(fd);
>>>> return size;
>>>> }
>>>> @@ -129,14 +133,24 @@ ssize_t load_image_targphys_as(const char
>>>> *filename,
>>>> hwaddr addr, uint64_t max_sz,
>>>> AddressSpace *as,
>>>> Error **errp)
>>>> {
>>>> + ERRP_GUARD();
>>>> ssize_t size;
>>>>
>>>> size = get_image_size(filename, errp);
>>>> - if (size < 0 || size > max_sz) {
>>>> + if (*errp) {
>>>> + return -1;
>>>> + }
>>>> +
>>>> + if (size > max_sz) {
>>>> + error_setg(errp, "%s exceeds maximum image size (%" PRIu64
>>>> " MiB)",
>>>> + filename, max_sz / MiB);
>>>
>>> MiB is arbitrary here. This function is used to load all kinds of
>>> images such as ROMs which may be 64k-2MB or even executables in
>>> generic loader that can be a few kilobytes. This might result in
>>> errors saying max size is 0 MiB if the allowed size is less than a
>>> MiB (e.g. amigaone PROM_SIZE = 512 KiB) and integer division
>>> discards fractions. Do we have a function to pretty print sizes or
>>> maybe this should be left as bytes or at most kilobytes?
>>>
>> Yes, for images sizes less than a megabyte.
>> Perhaps we can leave it at Kilobytes
>>
>> I will send out another version.
>
> If so, then please use qemu_strtosz().
There is another function size_to_str() suggested by Peter I was
planning to use it, as it was doing the expected job.
It seems qemu_strtosz() converts size strings to bytes.
Thanks, vishalc
>
>> I will also address the *errp vs size<0 comment from Philippe.
>>
>> Thanks,
>> vishalc
>>
>>> Regards,
>>> BALATON Zoltan
>>>
>>>> return -1;
>>>> }
>>>> +
>>>> if (size > 0) {
>>>> if (rom_add_file_fixed_as(filename, addr, -1, as) < 0) {
>>>> + error_setg(errp, "could not load '%s' at %" HWADDR_PRIx,
>>>> + filename, addr);
>>>> return -1;
>>>> }
>>>> }
>>>>
>
On 24/10/25 14:58, Vishal Chourasia wrote:
> Hi Philippe,
>
> On 24/10/25 18:17, Philippe Mathieu-Daudé wrote:
>> On 24/10/25 14:11, Vishal Chourasia wrote:
>>> Hi Balaton, Philippe
>>>
>>> On 24/10/25 16:55, BALATON Zoltan wrote:
>>>> On Fri, 24 Oct 2025, Vishal Chourasia wrote:
>>>>> Add error checking for lseek() failure and provide better error
>>>>> messages when image loading fails, including filenames and addresses.
>>>>>
>>>>> Signed-off-by: Vishal Chourasia <vishalc@linux.ibm.com>
>>>>> ---
>>>>> hw/core/loader.c | 16 +++++++++++++++-
>>>>> 1 file changed, 15 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/core/loader.c b/hw/core/loader.c
>>>>> index 7aca4989ef..48dd4e7b33 100644
>>>>> --- a/hw/core/loader.c
>>>>> +++ b/hw/core/loader.c
>>>>> @@ -79,6 +79,10 @@ int64_t get_image_size(const char *filename,
>>>>> Error **errp)
>>>>> if (fd < 0)
>>>>> return -1;
>>>>> size = lseek(fd, 0, SEEK_END);
>>>>> + if (size < 0) {
>>>>> + error_setg_errno(errp, errno, "lseek failure: %s", filename);
>>>>> + return -1;
>>>>> + }
>>>>> close(fd);
>>>>> return size;
>>>>> }
>>>>> @@ -129,14 +133,24 @@ ssize_t load_image_targphys_as(const char
>>>>> *filename,
>>>>> hwaddr addr, uint64_t max_sz,
>>>>> AddressSpace *as,
>>>>> Error **errp)
>>>>> {
>>>>> + ERRP_GUARD();
>>>>> ssize_t size;
>>>>>
>>>>> size = get_image_size(filename, errp);
>>>>> - if (size < 0 || size > max_sz) {
>>>>> + if (*errp) {
>>>>> + return -1;
>>>>> + }
>>>>> +
>>>>> + if (size > max_sz) {
>>>>> + error_setg(errp, "%s exceeds maximum image size (%" PRIu64
>>>>> " MiB)",
>>>>> + filename, max_sz / MiB);
>>>>
>>>> MiB is arbitrary here. This function is used to load all kinds of
>>>> images such as ROMs which may be 64k-2MB or even executables in
>>>> generic loader that can be a few kilobytes. This might result in
>>>> errors saying max size is 0 MiB if the allowed size is less than a
>>>> MiB (e.g. amigaone PROM_SIZE = 512 KiB) and integer division
>>>> discards fractions. Do we have a function to pretty print sizes or
>>>> maybe this should be left as bytes or at most kilobytes?
>>>>
>>> Yes, for images sizes less than a megabyte.
>>> Perhaps we can leave it at Kilobytes
>>>
>>> I will send out another version.
>>
>> If so, then please use qemu_strtosz().
>
> There is another function size_to_str() suggested by Peter I was
> planning to use it, as it was doing the expected job.
>
> It seems qemu_strtosz() converts size strings to bytes.
Yes, they are the opposite ;)
On 24/10/25 18:52, Philippe Mathieu-Daudé wrote:
> On 24/10/25 14:58, Vishal Chourasia wrote:
>> Hi Philippe,
>>
>> On 24/10/25 18:17, Philippe Mathieu-Daudé wrote:
>>> On 24/10/25 14:11, Vishal Chourasia wrote:
>>>> Hi Balaton, Philippe
>>>>
>>>> On 24/10/25 16:55, BALATON Zoltan wrote:
>>>>> On Fri, 24 Oct 2025, Vishal Chourasia wrote:
>>>>>> Add error checking for lseek() failure and provide better error
>>>>>> messages when image loading fails, including filenames and
>>>>>> addresses.
>>>>>>
>>>>>> Signed-off-by: Vishal Chourasia <vishalc@linux.ibm.com>
>>>>>> ---
>>>>>> hw/core/loader.c | 16 +++++++++++++++-
>>>>>> 1 file changed, 15 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/hw/core/loader.c b/hw/core/loader.c
>>>>>> index 7aca4989ef..48dd4e7b33 100644
>>>>>> --- a/hw/core/loader.c
>>>>>> +++ b/hw/core/loader.c
>>>>>> @@ -79,6 +79,10 @@ int64_t get_image_size(const char *filename,
>>>>>> Error **errp)
>>>>>> if (fd < 0)
>>>>>> return -1;
>>>>>> size = lseek(fd, 0, SEEK_END);
>>>>>> + if (size < 0) {
>>>>>> + error_setg_errno(errp, errno, "lseek failure: %s",
>>>>>> filename);
>>>>>> + return -1;
>>>>>> + }
>>>>>> close(fd);
>>>>>> return size;
>>>>>> }
>>>>>> @@ -129,14 +133,24 @@ ssize_t load_image_targphys_as(const char
>>>>>> *filename,
>>>>>> hwaddr addr, uint64_t max_sz,
>>>>>> AddressSpace *as,
>>>>>> Error **errp)
>>>>>> {
>>>>>> + ERRP_GUARD();
>>>>>> ssize_t size;
>>>>>>
>>>>>> size = get_image_size(filename, errp);
>>>>>> - if (size < 0 || size > max_sz) {
>>>>>> + if (*errp) {
>>>>>> + return -1;
>>>>>> + }
>>>>>> +
>>>>>> + if (size > max_sz) {
>>>>>> + error_setg(errp, "%s exceeds maximum image size (%"
>>>>>> PRIu64 " MiB)",
>>>>>> + filename, max_sz / MiB);
>>>>>
>>>>> MiB is arbitrary here. This function is used to load all kinds of
>>>>> images such as ROMs which may be 64k-2MB or even executables in
>>>>> generic loader that can be a few kilobytes. This might result in
>>>>> errors saying max size is 0 MiB if the allowed size is less than a
>>>>> MiB (e.g. amigaone PROM_SIZE = 512 KiB) and integer division
>>>>> discards fractions. Do we have a function to pretty print sizes or
>>>>> maybe this should be left as bytes or at most kilobytes?
>>>>>
>>>> Yes, for images sizes less than a megabyte.
>>>> Perhaps we can leave it at Kilobytes
>>>>
>>>> I will send out another version.
>>>
>>> If so, then please use qemu_strtosz().
>>
>> There is another function size_to_str() suggested by Peter I was
>> planning to use it, as it was doing the expected job.
>>
>> It seems qemu_strtosz() converts size strings to bytes.
>
> Yes, they are the opposite ;)
:)
Hi Philippe,
I have sent out v10 in which I have used size_to_str() to pretty print
size information.
I have retained your RB, but now I am unsure if i should have not.
Can you review again perhaps
Thanks,
Vishal
On 24/10/25 11:26, Vishal Chourasia wrote:
> Add error checking for lseek() failure and provide better error
> messages when image loading fails, including filenames and addresses.
>
> Signed-off-by: Vishal Chourasia <vishalc@linux.ibm.com>
> ---
> hw/core/loader.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 7aca4989ef..48dd4e7b33 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -79,6 +79,10 @@ int64_t get_image_size(const char *filename, Error **errp)
> if (fd < 0)
> return -1;
> size = lseek(fd, 0, SEEK_END);
> + if (size < 0) {
> + error_setg_errno(errp, errno, "lseek failure: %s", filename);
> + return -1;
> + }
> close(fd);
> return size;
> }
> @@ -129,14 +133,24 @@ ssize_t load_image_targphys_as(const char *filename,
> hwaddr addr, uint64_t max_sz, AddressSpace *as,
> Error **errp)
> {
> + ERRP_GUARD();
> ssize_t size;
>
> size = get_image_size(filename, errp);
> - if (size < 0 || size > max_sz) {
> + if (*errp) {
Although safe with ERRP_GUARD(), we try to avoid checking *errp as
a pattern. I'll update (no need to repost) as:
if (size < 0) {
> + return -1;
> + }
else
> + if (size > max_sz) {
> + error_setg(errp, "%s exceeds maximum image size (%" PRIu64 " MiB)",
> + filename, max_sz / MiB);
> return -1;
> }
else
> if (size > 0) {
> if (rom_add_file_fixed_as(filename, addr, -1, as) < 0) {
> + error_setg(errp, "could not load '%s' at %" HWADDR_PRIx,
> + filename, addr);
> return -1;
> }
> }
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On 25/10/24 02:56PM, Vishal Chourasia wrote: > Add error checking for lseek() failure and provide better error > messages when image loading fails, including filenames and addresses. > > Signed-off-by: Vishal Chourasia <vishalc@linux.ibm.com> Reviewed-by: Aditya Gupta <adityag@linux.ibm.com> Thanks, - Aditya G
© 2016 - 2025 Red Hat, Inc.