Re: [PATCH v3 03/10] migration: Refactor error handling in source return path

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi Peter,

On 5/10/23 00:02, Peter Xu wrote:
> rp_state.error was a boolean used to show error happened in return path
> thread.  That's not only duplicating error reporting (migrate_set_error),
> but also not good enough in that we only do error_report() and set it to
> true, we never can keep a history of the exact error and show it in
> query-migrate.
> 
> To make this better, a few things done:
> 
>    - Use error_setg() rather than error_report() across the whole lifecycle
>      of return path thread, keeping the error in an Error*.
> 
>    - Use migrate_set_error() to apply that captured error to the global
>      migration object when error occured in this thread.
> 
>    - With above, no need to have mark_source_rp_bad(), remove it, alongside
>      with rp_state.error itself.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   migration/migration.h  |   1 -
>   migration/ram.h        |   5 +-
>   migration/migration.c  | 123 ++++++++++++++++++-----------------------
>   migration/ram.c        |  41 +++++++-------
>   migration/trace-events |   4 +-
>   5 files changed, 79 insertions(+), 95 deletions(-)


> -int ram_dirty_bitmap_reload(MigrationState *s, RAMBlock *block)
> +int ram_dirty_bitmap_reload(MigrationState *s, RAMBlock *block, Error **errp)
>   {
>       int ret = -EINVAL;
>       /* from_dst_file is always valid because we're within rp_thread */


> @@ -4193,16 +4194,16 @@ int ram_dirty_bitmap_reload(MigrationState *s, RAMBlock *block)
>   
>       ret = qemu_file_get_error(file);
>       if (ret || size != local_size) {
> -        error_report("%s: read bitmap failed for ramblock '%s': %d"
> -                     " (size 0x%"PRIx64", got: 0x%"PRIx64")",
> -                     __func__, block->idstr, ret, local_size, size);
> +        error_setg(errp, "read bitmap failed for ramblock '%s': %d"
> +                   " (size 0x%"PRIx64", got: 0x%"PRIx64")",
> +                   block->idstr, ret, local_size, size);
>           ret = -EIO;
>           goto out;
>       }
>   
>       if (end_mark != RAMBLOCK_RECV_BITMAP_ENDING) {
> -        error_report("%s: ramblock '%s' end mark incorrect: 0x%"PRIx64,
> -                     __func__, block->idstr, end_mark);
> +        error_setg(errp, "ramblock '%s' end mark incorrect: 0x%"PRIx64,
> +                   block->idstr, end_mark);
>           ret = -EINVAL;
>           goto out;
>       }

This function returns -EIO/-EINVAL errors, propagated to its 2 callers
  - migrate_handle_rp_recv_bitmap()
  - migrate_handle_rp_resume_ack()
which are only used in source_return_path_thread() where the return
value is only checked as boolean.

Could we simplify them returning a boolean (which is the pattern with
functions taking an Error** as last parameter)?

Regards,

Phil.