Hi Marc-André,
Thanks for the review.
On Wed, Aug 06, 2025 at 12:31:31PM +0400, Marc-André Lureau wrote:
> Hi
>
> On Tue, Aug 5, 2025 at 10:28 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.
> >
> > Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> > Signed-off-by: Arun Menon <arme...@redhat.com>
> > ---
> > migration/savevm.c | 72
> > +++++++++++++++++++++++++++++++++++++++---------------
> > 1 file changed, 52 insertions(+), 20 deletions(-)
> >
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index
> > f37c4455dcf839d46f026fc7c7ff02e2dfffe7b4..cb673f43b174249ff1525dba41284de2e5a70735
> > 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;
> > }
> >
> > @@ -2594,11 +2595,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 +2608,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);
> > + 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;
> > @@ -3074,7 +3106,7 @@ retry:
> > }
> > break;
> > case QEMU_VM_COMMAND:
> > - ret = loadvm_process_command(f);
> > + ret = loadvm_process_command(f, NULL);
> >
>
> The function used to error_report(), you should pass &error_warn to keep
> reporting.
Yes, will do. Thanks
>
>
> > trace_qemu_loadvm_state_section_command(ret);
> > if ((ret < 0) || (ret == LOADVM_QUIT)) {
> > goto out;
> >
> > --
> > 2.50.1
> >
> >
Regards,
Arun
