[PATCH 0/6] error: Eliminate QERR_IO_ERROR

[PATCH 0/6] error: Eliminate QERR_IO_ERROR

[Markus Armbruster]

Markus Armbruster (6):
  block: Improve error message when external snapshot can't flush
  dump/win_dump: Improve error messages on write error
  block/vmdk: Improve error messages on extent write error
  cpus: Improve error messages on memsave, pmemsave write error
  migration: Rephrase message on failure to save / load Xen device state
  qerror: QERR_IO_ERROR is no longer used, drop

 include/qapi/qmp/qerror.h |  3 ---
 block/vmdk.c              | 10 +++++-----
 blockdev.c                |  6 ++++--
 dump/win_dump.c           |  7 ++++---
 migration/savevm.c        |  5 ++---
 system/cpus.c             |  6 ++++--
 6 files changed, 19 insertions(+), 18 deletions(-)

-- 
2.45.0

[PATCH 1/6] block: Improve error message when external snapshot can't flush

[Markus Armbruster]

external_snapshot_action() reports bdrv_flush() failure to its caller
as

    An IO error has occurred

The errno code returned by bdrv_flush() is lost.

Improve this to

    Write to node '<device or node name>' failed: <description of errno>

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

diff --git a/blockdev.c b/blockdev.c
index 08eccc9052..528db3452f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1406,8 +1406,10 @@ static void external_snapshot_action(TransactionAction *action,
     }
 
     if (!bdrv_is_read_only(state->old_bs)) {
-        if (bdrv_flush(state->old_bs)) {
-            error_setg(errp, QERR_IO_ERROR);
+        ret = bdrv_flush(state->old_bs);
+        if (ret < 0) {
+            error_setg_errno(errp, -ret, "Write to node '%s' failed",
+                             bdrv_get_device_or_node_name(state->old_bs));
             return;
         }
     }
-- 
2.45.0

[PATCH 2/6] dump/win_dump: Improve error messages on write error

[Markus Armbruster]

create_win_dump() and write_run report qemu_write_full() failure to
their callers as

    An IO error has occurred

The errno set by qemu_write_full() is lost.

Improve this to

    win-dump: failed to write header: <description of errno>

and

    win-dump: failed to save memory: <description of errno>

This matches how dump.c reports similar errors.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 dump/win_dump.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/dump/win_dump.c b/dump/win_dump.c
index b7bfaff379..0e4fe692ce 100644
--- a/dump/win_dump.c
+++ b/dump/win_dump.c
@@ -12,7 +12,6 @@
 #include "sysemu/dump.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
-#include "qapi/qmp/qerror.h"
 #include "exec/cpu-defs.h"
 #include "hw/core/cpu.h"
 #include "qemu/win_dump_defs.h"
@@ -52,6 +51,7 @@ static size_t write_run(uint64_t base_page, uint64_t page_count,
     uint64_t addr = base_page << TARGET_PAGE_BITS;
     uint64_t size = page_count << TARGET_PAGE_BITS;
     uint64_t len, l;
+    int eno;
     size_t total = 0;
 
     while (size) {
@@ -65,9 +65,10 @@ static size_t write_run(uint64_t base_page, uint64_t page_count,
         }
 
         l = qemu_write_full(fd, buf, len);
+        eno = errno;
         cpu_physical_memory_unmap(buf, addr, false, len);
         if (l != len) {
-            error_setg(errp, QERR_IO_ERROR);
+            error_setg_errno(errp, eno, "win-dump: failed to save memory");
             return 0;
         }
 
@@ -459,7 +460,7 @@ void create_win_dump(DumpState *s, Error **errp)
 
     s->written_size = qemu_write_full(s->fd, h, hdr_size);
     if (s->written_size != hdr_size) {
-        error_setg(errp, QERR_IO_ERROR);
+        error_setg_errno(errp, errno, "win-dump: failed to write header");
         goto out_restore;
     }
 
-- 
2.45.0

[PATCH 3/6] block/vmdk: Improve error messages on extent write error

[Markus Armbruster]

vmdk_init_extent() reports blk_co_pwrite() failure to its caller as

    An IO error has occurred

The errno code returned by blk_co_pwrite() is lost.

Improve this to

    failed to write VMDK <what>: <description of errno>

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/vmdk.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 3b82979fdf..78f6433607 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -28,7 +28,6 @@
 #include "block/block_int.h"
 #include "sysemu/block-backend.h"
 #include "qapi/qmp/qdict.h"
-#include "qapi/qmp/qerror.h"
 #include "qemu/error-report.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
@@ -2278,12 +2277,12 @@ vmdk_init_extent(BlockBackend *blk, int64_t filesize, bool flat, bool compress,
     /* write all the data */
     ret = blk_co_pwrite(blk, 0, sizeof(magic), &magic, 0);
     if (ret < 0) {
-        error_setg(errp, QERR_IO_ERROR);
+        error_setg_errno(errp, -ret, "failed to write VMDK magic");
         goto exit;
     }
     ret = blk_co_pwrite(blk, sizeof(magic), sizeof(header), &header, 0);
     if (ret < 0) {
-        error_setg(errp, QERR_IO_ERROR);
+        error_setg_errno(errp, -ret, "failed to write VMDK header");
         goto exit;
     }
 
@@ -2303,7 +2302,7 @@ vmdk_init_extent(BlockBackend *blk, int64_t filesize, bool flat, bool compress,
     ret = blk_co_pwrite(blk, le64_to_cpu(header.rgd_offset) * BDRV_SECTOR_SIZE,
                         gd_buf_size, gd_buf, 0);
     if (ret < 0) {
-        error_setg(errp, QERR_IO_ERROR);
+        error_setg_errno(errp, -ret, "failed to write VMDK grain directory");
         goto exit;
     }
 
@@ -2315,7 +2314,8 @@ vmdk_init_extent(BlockBackend *blk, int64_t filesize, bool flat, bool compress,
     ret = blk_co_pwrite(blk, le64_to_cpu(header.gd_offset) * BDRV_SECTOR_SIZE,
                         gd_buf_size, gd_buf, 0);
     if (ret < 0) {
-        error_setg(errp, QERR_IO_ERROR);
+        error_setg_errno(errp, -ret,
+                         "failed to write VMDK backup grain directory");
     }
 
     ret = 0;
-- 
2.45.0

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

[Markus Armbruster]

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

[PATCH 5/6] migration: Rephrase message on failure to save / load Xen device state

[Markus Armbruster]

Functions that use an Error **errp parameter to return errors should
not also report them to the user, because reporting is the caller's
job.  When the caller does, the error is reported twice.  When it
doesn't (because it recovered from the error), there is no error to
report, i.e. the report is bogus.

qmp_xen_save_devices_state() and qmp_xen_load_devices_state() violate
this principle: they call qemu_save_device_state() and
qemu_loadvm_state(), which call error_report_err().

I wish I could clean this up now, but migration's error reporting is
too complicated (confused?) for me to mess with it.

Instead, I'm merely improving the error reported by
qmp_xen_load_devices_state() and qmp_xen_load_devices_state() to the
QMP core from

    An IO error has occurred

to
    saving Xen device state failed

and

    loading Xen device state failed

respectively.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 migration/savevm.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 4509482ec4..a4a856982a 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -45,7 +45,6 @@
 #include "qapi/qapi-commands-migration.h"
 #include "qapi/clone-visitor.h"
 #include "qapi/qapi-builtin-visit.h"
-#include "qapi/qmp/qerror.h"
 #include "qemu/error-report.h"
 #include "sysemu/cpus.h"
 #include "exec/memory.h"
@@ -3208,7 +3207,7 @@ void qmp_xen_save_devices_state(const char *filename, bool has_live, bool live,
     object_unref(OBJECT(ioc));
     ret = qemu_save_device_state(f);
     if (ret < 0 || qemu_fclose(f) < 0) {
-        error_setg(errp, QERR_IO_ERROR);
+        error_setg(errp, "saving Xen device state failed");
     } else {
         /* libxl calls the QMP command "stop" before calling
          * "xen-save-devices-state" and in case of migration failure, libxl
@@ -3257,7 +3256,7 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp)
     ret = qemu_loadvm_state(f);
     qemu_fclose(f);
     if (ret < 0) {
-        error_setg(errp, QERR_IO_ERROR);
+        error_setg(errp, "loading Xen device state failed");
     }
     migration_incoming_state_destroy();
 }
-- 
2.45.0

[PATCH 6/6] qerror: QERR_IO_ERROR is no longer used, drop

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qapi/qmp/qerror.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index 00b18e9082..bc9116f76a 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -20,9 +20,6 @@
 #define QERR_INVALID_PARAMETER_VALUE \
     "Parameter '%s' expects %s"
 
-#define QERR_IO_ERROR \
-    "An IO error has occurred"
-
 #define QERR_MISSING_PARAMETER \
     "Parameter '%s' is missing"
 
-- 
2.45.0

Re: [PATCH 2/6] dump/win_dump: Improve error messages on write error

[Philippe Mathieu-Daudé]

On 13/5/24 16:16, Markus Armbruster wrote:
> create_win_dump() and write_run report qemu_write_full() failure to
> their callers as
> 
>      An IO error has occurred
> 
> The errno set by qemu_write_full() is lost.
> 
> Improve this to
> 
>      win-dump: failed to write header: <description of errno>
> 
> and
> 
>      win-dump: failed to save memory: <description of errno>
> 
> This matches how dump.c reports similar errors.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   dump/win_dump.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/dump/win_dump.c b/dump/win_dump.c
> index b7bfaff379..0e4fe692ce 100644
> --- a/dump/win_dump.c
> +++ b/dump/win_dump.c
> @@ -12,7 +12,6 @@
>   #include "sysemu/dump.h"
>   #include "qapi/error.h"
>   #include "qemu/error-report.h"
> -#include "qapi/qmp/qerror.h"
>   #include "exec/cpu-defs.h"
>   #include "hw/core/cpu.h"
>   #include "qemu/win_dump_defs.h"
> @@ -52,6 +51,7 @@ static size_t write_run(uint64_t base_page, uint64_t page_count,
>       uint64_t addr = base_page << TARGET_PAGE_BITS;
>       uint64_t size = page_count << TARGET_PAGE_BITS;
>       uint64_t len, l;
> +    int eno;
>       size_t total = 0;
>   
>       while (size) {
> @@ -65,9 +65,10 @@ static size_t write_run(uint64_t base_page, uint64_t page_count,
>           }
>   
>           l = qemu_write_full(fd, buf, len);
> +        eno = errno;

Hmm this show the qemu_write_full() API isn't ideal.
Maybe we could pass &l as argument and return errno.
There are only 20 calls.

>           cpu_physical_memory_unmap(buf, addr, false, len);
>           if (l != len) {
> -            error_setg(errp, QERR_IO_ERROR);
> +            error_setg_errno(errp, eno, "win-dump: failed to save memory");
>               return 0;
>           }
>   
> @@ -459,7 +460,7 @@ void create_win_dump(DumpState *s, Error **errp)
>   
>       s->written_size = qemu_write_full(s->fd, h, hdr_size);
>       if (s->written_size != hdr_size) {
> -        error_setg(errp, QERR_IO_ERROR);
> +        error_setg_errno(errp, errno, "win-dump: failed to write header");
>           goto out_restore;
>       }
>   

Re: [PATCH 3/6] block/vmdk: Improve error messages on extent write error

[Philippe Mathieu-Daudé]

On 13/5/24 16:17, Markus Armbruster wrote:
> vmdk_init_extent() reports blk_co_pwrite() failure to its caller as
> 
>      An IO error has occurred
> 
> The errno code returned by blk_co_pwrite() is lost.
> 
> Improve this to
> 
>      failed to write VMDK <what>: <description of errno>
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   block/vmdk.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)

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

Re: [PATCH 5/6] migration: Rephrase message on failure to save / load Xen device state

[Philippe Mathieu-Daudé]

On 13/5/24 16:17, Markus Armbruster wrote:
> Functions that use an Error **errp parameter to return errors should
> not also report them to the user, because reporting is the caller's
> job.  When the caller does, the error is reported twice.  When it
> doesn't (because it recovered from the error), there is no error to
> report, i.e. the report is bogus.
> 
> qmp_xen_save_devices_state() and qmp_xen_load_devices_state() violate
> this principle: they call qemu_save_device_state() and
> qemu_loadvm_state(), which call error_report_err().
> 
> I wish I could clean this up now, but migration's error reporting is
> too complicated (confused?) for me to mess with it.
> 
> Instead, I'm merely improving the error reported by
> qmp_xen_load_devices_state() and qmp_xen_load_devices_state() to the
> QMP core from
> 
>      An IO error has occurred
> 
> to
>      saving Xen device state failed
> 
> and
> 
>      loading Xen device state failed
> 
> respectively.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   migration/savevm.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)

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

Re: [PATCH 1/6] block: Improve error message when external snapshot can't flush

[Philippe Mathieu-Daudé]

On 13/5/24 16:16, Markus Armbruster wrote:
> external_snapshot_action() reports bdrv_flush() failure to its caller
> as
> 
>      An IO error has occurred
> 
> The errno code returned by bdrv_flush() is lost.
> 
> Improve this to
> 
>      Write to node '<device or node name>' failed: <description of errno>
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   blockdev.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)

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

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

[Philippe Mathieu-Daudé]

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 6/6] qerror: QERR_IO_ERROR is no longer used, drop

[Philippe Mathieu-Daudé]

On 13/5/24 16:17, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   include/qapi/qmp/qerror.h | 3 ---
>   1 file changed, 3 deletions(-)

One less!

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

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: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 2/6] dump/win_dump: Improve error messages on write error

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> On 13/5/24 16:16, Markus Armbruster wrote:
>> create_win_dump() and write_run report qemu_write_full() failure to
>> their callers as
>>      An IO error has occurred
>> The errno set by qemu_write_full() is lost.
>> Improve this to
>>      win-dump: failed to write header: <description of errno>
>> and
>>      win-dump: failed to save memory: <description of errno>
>> This matches how dump.c reports similar errors.
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   dump/win_dump.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>> diff --git a/dump/win_dump.c b/dump/win_dump.c
>> index b7bfaff379..0e4fe692ce 100644
>> --- a/dump/win_dump.c
>> +++ b/dump/win_dump.c
>> @@ -12,7 +12,6 @@
>>   #include "sysemu/dump.h"
>>   #include "qapi/error.h"
>>   #include "qemu/error-report.h"
>> -#include "qapi/qmp/qerror.h"
>>   #include "exec/cpu-defs.h"
>>   #include "hw/core/cpu.h"
>>   #include "qemu/win_dump_defs.h"
>> @@ -52,6 +51,7 @@ static size_t write_run(uint64_t base_page, uint64_t page_count,
>>       uint64_t addr = base_page << TARGET_PAGE_BITS;
>>       uint64_t size = page_count << TARGET_PAGE_BITS;
>>       uint64_t len, l;
>> +    int eno;
>>       size_t total = 0;
>>         while (size) {
>> @@ -65,9 +65,10 @@ static size_t write_run(uint64_t base_page, uint64_t page_count,
>>           }
>>             l = qemu_write_full(fd, buf, len);
>> +        eno = errno;
>
> Hmm this show the qemu_write_full() API isn't ideal.
> Maybe we could pass &l as argument and return errno.
> There are only 20 calls.

qemu_write_full() is a drop-in replacement for write().

>>           cpu_physical_memory_unmap(buf, addr, false, len);
>>           if (l != len) {
>> -            error_setg(errp, QERR_IO_ERROR);
>> +            error_setg_errno(errp, eno, "win-dump: failed to save memory");
>>               return 0;
>>           }
>>   @@ -459,7 +460,7 @@ void create_win_dump(DumpState *s, Error **errp)
>>         s->written_size = qemu_write_full(s->fd, h, hdr_size);
>>       if (s->written_size != hdr_size) {
>> -        error_setg(errp, QERR_IO_ERROR);
>> +        error_setg_errno(errp, errno, "win-dump: failed to write header");
>>           goto out_restore;
>>       }
>>   

Re: [PATCH 2/6] dump/win_dump: Improve error messages on write error

[Philippe Mathieu-Daudé]

On 13/5/24 16:48, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
>> On 13/5/24 16:16, Markus Armbruster wrote:
>>> create_win_dump() and write_run report qemu_write_full() failure to
>>> their callers as
>>>       An IO error has occurred
>>> The errno set by qemu_write_full() is lost.
>>> Improve this to
>>>       win-dump: failed to write header: <description of errno>
>>> and
>>>       win-dump: failed to save memory: <description of errno>
>>> This matches how dump.c reports similar errors.
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>    dump/win_dump.c | 7 ++++---
>>>    1 file changed, 4 insertions(+), 3 deletions(-)
>>> diff --git a/dump/win_dump.c b/dump/win_dump.c
>>> index b7bfaff379..0e4fe692ce 100644
>>> --- a/dump/win_dump.c
>>> +++ b/dump/win_dump.c
>>> @@ -12,7 +12,6 @@
>>>    #include "sysemu/dump.h"
>>>    #include "qapi/error.h"
>>>    #include "qemu/error-report.h"
>>> -#include "qapi/qmp/qerror.h"
>>>    #include "exec/cpu-defs.h"
>>>    #include "hw/core/cpu.h"
>>>    #include "qemu/win_dump_defs.h"
>>> @@ -52,6 +51,7 @@ static size_t write_run(uint64_t base_page, uint64_t page_count,
>>>        uint64_t addr = base_page << TARGET_PAGE_BITS;
>>>        uint64_t size = page_count << TARGET_PAGE_BITS;
>>>        uint64_t len, l;
>>> +    int eno;
>>>        size_t total = 0;
>>>          while (size) {
>>> @@ -65,9 +65,10 @@ static size_t write_run(uint64_t base_page, uint64_t page_count,
>>>            }
>>>              l = qemu_write_full(fd, buf, len);
>>> +        eno = errno;
>>
>> Hmm this show the qemu_write_full() API isn't ideal.
>> Maybe we could pass &l as argument and return errno.
>> There are only 20 calls.
> 
> qemu_write_full() is a drop-in replacement for write().

Fine.

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

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

[Philippe Mathieu-Daudé]

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 5/6] migration: Rephrase message on failure to save / load Xen device state

[Fabiano Rosas]

Markus Armbruster <armbru@redhat.com> writes:

> Functions that use an Error **errp parameter to return errors should
> not also report them to the user, because reporting is the caller's
> job.  When the caller does, the error is reported twice.  When it
> doesn't (because it recovered from the error), there is no error to
> report, i.e. the report is bogus.
>
> qmp_xen_save_devices_state() and qmp_xen_load_devices_state() violate
> this principle: they call qemu_save_device_state() and
> qemu_loadvm_state(), which call error_report_err().
>
> I wish I could clean this up now, but migration's error reporting is
> too complicated (confused?) for me to mess with it.
>
> Instead, I'm merely improving the error reported by
> qmp_xen_load_devices_state() and qmp_xen_load_devices_state() to the
> QMP core from
>
>     An IO error has occurred
>
> to
>     saving Xen device state failed
>
> and
>
>     loading Xen device state failed
>
> respectively.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Acked-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH 5/6] migration: Rephrase message on failure to save / load Xen device state

[Peter Xu]

On Mon, May 13, 2024 at 04:17:02PM +0200, Markus Armbruster wrote:
> Functions that use an Error **errp parameter to return errors should
> not also report them to the user, because reporting is the caller's
> job.  When the caller does, the error is reported twice.  When it
> doesn't (because it recovered from the error), there is no error to
> report, i.e. the report is bogus.
> 
> qmp_xen_save_devices_state() and qmp_xen_load_devices_state() violate
> this principle: they call qemu_save_device_state() and
> qemu_loadvm_state(), which call error_report_err().
> 
> I wish I could clean this up now, but migration's error reporting is
> too complicated (confused?) for me to mess with it.

:-(

> 
> Instead, I'm merely improving the error reported by
> qmp_xen_load_devices_state() and qmp_xen_load_devices_state() to the
> QMP core from
> 
>     An IO error has occurred
> 
> to
>     saving Xen device state failed
> 
> and
> 
>     loading Xen device state failed
> 
> respectively.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Acked-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

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!

Re: [PATCH 5/6] migration: Rephrase message on failure to save / load Xen device state

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Mon, May 13, 2024 at 04:17:02PM +0200, Markus Armbruster wrote:
>> Functions that use an Error **errp parameter to return errors should
>> not also report them to the user, because reporting is the caller's
>> job.  When the caller does, the error is reported twice.  When it
>> doesn't (because it recovered from the error), there is no error to
>> report, i.e. the report is bogus.
>> 
>> qmp_xen_save_devices_state() and qmp_xen_load_devices_state() violate
>> this principle: they call qemu_save_device_state() and
>> qemu_loadvm_state(), which call error_report_err().
>> 
>> I wish I could clean this up now, but migration's error reporting is
>> too complicated (confused?) for me to mess with it.
>
> :-(

If I understood how it's *supposed* to work, I might have a chance...

I can see a mixture of reporting errors directly (with error_report() &
friends), passing them to callers (via Error **errp), and storing them
in / retrieving them from MigrationState member @error.  This can't be
right.

I think a necessary first step towards getting it right is a shared
understanding how errors are to be handled in migration code.  This
includes how error data should flow from error source to error sink, and
what the possible sinks are.

>> Instead, I'm merely improving the error reported by
>> qmp_xen_load_devices_state() and qmp_xen_load_devices_state() to the
>> QMP core from
>> 
>>     An IO error has occurred
>> 
>> to
>>     saving Xen device state failed
>> 
>> and
>> 
>>     loading Xen device state failed
>> 
>> respectively.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Acked-by: Peter Xu <peterx@redhat.com>

Thanks!

Re: [PATCH 5/6] migration: Rephrase message on failure to save / load Xen device state

[Peter Xu]

On Mon, May 27, 2024 at 12:53:22PM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Mon, May 13, 2024 at 04:17:02PM +0200, Markus Armbruster wrote:
> >> Functions that use an Error **errp parameter to return errors should
> >> not also report them to the user, because reporting is the caller's
> >> job.  When the caller does, the error is reported twice.  When it
> >> doesn't (because it recovered from the error), there is no error to
> >> report, i.e. the report is bogus.
> >> 
> >> qmp_xen_save_devices_state() and qmp_xen_load_devices_state() violate
> >> this principle: they call qemu_save_device_state() and
> >> qemu_loadvm_state(), which call error_report_err().
> >> 
> >> I wish I could clean this up now, but migration's error reporting is
> >> too complicated (confused?) for me to mess with it.
> >
> > :-(
> 
> If I understood how it's *supposed* to work, I might have a chance...
> 
> I can see a mixture of reporting errors directly (with error_report() &
> friends), passing them to callers (via Error **errp), and storing them
> in / retrieving them from MigrationState member @error.  This can't be
> right.
> 
> I think a necessary first step towards getting it right is a shared
> understanding how errors are to be handled in migration code.  This
> includes how error data should flow from error source to error sink, and
> what the possible sinks are.

True.  I think the sink should always be MigrationState.error so far.

There's also the other complexity on detecting errors using either
qemu_file_get_error() or migrate_get_error().. the error handling in
migration is indeed prone to a cleanup.

I've added a cleanup entry for migration todo page:

https://wiki.qemu.org/ToDo/LiveMigration#Migration_error_detection_and_reporting

Thanks,

-- 
Peter Xu