Hi,
Thanks for the review.
On Mon, Jul 21, 2025 at 09:38:53PM +0900, Akihiko Odaki wrote:
> On 2025/07/21 20:29, Arun Menon 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 | 87
> > +++++++++++++++++++++++++++++++++++++++++-------------
> > 1 file changed, 66 insertions(+), 21 deletions(-)
> >
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index
> > 96af7b412f2ed43468f4bcac8b833cda223f8321..d8feb9e1599d019636cd400ee7ebe594df27bd1d
> > 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -2546,12 +2546,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)
> > {
> > 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);
> > @@ -2562,16 +2563,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;
> > }
> > @@ -2590,11 +2591,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(errp, "Could not send switchover ack "
> > + "RP MSG, err %d (%s)", ret, strerror(-ret));
> > return ret;
> > }
> > }
> > @@ -2604,39 +2604,84 @@ 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 -1;
>
> This "return -1" is extraneous.
>
> The error_setg() call is later replaced with "[PATCH v6 09/24] migration:
> push Error **errp into loadvm_handle_cmd_packaged()", but this "return -1"
> is simply removed in the patch. There is no need to add it in the first
> place.
Agreed. I will remove the return -1 here.
>
> > + }
> > + 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 -1;
> > + }
> > + 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 -1;
> > + }
> > + 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 -1;
> > + }
> > + 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 -1;
> > + }
> > + return ret;
> > case MIG_CMD_POSTCOPY_RESUME:
> > - return loadvm_postcopy_handle_resume(mis);
> > + ret = loadvm_postcopy_handle_resume(mis);
> > + if (ret < 0) {
> > + error_setg(errp, "Failed to load device state command: %d",
> > ret);
> > + return -1;
> > + }
> > + return ret;
> > 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 -1;
> > + }
> > + 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 -1;
> > + }
> > + 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 -1;
> > + }
> > + return ret;
> > }
> > return 0;
> > @@ -3074,7 +3119,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;
> >
>
Regards,
Arun
