Re: [PATCH v11 19/27] migration: push Error **errp into loadvm_handle_recv_bitmap()

[Fabiano Rosas <farosas@suse.de>]

Arun Menon <armenon@redhat.com> writes:

> This is an incremental step in converting vmstate loading
> code to report error via Error objects instead of directly
> printing it to console/monitor.
> It is ensured that loadvm_handle_recv_bitmap() must report an error
> in errp, in case of failure.
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
>  migration/savevm.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 9098c4bd3394d7b9ed77e20afbb26fd9c9be6550..a7aede1b3df9164e322e68f3889df7c4166876f5 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2480,32 +2480,35 @@ static int loadvm_handle_cmd_packaged(MigrationIncomingState *mis, Error **errp)
>   * len (1 byte) + ramblock_name (<255 bytes)
>   */
>  static int loadvm_handle_recv_bitmap(MigrationIncomingState *mis,
> -                                     uint16_t len)
> +                                     uint16_t len, Error **errp)
>  {
>      QEMUFile *file = mis->from_src_file;
>      RAMBlock *rb;
>      char block_name[256];
>      size_t cnt;
> +    int ret;
>  
>      cnt = qemu_get_counted_string(file, block_name);
>      if (!cnt) {
> -        error_report("%s: failed to read block name", __func__);
> +        error_setg(errp, "failed to read block name: %s", block_name);

Could we not print the buffer that's just failed to be written? As a
matter of principle =)

>          return -EINVAL;
>      }
>  
>      /* Validate before using the data */
> -    if (qemu_file_get_error(file)) {
> -        return qemu_file_get_error(file);
> +    ret = qemu_file_get_error(file);
> +    if (ret < 0) {
> +        error_setg(errp, "migration stream has error: %d", ret);

I've been suggesting "stream error:", probably best to keep it uniform.

> +        return ret;
>      }
>  
>      if (len != cnt + 1) {
> -        error_report("%s: invalid payload length (%d)", __func__, len);
> +        error_setg(errp, "invalid payload length (%d)", len);
>          return -EINVAL;
>      }
>  
>      rb = qemu_ram_block_by_name(block_name);
>      if (!rb) {
> -        error_report("%s: block '%s' not found", __func__, block_name);
> +        error_setg(errp, "block '%s' not found", block_name);
>          return -EINVAL;
>      }
>  
> @@ -2642,11 +2645,7 @@ static int loadvm_process_command(QEMUFile *f, Error **errp)
>          return 0;
>  
>      case MIG_CMD_RECV_BITMAP:
> -        ret = loadvm_handle_recv_bitmap(mis, len);
> -        if (ret < 0) {
> -            error_setg(errp, "Failed to load device state command: %d", ret);
> -        }
> -        return ret;
> +        return loadvm_handle_recv_bitmap(mis, len, errp);
>  
>      case MIG_CMD_ENABLE_COLO:
>          ret = loadvm_process_enable_colo(mis);