| Message ID | 20240513141703.549874-5-armbru@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | error: Eliminate QERR_IO_ERROR | expand |
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;
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;
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>
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!
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;
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(-)