Re: [PATCH v7 09/24] migration: push Error **errp into loadvm_process_command()

["Marc-André Lureau" <marcandre.lureau@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6589 bytes --]

Hi

On Fri, Jul 25, 2025 at 4:20 PM Arun Menon <armenon@redhat.com> wrote:

> 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.
>
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
>  migration/savevm.c | 73
> +++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 53 insertions(+), 20 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index
> ad96da3651b89023e4b70ffeecab46d176bae6f5..d40b25d74be46b209be8f28d10b7538a5ff2e3dd
> 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2544,12 +2544,13 @@ 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)
>  {
>

better with ERRP_GUARD


>      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);
> @@ -2560,16 +2561,16 @@ static int loadvm_process_command(QEMUFile *f)
>      }
>
>      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;
>      }
>
> @@ -2588,11 +2589,11 @@ 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(errp,
> +                           "Could not send switchover ack RP MSG, err %d
> (%s)",
> +                           ret, strerror(-ret));
>

You could switch to using "error_setg_errno(errp, -ret.."  instead.

                 return ret;
>              }
>          }
> @@ -2602,40 +2603,72 @@ 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);
> +        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:
>          loadvm_postcopy_handle_resume(mis);
>          return 0;
>
>      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;
> @@ -3075,7 +3108,7 @@ retry:
>              }
>              break;
>          case QEMU_VM_COMMAND:
> -            ret = loadvm_process_command(f);
> +            ret = loadvm_process_command(f, NULL);
>              trace_qemu_loadvm_state_section_command(ret);
>              if ((ret < 0) || (ret == LOADVM_QUIT)) {
>                  goto out;
>
> --
> 2.50.0
>


Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

[-- Attachment #2: Type: text/html, Size: 8515 bytes --]