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

[Markus Armbruster]

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!