Re: [PATCH 4/6] cpus: Improve error messages on memsave, pmemsave write error
Philippe Mathieu-Daudé writes:
> On 13/5/24 16:45, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé 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 '' failed
Signed-off-by: Markus Armbruster
---
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();
> 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é
Thanks!
Re: [PATCH 4/6] cpus: Improve error messages on memsave, pmemsave write error
On 13/5/24 16:45, Markus Armbruster wrote:
Philippe Mathieu-Daudé 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 '' failed
Signed-off-by: Markus Armbruster
---
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();
util/qemu-config.c-154-error_setg_errno(errp, errno, "Cannot
read config file");
Regardless,
Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH 4/6] cpus: Improve error messages on memsave, pmemsave write error
Philippe Mathieu-Daudé 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 '' failed
>> Signed-off-by: Markus Armbruster
>> ---
>> 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
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 '' failed
Signed-off-by: Markus Armbruster
---
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;
[PATCH 4/6] cpus: Improve error messages on memsave, pmemsave write error
qmp_memsave() and qmp_pmemsave() report fwrite() error as
An IO error has occurred
Improve this to
writing memory to '' failed
Signed-off-by: Markus Armbruster
---
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
