Re: [PATCH 05/33] migration: push Error **errp into qemu_loadvm_state_main()

[Daniel P . Berrangé]

On Mon, Feb 15, 2021 at 06:35:15PM +0000, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > This is an incremental step in converting vmstate loading code to report
> > via Error objects instead of printing directly to the console/monitor.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  migration/colo.c   |  3 +-
> >  migration/savevm.c | 73 +++++++++++++++++++++++++++++++---------------
> >  migration/savevm.h |  3 +-
> >  3 files changed, 52 insertions(+), 27 deletions(-)
> > 
> > diff --git a/migration/colo.c b/migration/colo.c
> > index e344b7cf32..4a050ac579 100644
> > --- a/migration/colo.c
> > +++ b/migration/colo.c
> > @@ -705,11 +705,10 @@ static void 
> > colo_incoming_process_checkpoint(MigrationIncomingState *mis,
> >  
> >      qemu_mutex_lock_iothread();
> >      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);
> >      qemu_mutex_unlock_iothread();
> >  
> >      if (ret < 0) {
> > -        error_setg(errp, "Load VM's live state (ram) error");
> >          return;
> >      }
> >  
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index dd41292d4e..e47aec435c 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -1819,6 +1819,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
> >      QEMUFile *f = mis->from_src_file;
> >      int load_res;
> >      MigrationState *migr = migrate_get_current();
> > +    Error *local_err = NULL;
> >  
> >      object_ref(OBJECT(migr));
> >  
> > @@ -1833,7 +1834,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
> >       * in qemu_file, and thus we must be blocking now.
> >       */
> >      qemu_file_set_blocking(f, true);
> > -    load_res = qemu_loadvm_state_main(f, mis);
> > +    load_res = qemu_loadvm_state_main(f, mis, &local_err);
> >  
> >      /*
> >       * This is tricky, but, mis->from_src_file can change after it
> > @@ -1849,6 +1850,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
> >      if (load_res < 0) {
> >          qemu_file_set_error(f, load_res);
> >          dirty_bitmap_mig_cancel_incoming();
> > +        error_report_err(local_err);
> >          if (postcopy_state_get() == POSTCOPY_INCOMING_RUNNING &&
> >              !migrate_postcopy_ram() && migrate_dirty_bitmaps())
> >          {
> > @@ -1859,12 +1861,10 @@ static void *postcopy_ram_listen_thread(void 
> > *opaque)
> >                           __func__, load_res);
> >              load_res = 0; /* prevent further exit() */
> >          } else {
> > -            error_report("%s: loadvm failed: %d", __func__, load_res);
> >              migrate_set_state(&mis->state, 
> > MIGRATION_STATUS_POSTCOPY_ACTIVE,
> >                                             MIGRATION_STATUS_FAILED);
> >          }
> > -    }
> > -    if (load_res >= 0) {
> > +    } else {
> >          /*
> >           * This looks good, but it's possible that the device loading in 
> > the
> >           * main thread hasn't finished yet, and so we might not be in 'RUN'
> > @@ -2116,14 +2116,17 @@ static int 
> > loadvm_postcopy_handle_resume(MigrationIncomingState *mis)
> >   * @mis: Incoming state
> >   * @length: Length of packaged data to read
> >   *
> > - * Returns: Negative values on error
> > - *
> > + * Returns:
> > + *   0: success
> > + *   LOADVM_QUIT: success, but stop
> > + *   -1: error
> >   */
> >  static int loadvm_handle_cmd_packaged(MigrationIncomingState *mis)
> >  {
> >      int ret;
> >      size_t length;
> >      QIOChannelBuffer *bioc;
> > +    Error *local_err = NULL;
> >  
> >      length = qemu_get_be32(mis->from_src_file);
> >      trace_loadvm_handle_cmd_packaged(length);
> > @@ -2149,8 +2152,11 @@ static int 
> > loadvm_handle_cmd_packaged(MigrationIncomingState *mis)
> >  
> >      QEMUFile *packf = qemu_fopen_channel_input(QIO_CHANNEL(bioc));
> >  
> > -    ret = qemu_loadvm_state_main(packf, mis);
> > +    ret = qemu_loadvm_state_main(packf, mis, &local_err);
> >      trace_loadvm_handle_cmd_packaged_main(ret);
> > +    if (ret < 0) {
> > +        error_report_err(local_err);
> > +    }
> >      qemu_fclose(packf);
> >      object_unref(OBJECT(bioc));
> >  
> > @@ -2568,7 +2574,14 @@ static bool 
> > postcopy_pause_incoming(MigrationIncomingState *mis)
> >      return true;
> >  }
> >  
> > -int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
> > +/*
> > + * Returns:
> > + *   0: success
> > + *   LOADVM_QUIT: success, but stop
> > + *   -1: error
> > + */
> > +int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis,
> > +                           Error **errp)
> >  {
> >      uint8_t section_type;
> >      int ret = 0;
> > @@ -2579,7 +2592,9 @@ retry:
> >  
> >          if (qemu_file_get_error(f)) {
> >              ret = qemu_file_get_error(f);
> > -            break;
> > +            error_setg(errp,
> > +                       "Failed to load device state section ID: %d", ret);
> 
> Can I ask why these don't use strerror(ret) ?
> 
> The test I'm running is, start a VM with an actual guest and a useful
> amount of ram:
> 
> ./x86_64-softmmu/qemu-system-x86_64 -M pc,accel=kvm -nographic -m 8G -drive 
> if=virtio,file=/home/vmimages/fedora-33-nest.qcow
> 
> ./x86_64-softmmu/qemu-system-x86_64 -M pc,accel=kvm -nographic -m 8G -drive 
> if=virtio,file=/home/vmimages/fedora-33-nest.qcow -incoming tcp:0:4444
> 
> source:
>   migrate_set_speed 1m
>   migrate -d tcp:0:4444
>   <Now quickly>
>   migrate_cancel
> 
> In the old world I get:
> qemu-system-x86_64: load of migration failed: Input/output error
> 
> In your world I get:
> qemu-system-x86_64: Failed to load device state section ID: -5
> 
> (5 being EIO)

Ok, so it looks like I do indeed need to pay more attention to
correctly using error_setg_errno() instead of error_setg(), as
Philippe suggested in the earlier patches.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|