diff mbox series

[01/33] migration: push Error **errp into qemu_loadvm_state()

Message ID 20210204171907.901471-2-berrange@redhat.com (mailing list archive)
State New, archived
Headers show
Series migration: capture error reports into Error object | expand

Commit Message

Daniel P. Berrangé Feb. 4, 2021, 5:18 p.m. UTC 
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/migration.c |  4 ++--
 migration/savevm.c    | 36 ++++++++++++++++++++----------------
 migration/savevm.h    |  2 +-
 3 files changed, 23 insertions(+), 19 deletions(-)

Comments

Philippe Mathieu-Daudé Feb. 4, 2021, 9:57 p.m. UTC | #1 
On 2/4/21 6:18 PM, Daniel P. Berrangé 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/migration.c |  4 ++--
>  migration/savevm.c    | 36 ++++++++++++++++++++----------------
>  migration/savevm.h    |  2 +-
>  3 files changed, 23 insertions(+), 19 deletions(-)
...

> diff --git a/migration/savevm.c b/migration/savevm.c
> index 6b320423c7..c8d93eee1e 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2638,40 +2638,49 @@ out:
>      return ret;
>  }
>  
> -int qemu_loadvm_state(QEMUFile *f)
> +int qemu_loadvm_state(QEMUFile *f, Error **errp)
>  {
>      MigrationIncomingState *mis = migration_incoming_get_current();
> -    Error *local_err = NULL;
>      int ret;
>  
> -    if (qemu_savevm_state_blocked(&local_err)) {
> -        error_report_err(local_err);
> -        return -EINVAL;
> +    if (qemu_savevm_state_blocked(errp)) {
> +        return -1;
>      }
>  
>      ret = qemu_loadvm_state_header(f);
>      if (ret) {
> -        return ret;
> +        error_setg(errp, "Error %d while loading VM state", ret);

Using error_setg_errno() instead (multiple occurences):
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +        return -1;
>      }
>
Daniel P. Berrangé Feb. 5, 2021, 9:33 a.m. UTC | #2 
On Thu, Feb 04, 2021 at 10:57:20PM +0100, Philippe Mathieu-Daudé wrote:
> On 2/4/21 6:18 PM, Daniel P. Berrangé 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/migration.c |  4 ++--
> >  migration/savevm.c    | 36 ++++++++++++++++++++----------------
> >  migration/savevm.h    |  2 +-
> >  3 files changed, 23 insertions(+), 19 deletions(-)
> ...
> 
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index 6b320423c7..c8d93eee1e 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -2638,40 +2638,49 @@ out:
> >      return ret;
> >  }
> >  
> > -int qemu_loadvm_state(QEMUFile *f)
> > +int qemu_loadvm_state(QEMUFile *f, Error **errp)
> >  {
> >      MigrationIncomingState *mis = migration_incoming_get_current();
> > -    Error *local_err = NULL;
> >      int ret;
> >  
> > -    if (qemu_savevm_state_blocked(&local_err)) {
> > -        error_report_err(local_err);
> > -        return -EINVAL;
> > +    if (qemu_savevm_state_blocked(errp)) {
> > +        return -1;
> >      }
> >  
> >      ret = qemu_loadvm_state_header(f);
> >      if (ret) {
> > -        return ret;
> > +        error_setg(errp, "Error %d while loading VM state", ret);
> 
> Using error_setg_errno() instead (multiple occurences):

I don't think we want todo that in general, because the code is
already not reliable at actually returning an errno value, sometimes
returning just "-1". At the end of this series it will almost always
be returning "-1", not an errno.  There are some places where an
errno is relevant though - specificially qemu_get_file_error calls.

> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 

Regards,
Daniel
Philippe Mathieu-Daudé Feb. 5, 2021, 9:35 a.m. UTC | #3 
On Fri, Feb 5, 2021 at 10:33 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Thu, Feb 04, 2021 at 10:57:20PM +0100, Philippe Mathieu-Daudé wrote:
> > On 2/4/21 6:18 PM, Daniel P. Berrangé 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/migration.c |  4 ++--
> > >  migration/savevm.c    | 36 ++++++++++++++++++++----------------
> > >  migration/savevm.h    |  2 +-
> > >  3 files changed, 23 insertions(+), 19 deletions(-)
> > ...
> >
> > > diff --git a/migration/savevm.c b/migration/savevm.c
> > > index 6b320423c7..c8d93eee1e 100644
> > > --- a/migration/savevm.c
> > > +++ b/migration/savevm.c
> > > @@ -2638,40 +2638,49 @@ out:
> > >      return ret;
> > >  }
> > >
> > > -int qemu_loadvm_state(QEMUFile *f)
> > > +int qemu_loadvm_state(QEMUFile *f, Error **errp)
> > >  {
> > >      MigrationIncomingState *mis = migration_incoming_get_current();
> > > -    Error *local_err = NULL;
> > >      int ret;
> > >
> > > -    if (qemu_savevm_state_blocked(&local_err)) {
> > > -        error_report_err(local_err);
> > > -        return -EINVAL;
> > > +    if (qemu_savevm_state_blocked(errp)) {
> > > +        return -1;
> > >      }
> > >
> > >      ret = qemu_loadvm_state_header(f);
> > >      if (ret) {
> > > -        return ret;
> > > +        error_setg(errp, "Error %d while loading VM state", ret);
> >
> > Using error_setg_errno() instead (multiple occurences):
>
> I don't think we want todo that in general, because the code is
> already not reliable at actually returning an errno value, sometimes
> returning just "-1". At the end of this series it will almost always
> be returning "-1", not an errno.  There are some places where an
> errno is relevant though - specificially qemu_get_file_error calls.

Fair. Ignore my other same comments in this. R-b tag stands.

>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >
>
> 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 :|
>
Daniel P. Berrangé March 11, 2021, 12:38 p.m. UTC | #4 
On Fri, Feb 05, 2021 at 10:35:28AM +0100, Philippe Mathieu-Daudé wrote:
> On Fri, Feb 5, 2021 at 10:33 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> > On Thu, Feb 04, 2021 at 10:57:20PM +0100, Philippe Mathieu-Daudé wrote:
> > > On 2/4/21 6:18 PM, Daniel P. Berrangé 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/migration.c |  4 ++--
> > > >  migration/savevm.c    | 36 ++++++++++++++++++++----------------
> > > >  migration/savevm.h    |  2 +-
> > > >  3 files changed, 23 insertions(+), 19 deletions(-)
> > > ...
> > >
> > > > diff --git a/migration/savevm.c b/migration/savevm.c
> > > > index 6b320423c7..c8d93eee1e 100644
> > > > --- a/migration/savevm.c
> > > > +++ b/migration/savevm.c
> > > > @@ -2638,40 +2638,49 @@ out:
> > > >      return ret;
> > > >  }
> > > >
> > > > -int qemu_loadvm_state(QEMUFile *f)
> > > > +int qemu_loadvm_state(QEMUFile *f, Error **errp)
> > > >  {
> > > >      MigrationIncomingState *mis = migration_incoming_get_current();
> > > > -    Error *local_err = NULL;
> > > >      int ret;
> > > >
> > > > -    if (qemu_savevm_state_blocked(&local_err)) {
> > > > -        error_report_err(local_err);
> > > > -        return -EINVAL;
> > > > +    if (qemu_savevm_state_blocked(errp)) {
> > > > +        return -1;
> > > >      }
> > > >
> > > >      ret = qemu_loadvm_state_header(f);
> > > >      if (ret) {
> > > > -        return ret;
> > > > +        error_setg(errp, "Error %d while loading VM state", ret);
> > >
> > > Using error_setg_errno() instead (multiple occurences):
> >
> > I don't think we want todo that in general, because the code is
> > already not reliable at actually returning an errno value, sometimes
> > returning just "-1". At the end of this series it will almost always
> > be returning "-1", not an errno.  There are some places where an
> > errno is relevant though - specificially qemu_get_file_error calls.
> 
> Fair. Ignore my other same comments in this. R-b tag stands.

On further investigation you are right. Not using error_setg_errno
has caused a regression in error quality as shown by Dave in a later
patch in this series.

Regards,
Daniel
diff mbox series

Patch

diff --git a/migration/migration.c b/migration/migration.c
index 1986cb8573..287a18d269 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -519,7 +519,7 @@  static void process_incoming_migration_co(void *opaque)
     postcopy_state_set(POSTCOPY_INCOMING_NONE);
     migrate_set_state(&mis->state, MIGRATION_STATUS_NONE,
                       MIGRATION_STATUS_ACTIVE);
-    ret = qemu_loadvm_state(mis->from_src_file);
+    ret = qemu_loadvm_state(mis->from_src_file, &local_err);
 
     ps = postcopy_state_get();
     trace_process_incoming_migration_co_end(ret, ps);
@@ -563,7 +563,7 @@  static void process_incoming_migration_co(void *opaque)
     }
 
     if (ret < 0) {
-        error_report("load of migration failed: %s", strerror(-ret));
+        error_report_err(local_err);
         goto fail;
     }
     mis->bh = qemu_bh_new(process_incoming_migration_bh, mis);
diff --git a/migration/savevm.c b/migration/savevm.c
index 6b320423c7..c8d93eee1e 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2638,40 +2638,49 @@  out:
     return ret;
 }
 
-int qemu_loadvm_state(QEMUFile *f)
+int qemu_loadvm_state(QEMUFile *f, Error **errp)
 {
     MigrationIncomingState *mis = migration_incoming_get_current();
-    Error *local_err = NULL;
     int ret;
 
-    if (qemu_savevm_state_blocked(&local_err)) {
-        error_report_err(local_err);
-        return -EINVAL;
+    if (qemu_savevm_state_blocked(errp)) {
+        return -1;
     }
 
     ret = qemu_loadvm_state_header(f);
     if (ret) {
-        return ret;
+        error_setg(errp, "Error %d while loading VM state", ret);
+        return -1;
     }
 
     if (qemu_loadvm_state_setup(f) != 0) {
-        return -EINVAL;
+        error_setg(errp, "Error %d while loading VM state", -EINVAL);
+        return -1;
     }
 
     cpu_synchronize_all_pre_loadvm();
 
     ret = qemu_loadvm_state_main(f, mis);
+    if (ret < 0) {
+        error_setg(errp, "Error %d while loading VM state", ret);
+        ret = -1;
+    }
     qemu_event_set(&mis->main_thread_load_event);
 
     trace_qemu_loadvm_state_post_main(ret);
 
     if (mis->have_listen_thread) {
+        error_setg(errp, "Error %d while loading VM state", ret);
         /* Listen thread still going, can't clean up yet */
         return ret;
     }
 
     if (ret == 0) {
         ret = qemu_file_get_error(f);
+        if (ret < 0) {
+            error_setg(errp, "Error %d while loading VM state", ret);
+            ret = -1;
+        }
     }
 
     /*
@@ -2690,8 +2699,8 @@  int qemu_loadvm_state(QEMUFile *f)
         uint8_t  section_type = qemu_get_byte(f);
 
         if (section_type != QEMU_VM_VMDESCRIPTION) {
-            error_report("Expected vmdescription section, but got %d",
-                         section_type);
+            error_setg(errp, "Expected vmdescription section, but got %d",
+                       section_type);
             /*
              * It doesn't seem worth failing at this point since
              * we apparently have an otherwise valid VM state
@@ -2921,7 +2930,6 @@  void qmp_xen_load_devices_state(const char *filename, Error **errp)
 {
     QEMUFile *f;
     QIOChannelFile *ioc;
-    int ret;
 
     /* Guest must be paused before loading the device state; the RAM state
      * will already have been loaded by xc
@@ -2940,11 +2948,8 @@  void qmp_xen_load_devices_state(const char *filename, Error **errp)
     f = qemu_fopen_channel_input(QIO_CHANNEL(ioc));
     object_unref(OBJECT(ioc));
 
-    ret = qemu_loadvm_state(f);
+    qemu_loadvm_state(f, errp);
     qemu_fclose(f);
-    if (ret < 0) {
-        error_setg(errp, QERR_IO_ERROR);
-    }
     migration_incoming_state_destroy();
 }
 
@@ -3018,14 +3023,13 @@  bool load_snapshot(const char *name, const char *vmstate,
         goto err_drain;
     }
     aio_context_acquire(aio_context);
-    ret = qemu_loadvm_state(f);
+    ret = qemu_loadvm_state(f, errp);
     migration_incoming_state_destroy();
     aio_context_release(aio_context);
 
     bdrv_drain_all_end();
 
     if (ret < 0) {
-        error_setg(errp, "Error %d while loading VM state", ret);
         return false;
     }
 
diff --git a/migration/savevm.h b/migration/savevm.h
index ba64a7e271..1069e2dd4f 100644
--- a/migration/savevm.h
+++ b/migration/savevm.h
@@ -60,7 +60,7 @@  void qemu_savevm_send_colo_enable(QEMUFile *f);
 void qemu_savevm_live_state(QEMUFile *f);
 int qemu_save_device_state(QEMUFile *f);
 
-int qemu_loadvm_state(QEMUFile *f);
+int qemu_loadvm_state(QEMUFile *f, Error **errp);
 void qemu_loadvm_state_cleanup(void);
 int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis);
 int qemu_load_device_state(QEMUFile *f);