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

[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_process_command() must report an error
> in errp, in case of failure.
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
>  migration/savevm.c | 82 +++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 60 insertions(+), 22 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 7f79461844105bf672314c3325caee9cdb654c27..715cc35394cac5fe225ef88cf448714a02321bd7 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2546,32 +2546,35 @@ static int loadvm_postcopy_handle_switchover_start(void)
>   * LOADVM_QUIT All good, but exit the loop
>   * <0          Error
>   */
> -static int loadvm_process_command(QEMUFile *f)
> +static int loadvm_process_command(QEMUFile *f, Error **errp)
>  {
>      MigrationIncomingState *mis = migration_incoming_get_current();
>      uint16_t cmd;
>      uint16_t len;
>      uint32_t tmp32;
> +    int ret;
>  
>      cmd = qemu_get_be16(f);
>      len = qemu_get_be16(f);
>  
>      /* Check validity before continue processing of cmds */
> -    if (qemu_file_get_error(f)) {
> -        return qemu_file_get_error(f);
> +    ret = qemu_file_get_error(f);
> +    if (ret) {
> +        error_setg(errp, "Failed to load VM process command: %d", ret);

"Failed to process command: stream error: %d"

> +        return ret;
>      }
>  
>      if (cmd >= MIG_CMD_MAX || cmd == MIG_CMD_INVALID) {
> -        error_report("MIG_CMD 0x%x unknown (len 0x%x)", cmd, len);
> +        error_setg(errp, "MIG_CMD 0x%x unknown (len 0x%x)", cmd, len);
>          return -EINVAL;
>      }
>  
>      trace_loadvm_process_command(mig_cmd_args[cmd].name, len);
>  
>      if (mig_cmd_args[cmd].len != -1 && mig_cmd_args[cmd].len != len) {
> -        error_report("%s received with bad length - expecting %zu, got %d",
> -                     mig_cmd_args[cmd].name,
> -                     (size_t)mig_cmd_args[cmd].len, len);
> +        error_setg(errp, "%s received with bad length - expecting %zu, got %d",
> +                   mig_cmd_args[cmd].name,
> +                   (size_t)mig_cmd_args[cmd].len, len);
>          return -ERANGE;
>      }
>  

Where's MIG_CMD_OPEN_RETURN_PATH?

> @@ -2594,11 +2597,10 @@ static int loadvm_process_command(QEMUFile *f)
>           * been created.
>           */
>          if (migrate_switchover_ack() && !mis->switchover_ack_pending_num) {
> -            int ret = migrate_send_rp_switchover_ack(mis);
> +            ret = migrate_send_rp_switchover_ack(mis);
>              if (ret) {
> -                error_report(
> -                    "Could not send switchover ack RP MSG, err %d (%s)", ret,
> -                    strerror(-ret));
> +                error_setg_errno(errp, -ret,
> +                                 "Could not send switchover ack RP MSG");
>                  return ret;
>              }
>          }
> @@ -2608,39 +2610,71 @@ static int loadvm_process_command(QEMUFile *f)
>          tmp32 = qemu_get_be32(f);
>          trace_loadvm_process_command_ping(tmp32);
>          if (!mis->to_src_file) {
> -            error_report("CMD_PING (0x%x) received with no return path",
> -                         tmp32);
> +            error_setg(errp, "CMD_PING (0x%x) received with no return path",
> +                       tmp32);
>              return -1;
>          }
>          migrate_send_rp_pong(mis, tmp32);
>          break;
>  
>      case MIG_CMD_PACKAGED:
> -        return loadvm_handle_cmd_packaged(mis);
> +        ret = loadvm_handle_cmd_packaged(mis);

I missed a lot of the discussion in this series, but I assume there's a
good reason to not put the conversion of each command first in the
series, so there's no need for temporary code in this patch.

> +        if (ret < 0) {
> +            error_setg(errp, "Failed to load device state command: %d", ret);
> +        }
> +        return ret;
>  
>      case MIG_CMD_POSTCOPY_ADVISE:
> -        return loadvm_postcopy_handle_advise(mis, len);
> +        ret = loadvm_postcopy_handle_advise(mis, len);
> +        if (ret < 0) {
> +            error_setg(errp, "Failed to load device state command: %d", ret);
> +        }
> +        return ret;
>  
>      case MIG_CMD_POSTCOPY_LISTEN:
> -        return loadvm_postcopy_handle_listen(mis);
> +        ret = loadvm_postcopy_handle_listen(mis);
> +        if (ret < 0) {
> +            error_setg(errp, "Failed to load device state command: %d", ret);
> +        }
> +        return ret;
>  
>      case MIG_CMD_POSTCOPY_RUN:
> -        return loadvm_postcopy_handle_run(mis);
> +        ret = loadvm_postcopy_handle_run(mis);
> +        if (ret < 0) {
> +            error_setg(errp, "Failed to load device state command: %d", ret);
> +        }
> +        return ret;
>  
>      case MIG_CMD_POSTCOPY_RAM_DISCARD:
> -        return loadvm_postcopy_ram_handle_discard(mis, len);
> +        ret = loadvm_postcopy_ram_handle_discard(mis, len);
> +        if (ret < 0) {
> +            error_setg(errp, "Failed to load device state command: %d", ret);
> +        }
> +        return ret;
>  
>      case MIG_CMD_POSTCOPY_RESUME:
>          return loadvm_postcopy_handle_resume(mis);
>  
>      case MIG_CMD_RECV_BITMAP:
> -        return loadvm_handle_recv_bitmap(mis, len);
> +        ret = loadvm_handle_recv_bitmap(mis, len);
> +        if (ret < 0) {
> +            error_setg(errp, "Failed to load device state command: %d", ret);
> +        }
> +        return ret;
>  
>      case MIG_CMD_ENABLE_COLO:
> -        return loadvm_process_enable_colo(mis);
> +        ret = loadvm_process_enable_colo(mis);
> +        if (ret < 0) {
> +            error_setg(errp, "Failed to load device state command: %d", ret);
> +        }
> +        return ret;
>  
>      case MIG_CMD_SWITCHOVER_START:
> -        return loadvm_postcopy_handle_switchover_start();
> +        ret = loadvm_postcopy_handle_switchover_start();
> +        if (ret < 0) {
> +            error_setg(errp, "Failed to load device state command: %d", ret);
> +        }
> +        return ret;
>      }
>  
>      return 0;
> @@ -3051,6 +3085,7 @@ int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
>  {
>      uint8_t section_type;
>      int ret = 0;
> +    Error *local_err = NULL;
>  
>  retry:
>      while (true) {
> @@ -3078,7 +3113,10 @@ retry:
>              }
>              break;
>          case QEMU_VM_COMMAND:
> -            ret = loadvm_process_command(f);
> +            ret = loadvm_process_command(f, &local_err);
> +            if (ret < 0) {
> +                warn_report_err(local_err);

Again, some throwaway code here. Commit message could have made this
clear: "For now, report the error with warn_report until all callers
have been converted to pass errp".

> +            }
>              trace_qemu_loadvm_state_section_command(ret);
>              if ((ret < 0) || (ret == LOADVM_QUIT)) {
>                  goto out;