Re: [PATCH v11 09/27] migration: push Error **errp into qemu_loadvm_state_main()

[Arun Menon <armenon@redhat.com>]

Hi Fabiano,
Thanks for the review.
On Fri, Aug 15, 2025 at 04:23:42PM -0300, Fabiano Rosas wrote:
> Arun Menon <armenon@redhat.com> writes:
> 
> > 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 qemu_loadvm_state_main() must report an error
> > in errp, in case of failure.
> > loadvm_process_command also sets the errp object explicitly.
> >
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> > Signed-off-by: Arun Menon <armenon@redhat.com>
> > ---
> >  migration/colo.c   |  5 +++--
> >  migration/savevm.c | 36 +++++++++++++++++++-----------------
> >  migration/savevm.h |  3 ++-
> >  3 files changed, 24 insertions(+), 20 deletions(-)
> >
> > diff --git a/migration/colo.c b/migration/colo.c
> > index 0ba22ee76a13e313793f653f43a728e3c433bbc1..a96e4dba15516b71d1b315c736c3b4879ff04e58 100644
> > --- a/migration/colo.c
> > +++ b/migration/colo.c
> > @@ -659,6 +659,7 @@ void migrate_start_colo_process(MigrationState *s)
> >  static void colo_incoming_process_checkpoint(MigrationIncomingState *mis,
> >                        QEMUFile *fb, QIOChannelBuffer *bioc, Error **errp)
> >  {
> > +    ERRP_GUARD();
> 
> With my suggestion below, this goes away.
Yeah.
> 
> >      uint64_t total_size;
> >      uint64_t value;
> >      Error *local_err = NULL;
> > @@ -686,11 +687,11 @@ static void colo_incoming_process_checkpoint(MigrationIncomingState *mis,
> >  
> >      bql_lock();
> >      cpu_synchronize_all_states();
> > -    ret = qemu_loadvm_state_main(mis->from_src_file, mis);
> > +    ret = qemu_loadvm_state_main(mis->from_src_file, mis, errp);
> >      bql_unlock();
> >  
> >      if (ret < 0) {
> > -        error_setg(errp, "Load VM's live state (ram) error");
> > +        error_prepend(errp, "Load VM's live state (ram) error: ");
> 
> Another one to leave out. There's enough information downstream
> already. Also, this "(ram)" doesn't look right.
Yes, shall remove it.
> 
> >          return;
> >      }
> >  
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index 70e021597d884030c4a0dc2a7bc27d42a7371797..9ec07892cd6ea666431410657c840b6325377d97 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -2105,7 +2105,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
> >      qemu_file_set_blocking(f, true);
> >  
> >      /* TODO: sanity check that only postcopiable data will be loaded here */
> > -    load_res = qemu_loadvm_state_main(f, mis);
> > +    load_res = qemu_loadvm_state_main(f, mis, &error_fatal);
> >  
> >      /*
> >       * This is tricky, but, mis->from_src_file can change after it
> > @@ -2407,6 +2407,7 @@ static int loadvm_postcopy_handle_resume(MigrationIncomingState *mis)
> >   */
> >  static int loadvm_handle_cmd_packaged(MigrationIncomingState *mis, Error **errp)
> >  {
> > +    ERRP_GUARD();
> >      int ret;
> >      size_t length;
> >      QIOChannelBuffer *bioc;
> > @@ -2456,9 +2457,9 @@ static int loadvm_handle_cmd_packaged(MigrationIncomingState *mis, Error **errp)
> >          qemu_coroutine_yield();
> >      } while (1);
> >  
> > -    ret = qemu_loadvm_state_main(packf, mis);
> > +    ret = qemu_loadvm_state_main(packf, mis, errp);
> >      if (ret < 0) {
> > -        error_setg(errp, "VM state load failed: %d", ret);
> > +        error_prepend(errp, "Loading VM state failed: %d: ", ret);
> 
> This is getting out of hand for code review, may I suggest you
> artificially trigger these errors, look at the resulting message and
> remove all the unnecessary wrapping? Each error_prepend is a candidate
> for removal if it will just state "load failed".
> 
> Using error_prepend partly defeats the purpose of propagating errp. We
> should only use it when there's valuable information to be provided.
I understand. I shall remove error_prepend from where it is not required.
> 
> >      }
> >      trace_loadvm_handle_cmd_packaged_main(ret);
> >      qemu_fclose(packf);
> > @@ -3080,18 +3081,21 @@ static bool postcopy_pause_incoming(MigrationIncomingState *mis)
> >      return true;
> >  }
> >  
> > -int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
> > +int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis,
> > +                           Error **errp)
> >  {
> > +    ERRP_GUARD();
> >      uint8_t section_type;
> >      int ret = 0;
> > -    Error *local_err = NULL;
> >  
> >  retry:
> >      while (true) {
> >          section_type = qemu_get_byte(f);
> >  
> > -        ret = qemu_file_get_error_obj_any(f, mis->postcopy_qemufile_dst, NULL);
> > +        ret = qemu_file_get_error_obj_any(f, mis->postcopy_qemufile_dst, errp);
> >          if (ret) {
> > +            error_prepend(errp, "Failed to load device state section ID: %d: ",
> > +                          ret);
> 
> We could drop some extra words here, the term 'section' is already quite
> representative.
> 
> "Failed to load section ID: stream error: %d: "
Thanks, will do.
> 
> 
> >              break;
> >          }
> >  
> > @@ -3112,10 +3116,7 @@ retry:
> >              }
> >              break;
> >          case QEMU_VM_COMMAND:
> > -            ret = loadvm_process_command(f, &local_err);
> > -            if (ret < 0) {
> > -                warn_report_err(local_err);
> > -            }
> > +            ret = loadvm_process_command(f, errp);
> 
> Good.
> 
> >              trace_qemu_loadvm_state_section_command(ret);
> >              if ((ret < 0) || (ret == LOADVM_QUIT)) {
> >                  goto out;
> > @@ -3125,7 +3126,7 @@ retry:
> >              /* This is the end of migration */
> >              goto out;
> >          default:
> > -            error_report("Unknown savevm section type %d", section_type);
> > +            error_setg(errp, "Unknown savevm section type %d", section_type);
> 
> Not sure if they're referring to "savevm" here as a generic term for
> vmstate/migration or if it was intended to say: "savevm wrote a section
> type that this loadvm instance doesn't understand".
> 
> Since you're here, could you fix this? Migration errors from source and
> destination are often interleaved in logs, we don't want to see the
> "savevm" word in a destination-side error message. Just put a small note
> in the commit message, no need for another patch.
Yes, shall remove savevm and amend the commit message.
> 
> >              ret = -EINVAL;
> >              goto out;
> >          }
> > @@ -3133,6 +3134,9 @@ retry:
> >  
> >  out:
> >      if (ret < 0) {
> > +        if (*errp == NULL) {
> > +            error_setg(errp, "Loading VM state failed: %d", ret);
> > +        }
> 
> Another candidate for removal, then we avoid having to dereference errp.
This is added so that errp is set in case of failure in paths that are not
converted yet in the series. The out label comes in handy.
This is removed in the future patch 4648a1849a
> 
> >          qemu_file_set_error(f, ret);
> >  
> >          /* Cancel bitmaps incoming regardless of recovery */
> > @@ -3153,6 +3157,7 @@ out:
> >              migrate_postcopy_ram() && postcopy_pause_incoming(mis)) {
> >              /* Reset f to point to the newly created channel */
> >              f = mis->from_src_file;
> > +            error_free_or_abort(errp);
> 
> What's this about?
The errp needs to be freed and reset to NULL if we go into retry and
intend to use the variable again.
> 
> >              goto retry;
> >          }
> >      }
> > @@ -3186,10 +3191,7 @@ int qemu_loadvm_state(QEMUFile *f, Error **errp)
> >  
> >      cpu_synchronize_all_pre_loadvm();
> >  
> > -    ret = qemu_loadvm_state_main(f, mis);
> > -    if (ret < 0) {
> > -        error_setg(errp, "Load VM state failed: %d", ret);
> > -    }
> > +    ret = qemu_loadvm_state_main(f, mis, errp);
> >      qemu_event_set(&mis->main_thread_load_event);
> >  
> >      trace_qemu_loadvm_state_post_main(ret);
> > @@ -3270,9 +3272,9 @@ int qemu_load_device_state(QEMUFile *f, Error **errp)
> >      int ret;
> >  
> >      /* Load QEMU_VM_SECTION_FULL section */
> > -    ret = qemu_loadvm_state_main(f, mis);
> > +    ret = qemu_loadvm_state_main(f, mis, errp);
> >      if (ret < 0) {
> > -        error_setg(errp, "Failed to load device state: %d", ret);
> > +        error_prepend(errp, "Failed to load device state: %d: ", ret);
Maybe this can also be removed.
> >          return ret;
> >      }
> >  
> > diff --git a/migration/savevm.h b/migration/savevm.h
> > index b12681839f0b1afa3255e45215d99c13a224b19f..c337e3e3d111a7f28a57b90f61e8f70b71803d4e 100644
> > --- a/migration/savevm.h
> > +++ b/migration/savevm.h
> > @@ -66,7 +66,8 @@ int qemu_save_device_state(QEMUFile *f);
> >  
> >  int qemu_loadvm_state(QEMUFile *f, Error **errp);
> >  void qemu_loadvm_state_cleanup(MigrationIncomingState *mis);
> > -int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis);
> > +int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis,
> > +                           Error **errp);
> >  int qemu_load_device_state(QEMUFile *f, Error **errp);
> >  int qemu_loadvm_approve_switchover(void);
> >  int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
> 

Regards,
Arun