Hi
On Fri, Jul 25, 2025 at 4:20 PM Arun Menon <arme...@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 <arme...@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.lur...@redhat.com>
