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(&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é 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(&loc); 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