1. Patchew
  2. QEMU
  3. [PATCH 0/6] error: Eliminate QERR_IO_ERROR
  4. View patch

[PATCH 4/6] cpus: Improve error messages on memsave, pmemsave write error

Markus Armbruster posted 6 patches 6 months, 2 weeks ago
Maintainers: Fam Zheng <fam@euphon.net>, Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Markus Armbruster <armbru@redhat.com>, Peter Xu <peterx@redhat.com>, Fabiano Rosas <farosas@suse.de>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>
[PATCH 4/6] cpus: Improve error messages on memsave, pmemsave write error
Posted by Markus Armbruster 6 months, 2 weeks ago 
qmp_memsave() and qmp_pmemsave() report fwrite() error as

    An IO error has occurred

Improve this to

    writing memory to '<filename>' failed

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 system/cpus.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/system/cpus.c b/system/cpus.c
index 68d161d96b..f8fa78f33d 100644
--- a/system/cpus.c
+++ b/system/cpus.c
@@ -813,7 +813,8 @@ void qmp_memsave(int64_t addr, int64_t size, const char *filename,
             goto exit;
         }
         if (fwrite(buf, 1, l, f) != l) {
-            error_setg(errp, QERR_IO_ERROR);
+            error_setg(errp, "writing memory to '%s' failed",
+                       filename);
             goto exit;
         }
         addr += l;
@@ -843,7 +844,8 @@ void qmp_pmemsave(int64_t addr, int64_t size, const char *filename,
             l = size;
         cpu_physical_memory_read(addr, buf, l);
         if (fwrite(buf, 1, l, f) != l) {
-            error_setg(errp, QERR_IO_ERROR);
+            error_setg(errp, "writing memory to '%s' failed",
+                       filename);
             goto exit;
         }
         addr += l;
-- 
2.45.0
Re: [PATCH 4/6] cpus: Improve error messages on memsave, pmemsave write error
Posted by Philippe Mathieu-Daudé 6 months, 2 weeks ago 
On 13/5/24 16:17, Markus Armbruster wrote:
> qmp_memsave() and qmp_pmemsave() report fwrite() error as
> 
>      An IO error has occurred
> 
> Improve this to
> 
>      writing memory to '<filename>' failed
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   system/cpus.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/system/cpus.c b/system/cpus.c
> index 68d161d96b..f8fa78f33d 100644
> --- a/system/cpus.c
> +++ b/system/cpus.c
> @@ -813,7 +813,8 @@ void qmp_memsave(int64_t addr, int64_t size, const char *filename,
>               goto exit;
>           }
>           if (fwrite(buf, 1, l, f) != l) {
> -            error_setg(errp, QERR_IO_ERROR);
> +            error_setg(errp, "writing memory to '%s' failed",
> +                       filename);
>               goto exit;
>           }
>           addr += l;
> @@ -843,7 +844,8 @@ void qmp_pmemsave(int64_t addr, int64_t size, const char *filename,
>               l = size;
>           cpu_physical_memory_read(addr, buf, l);
>           if (fwrite(buf, 1, l, f) != l) {
> -            error_setg(errp, QERR_IO_ERROR);
> +            error_setg(errp, "writing memory to '%s' failed",
> +                       filename);

What about including errno with error_setg_errno()?

>               goto exit;
>           }
>           addr += l;
Re: [PATCH 4/6] cpus: Improve error messages on memsave, pmemsave write error
Posted by Markus Armbruster 6 months, 2 weeks ago 
Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> On 13/5/24 16:17, Markus Armbruster wrote:
>> qmp_memsave() and qmp_pmemsave() report fwrite() error as
>>      An IO error has occurred
>> Improve this to
>>      writing memory to '<filename>' failed
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   system/cpus.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>> diff --git a/system/cpus.c b/system/cpus.c
>> index 68d161d96b..f8fa78f33d 100644
>> --- a/system/cpus.c
>> +++ b/system/cpus.c
>> @@ -813,7 +813,8 @@ void qmp_memsave(int64_t addr, int64_t size, const char *filename,
>>               goto exit;
>>           }
>>           if (fwrite(buf, 1, l, f) != l) {
>> -            error_setg(errp, QERR_IO_ERROR);
>> +            error_setg(errp, "writing memory to '%s' failed",
>> +                       filename);
>>               goto exit;
>>           }
>>           addr += l;
>> @@ -843,7 +844,8 @@ void qmp_pmemsave(int64_t addr, int64_t size, const char *filename,
>>               l = size;
>>           cpu_physical_memory_read(addr, buf, l);
>>           if (fwrite(buf, 1, l, f) != l) {
>> -            error_setg(errp, QERR_IO_ERROR);
>> +            error_setg(errp, "writing memory to '%s' failed",
>> +                       filename);
>
> What about including errno with error_setg_errno()?

Sure fwrite() fails with errno reliably set?  The manual page doesn't
mention it...


>>               goto exit;
>>           }
>>           addr += l;
Re: [PATCH 4/6] cpus: Improve error messages on memsave, pmemsave write error
Posted by Philippe Mathieu-Daudé 6 months, 2 weeks ago 
On 13/5/24 16:45, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
>> On 13/5/24 16:17, Markus Armbruster wrote:
>>> qmp_memsave() and qmp_pmemsave() report fwrite() error as
>>>       An IO error has occurred
>>> Improve this to
>>>       writing memory to '<filename>' failed
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>    system/cpus.c | 6 ++++--
>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>> diff --git a/system/cpus.c b/system/cpus.c
>>> index 68d161d96b..f8fa78f33d 100644
>>> --- a/system/cpus.c
>>> +++ b/system/cpus.c
>>> @@ -813,7 +813,8 @@ void qmp_memsave(int64_t addr, int64_t size, const char *filename,
>>>                goto exit;
>>>            }
>>>            if (fwrite(buf, 1, l, f) != l) {
>>> -            error_setg(errp, QERR_IO_ERROR);
>>> +            error_setg(errp, "writing memory to '%s' failed",
>>> +                       filename);
>>>                goto exit;
>>>            }
>>>            addr += l;
>>> @@ -843,7 +844,8 @@ void qmp_pmemsave(int64_t addr, int64_t size, const char *filename,
>>>                l = size;
>>>            cpu_physical_memory_read(addr, buf, l);
>>>            if (fwrite(buf, 1, l, f) != l) {
>>> -            error_setg(errp, QERR_IO_ERROR);
>>> +            error_setg(errp, "writing memory to '%s' failed",
>>> +                       filename);
>>
>> What about including errno with error_setg_errno()?
> 
> Sure fwrite() fails with errno reliably set?  The manual page doesn't
> mention it...

Indeed. I can see some uses in the code base:

qemu-io-cmds.c:409:    if (ferror(f)) {
qemu-io-cmds.c-410-        perror(file_name);

qga/commands-posix.c-632-    write_count = fwrite(buf, 1, count, fh);
qga/commands-posix.c:633:    if (ferror(fh)) {
qga/commands-posix.c-634-        error_setg_errno(errp, errno, "failed 
to write to file");

util/qemu-config.c:152:    if (ferror(fp)) {
util/qemu-config.c-153-        loc_pop(&loc);
util/qemu-config.c-154-        error_setg_errno(errp, errno, "Cannot 
read config file");

Regardless,

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Re: [PATCH 4/6] cpus: Improve error messages on memsave, pmemsave write error
Posted by Markus Armbruster 6 months ago 
Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> On 13/5/24 16:45, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>> 
>>> On 13/5/24 16:17, Markus Armbruster wrote:
>>>> qmp_memsave() and qmp_pmemsave() report fwrite() error as
>>>>
>>>>       An IO error has occurred
>>>>
>>>> Improve this to
>>>>
>>>>       writing memory to '<filename>' failed
>>>>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>> ---
>>>>    system/cpus.c | 6 ++++--
>>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/system/cpus.c b/system/cpus.c
>>>> index 68d161d96b..f8fa78f33d 100644
>>>> --- a/system/cpus.c
>>>> +++ b/system/cpus.c
>>>> @@ -813,7 +813,8 @@ void qmp_memsave(int64_t addr, int64_t size, const char *filename,
>>>>              goto exit;
>>>>          }
>>>>          if (fwrite(buf, 1, l, f) != l) {
>>>> -            error_setg(errp, QERR_IO_ERROR);
>>>> +            error_setg(errp, "writing memory to '%s' failed",
>>>> +                       filename);
>>>>              goto exit;
>>>>          }
>>>>          addr += l;
>>>> @@ -843,7 +844,8 @@ void qmp_pmemsave(int64_t addr, int64_t size, const char *filename,
>>>>              l = size;
>>>>          cpu_physical_memory_read(addr, buf, l);
>>>>          if (fwrite(buf, 1, l, f) != l) {
>>>> -            error_setg(errp, QERR_IO_ERROR);
>>>> +            error_setg(errp, "writing memory to '%s' failed",
>>>> +                       filename);
>>>
>>> What about including errno with error_setg_errno()?
>>
>> Sure fwrite() fails with errno reliably set?  The manual page doesn't
>> mention it...
>
> Indeed. I can see some uses in the code base:
>
> qemu-io-cmds.c:409:    if (ferror(f)) {
> qemu-io-cmds.c-410-        perror(file_name);

This is after fread(), which isn't specified to set errno, either.

> qga/commands-posix.c-632-    write_count = fwrite(buf, 1, count, fh);
> qga/commands-posix.c:633:    if (ferror(fh)) {
> qga/commands-posix.c-634-        error_setg_errno(errp, errno, "failed to write to file");

This one is after fwrite(), like the code I'm changing.

> util/qemu-config.c:152:    if (ferror(fp)) {
> util/qemu-config.c-153-        loc_pop(&loc);
> util/qemu-config.c-154-        error_setg_errno(errp, errno, "Cannot read config file");

This is after fgets(), which isn't specified to set errno, either.

All three uses feel iffy to me.  They work if the stream's error
indicator is clear before fread() / fwrite() / fgets(), and it is set
there, and the reason for it being set is something that sets errno
(such as a failed system call, which seems likely), and errno remains
untouched until after ferror().  Too much "if", "seems likely" for my
taste.

> Regardless,
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Thanks!

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.