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

[Arun Menon <armenon@redhat.com>]

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 <armenon@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