[RFC v3 0/2] migration: Update error description whenever migration fails

[RFC v3 0/2] migration: Update error description whenever migration fails

[Tejus GK]

Hi everyone,

Thank you everyone for the reviews, this is the	v3 patchset based on the 
reviews	received on the	previous ones. 

Links to the previous patchsets:
v1: https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg00868.html
v2: https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg01943.html

I've broken this patchset into two parts; the first patch contains fixes
for places in migration.c where	the failure reason is not updated.
Compared to the	previous patchset, this	fixes a	few errors existing in  
the last patch and covers a few	more places where the failure reason 
isn't updated. 

The second patch, covers places	outside	of migration.c,	which eventually 
lead to	a migration failure, along with	an error_report() call being 
made, however without an update	for the failure	reason.	I am aware that	
the changes in vmstate.c breaks	the build due to a unit-test build 
failing, so I wanted to	know the right way to approach this. 

regards,
Tejus


Tejus GK (2):
  migration: Update error description whenever migration fails
  migration: Update error description whenever migration fails

 migration/migration.c | 23 ++++++++++++-----------
 migration/savevm.c    | 13 ++++++++++---
 migration/vmstate.c   | 13 ++++++++++---
 3 files changed, 32 insertions(+), 17 deletions(-)

-- 
2.22.3

[RFC v3 1/2] migration: Update error description whenever migration fails

[Tejus GK]

There are places in migration.c where the migration is marked failed with
MIGRATION_STATUS_FAILED, but the failure reason is never updated. Hence
libvirt doesn't know why the migration failed when it queries for it.

Signed-off-by: Tejus GK <tejus.gk@nutanix.com>
---
 migration/migration.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 00d8ba8da0..864150d01d 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1683,15 +1683,11 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     } else if (strstart(uri, "fd:", &p)) {
         fd_start_outgoing_migration(s, p, &local_err);
     } else {
-        if (!(has_resume && resume)) {
-            yank_unregister_instance(MIGRATION_YANK_INSTANCE);
-        }
-        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "uri",
+        error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE, "uri",
                    "a valid migration protocol");
         migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
                           MIGRATION_STATUS_FAILED);
         block_cleanup_parameters();
-        return;
     }
 
     if (local_err) {
@@ -2073,7 +2069,7 @@ migration_wait_main_channel(MigrationState *ms)
  * Switch from normal iteration to postcopy
  * Returns non-0 on error
  */
-static int postcopy_start(MigrationState *ms)
+static int postcopy_start(MigrationState *ms, Error **errp)
 {
     int ret;
     QIOChannelBuffer *bioc;
@@ -2183,7 +2179,7 @@ static int postcopy_start(MigrationState *ms)
      */
     ret = qemu_file_get_error(ms->to_dst_file);
     if (ret) {
-        error_report("postcopy_start: Migration stream errored (pre package)");
+        error_setg(errp, "postcopy_start: Migration stream errored (pre package)");
         goto fail_closefb;
     }
 
@@ -2220,7 +2216,7 @@ static int postcopy_start(MigrationState *ms)
 
     ret = qemu_file_get_error(ms->to_dst_file);
     if (ret) {
-        error_report("postcopy_start: Migration stream errored");
+        error_setg(errp, "postcopy_start: Migration stream errored");
         migrate_set_state(&ms->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
                               MIGRATION_STATUS_FAILED);
     }
@@ -2741,6 +2737,7 @@ typedef enum {
 static MigIterateState migration_iteration_run(MigrationState *s)
 {
     uint64_t must_precopy, can_postcopy;
+    Error *local_err = NULL;
     bool in_postcopy = s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE;
 
     qemu_savevm_state_pending_estimate(&must_precopy, &can_postcopy);
@@ -2763,8 +2760,9 @@ static MigIterateState migration_iteration_run(MigrationState *s)
     /* Still a significant amount to transfer */
     if (!in_postcopy && must_precopy <= s->threshold_size &&
         qatomic_read(&s->start_postcopy)) {
-        if (postcopy_start(s)) {
-            error_report("%s: postcopy failed to start", __func__);
+        if (postcopy_start(s, &local_err)) {
+            migrate_set_error(s, local_err);
+            error_report_err(local_err);
         }
         return MIG_ITERATE_SKIP;
     }
@@ -3250,8 +3248,10 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
      */
     if (migrate_postcopy_ram() || migrate_return_path()) {
         if (open_return_path_on_source(s, !resume)) {
-            error_report("Unable to open return-path for postcopy");
+            error_setg(&local_err, "Unable to open return-path for postcopy");
             migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
+            migrate_set_error(s, local_err);
+            error_report_err(local_err);
             migrate_fd_cleanup(s);
             return;
         }
@@ -3275,6 +3275,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
     }
 
     if (multifd_save_setup(&local_err) != 0) {
+        migrate_set_error(s, local_err);
         error_report_err(local_err);
         migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
                           MIGRATION_STATUS_FAILED);
-- 
2.22.3

[RFC v3 2/2] migration: Update error description whenever migration fails

[Tejus GK]

There are places outside of migration.c which eventually leads to a
migration failure, but the failure reason is never updated. Hence
libvirt doesn't know why the migration failed when it queries for it.

Signed-off-by: Tejus GK <tejus.gk@nutanix.com>
---
 migration/savevm.c  | 13 ++++++++++---
 migration/vmstate.c | 13 ++++++++++---
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index e33788343a..39d4ecdd41 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1068,10 +1068,14 @@ void qemu_savevm_send_open_return_path(QEMUFile *f)
 int qemu_savevm_send_packaged(QEMUFile *f, const uint8_t *buf, size_t len)
 {
     uint32_t tmp;
+    MigrationState *ms = migrate_get_current();
+    Error *local_err = NULL;
 
     if (len > MAX_VM_CMD_PACKAGED_SIZE) {
-        error_report("%s: Unreasonably large packaged state: %zu",
+        error_setg(&local_err, "%s: Unreasonably large packaged state: %zu",
                      __func__, len);
+        migrate_set_error(ms, local_err);
+        error_report_err(local_err);
         return -1;
     }
 
@@ -1456,6 +1460,7 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
     int vmdesc_len;
     SaveStateEntry *se;
     int ret;
+    Error *local_err = NULL;
 
     QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
         if (se->vmsd && se->vmsd->early_setup) {
@@ -1475,8 +1480,10 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
          * bdrv_activate_all() on the other end won't fail. */
         ret = bdrv_inactivate_all();
         if (ret) {
-            error_report("%s: bdrv_inactivate_all() failed (%d)",
-                         __func__, ret);
+            error_setg(&local_err, "%s: bdrv_inactivate_all() failed (%d)",
+                       __func__, ret);
+            migrate_set_error(ms, local_err);
+            error_report_err(local_err);
             qemu_file_set_error(f, ret);
             return ret;
         }
diff --git a/migration/vmstate.c b/migration/vmstate.c
index af01d54b6f..3a5770b925 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -14,6 +14,7 @@
 #include "migration.h"
 #include "migration/vmstate.h"
 #include "savevm.h"
+#include "qapi/error.h"
 #include "qapi/qmp/json-writer.h"
 #include "qemu-file.h"
 #include "qemu/bitops.h"
@@ -323,6 +324,8 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
 {
     int ret = 0;
     const VMStateField *field = vmsd->fields;
+    MigrationState *ms = migrate_get_current();
+    Error *local_err = NULL;
 
     trace_vmstate_save_state_top(vmsd->name);
 
@@ -330,7 +333,9 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
         ret = vmsd->pre_save(opaque);
         trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
         if (ret) {
-            error_report("pre-save failed: %s", vmsd->name);
+            error_setg(&local_err, "pre-save failed: %s", vmsd->name);
+            migrate_set_error(ms, local_err);
+            error_report_err(local_err);
             return ret;
         }
     }
@@ -383,8 +388,10 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
                                      vmdesc_loop);
                 }
                 if (ret) {
-                    error_report("Save of field %s/%s failed",
-                                 vmsd->name, field->name);
+                    error_setg(&local_err, "Save of field %s/%s failed",
+                                vmsd->name, field->name);
+                    migrate_set_error(ms, local_err);
+                    error_report_err(local_err);
                     if (vmsd->post_save) {
                         vmsd->post_save(opaque);
                     }
-- 
2.22.3

Re: [RFC v3 1/2] migration: Update error description whenever migration fails

[Juan Quintela]

Tejus GK <tejus.gk@nutanix.com> wrote:
> There are places in migration.c where the migration is marked failed with
> MIGRATION_STATUS_FAILED, but the failure reason is never updated. Hence
> libvirt doesn't know why the migration failed when it queries for it.
>
> Signed-off-by: Tejus GK <tejus.gk@nutanix.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>


> ---
>  migration/migration.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 00d8ba8da0..864150d01d 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1683,15 +1683,11 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      } else if (strstart(uri, "fd:", &p)) {
>          fd_start_outgoing_migration(s, p, &local_err);
>      } else {
> -        if (!(has_resume && resume)) {
> -            yank_unregister_instance(MIGRATION_YANK_INSTANCE);
> -        }
> -        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "uri",
> +        error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE, "uri",
>                     "a valid migration protocol");
>          migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
>                            MIGRATION_STATUS_FAILED);
>          block_cleanup_parameters();
> -        return;

If you have to respin, please split the removal of the yan_unregister()
and return in one patch, and let of this patch the error bits.

>      }
>  
>      if (local_err) {
> @@ -2073,7 +2069,7 @@ migration_wait_main_channel(MigrationState *ms)
>   * Switch from normal iteration to postcopy
>   * Returns non-0 on error
>   */
> -static int postcopy_start(MigrationState *ms)
> +static int postcopy_start(MigrationState *ms, Error **errp)
>  {
>      int ret;
>      QIOChannelBuffer *bioc;
> @@ -2183,7 +2179,7 @@ static int postcopy_start(MigrationState *ms)
>       */
>      ret = qemu_file_get_error(ms->to_dst_file);
>      if (ret) {
> -        error_report("postcopy_start: Migration stream errored (pre package)");
> +        error_setg(errp, "postcopy_start: Migration stream errored (pre package)");
>          goto fail_closefb;
>      }
>  
> @@ -2220,7 +2216,7 @@ static int postcopy_start(MigrationState *ms)
>  
>      ret = qemu_file_get_error(ms->to_dst_file);
>      if (ret) {
> -        error_report("postcopy_start: Migration stream errored");
> +        error_setg(errp, "postcopy_start: Migration stream errored");
>          migrate_set_state(&ms->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
>                                MIGRATION_STATUS_FAILED);
>      }
> @@ -2741,6 +2737,7 @@ typedef enum {
>  static MigIterateState migration_iteration_run(MigrationState *s)
>  {
>      uint64_t must_precopy, can_postcopy;
> +    Error *local_err = NULL;
>      bool in_postcopy = s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE;
>  
>      qemu_savevm_state_pending_estimate(&must_precopy, &can_postcopy);
> @@ -2763,8 +2760,9 @@ static MigIterateState migration_iteration_run(MigrationState *s)
>      /* Still a significant amount to transfer */
>      if (!in_postcopy && must_precopy <= s->threshold_size &&
>          qatomic_read(&s->start_postcopy)) {
> -        if (postcopy_start(s)) {
> -            error_report("%s: postcopy failed to start", __func__);
> +        if (postcopy_start(s, &local_err)) {
> +            migrate_set_error(s, local_err);
> +            error_report_err(local_err);

Not your fault.
But the interface of migrate_set_error() is a mess.  It is followed
sometimes from error_report_err() another from error_free() and another
by ...

Re: [RFC v3 2/2] migration: Update error description whenever migration fails

[Juan Quintela]

Tejus GK <tejus.gk@nutanix.com> wrote:
> There are places outside of migration.c which eventually leads to a
> migration failure, but the failure reason is never updated. Hence
> libvirt doesn't know why the migration failed when it queries for it.
>
> Signed-off-by: Tejus GK <tejus.gk@nutanix.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>


If you have to respin:

> @@ -1456,6 +1460,7 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
>      int vmdesc_len;
>      SaveStateEntry *se;
>      int ret;
> +    Error *local_err = NULL;

You can declare this:

>      QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
>          if (se->vmsd && se->vmsd->early_setup) {
> @@ -1475,8 +1480,10 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
>           * bdrv_activate_all() on the other end won't fail. */
>          ret = bdrv_inactivate_all();
>          if (ret) {

here

> -            error_report("%s: bdrv_inactivate_all() failed (%d)",
> -                         __func__, ret);

Re: [RFC v3 0/2] migration: Update error description whenever migration fails

[Juan Quintela]

Tejus GK <tejus.gk@nutanix.com> wrote:
> Hi everyone,
>
> Thank you everyone for the reviews, this is the	v3 patchset based on the 
> reviews	received on the	previous ones. 

Hi

I did the review-by.
But you have to resend with a proper PATCH subject line to get this in.

I will split the removal of the duplicated yank, because that is
independent of the patch.

And for the rest it is ok.

Thanks, Juan.


>
> Links to the previous patchsets:
> v1: https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg00868.html
> v2: https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg01943.html
>
> I've broken this patchset into two parts; the first patch contains fixes
> for places in migration.c where	the failure reason is not updated.
> Compared to the	previous patchset, this	fixes a	few errors existing in  
> the last patch and covers a few	more places where the failure reason 
> isn't updated. 
>
> The second patch, covers places	outside	of migration.c,	which eventually 
> lead to	a migration failure, along with	an error_report() call being 
> made, however without an update	for the failure	reason.	I am aware that	
> the changes in vmstate.c breaks	the build due to a unit-test build 
> failing, so I wanted to	know the right way to approach this. 
>
> regards,
> Tejus
>
>
> Tejus GK (2):
>   migration: Update error description whenever migration fails
>   migration: Update error description whenever migration fails
>
>  migration/migration.c | 23 ++++++++++++-----------
>  migration/savevm.c    | 13 ++++++++++---
>  migration/vmstate.c   | 13 ++++++++++---
>  3 files changed, 32 insertions(+), 17 deletions(-)

Re: [RFC v3 1/2] migration: Update error description whenever migration fails

[Daniel P. Berrangé]

On Thu, May 18, 2023 at 06:23:07AM +0000, Tejus GK wrote:
> There are places in migration.c where the migration is marked failed with
> MIGRATION_STATUS_FAILED, but the failure reason is never updated. Hence
> libvirt doesn't know why the migration failed when it queries for it.
> 
> Signed-off-by: Tejus GK <tejus.gk@nutanix.com>
> ---
>  migration/migration.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With 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 :|

Re: [RFC v3 2/2] migration: Update error description whenever migration fails

[Tejus GK]

On 18/05/23 5:22 pm, Juan Quintela wrote:
> Tejus GK <tejus.gk@nutanix.com> wrote:
>> There are places outside of migration.c which eventually leads to a
>> migration failure, but the failure reason is never updated. Hence
>> libvirt doesn't know why the migration failed when it queries for it.
>>
>> Signed-off-by: Tejus GK <tejus.gk@nutanix.com>
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>

Thank you for the reviews Juan, but I believe that this particular patch shouldn't be approved yet. I have mentioned it in the RFC cover letter that the changes in this patch, in the file vmstate.c, end up breaking the build for a unit-test, eventually breaking the entire build. 

I was not sure how to implement the error reporting properly in such cases, and the aim of this patch was to receive advice on the same.  
> 
> 
> If you have to respin:
> 
>> @@ -1456,6 +1460,7 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
>>      int vmdesc_len;
>>      SaveStateEntry *se;
>>      int ret;
>> +    Error *local_err = NULL;
> 
> You can declare this:
> 
>>      QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
>>          if (se->vmsd && se->vmsd->early_setup) {
>> @@ -1475,8 +1480,10 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
>>           * bdrv_activate_all() on the other end won't fail. */
>>          ret = bdrv_inactivate_all();
>>          if (ret) {
> 
> here
> 
>> -            error_report("%s: bdrv_inactivate_all() failed (%d)",
>> -                         __func__, ret);
>