[PATCH v5 0/2] migration: Update error description outside migration.c

[PATCH v5 0/2] migration: Update error description outside migration.c

[Tejus GK]

Hi everyone,

Now that the patchset covering improvements in error descriptions inside
migration.c has been reviewed and merged, can this patchset, which
covers the same outside of the context of migration.c be reviewed as 
well.

Regards
Tejus


Changelog:

v4:
- Rebase on master

v3:
- Rebase on master

v2:
- Rebase on master

Tejus GK (2):
  migration/vmstate: Introduce vmstate_save_state_with_err
  migration: Update error description outside migration.c

 include/migration/vmstate.h |  4 +++-
 migration/savevm.c          | 19 +++++++++++++++----
 migration/vmstate.c         | 19 +++++++++++++------
 3 files changed, 31 insertions(+), 11 deletions(-)

-- 
2.22.3

[PATCH v5 1/2] migration/vmstate: Introduce vmstate_save_state_with_err

[Tejus GK]

Currently, a few code paths exist in the function vmstate_save_state_v,
which ultimately leads to a migration failure. However, an update in the
current MigrationState for the error description is never done.

vmstate.c somehow doesn't seem to allow	the use	of migrate_set_error due
to some	dependencies for unit tests. Hence, this patch introduces a new
function vmstate_save_state_with_err, which will eventually propagate
the error message to savevm.c where a migrate_set_error	call can be
eventually done.

Acked-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Tejus GK <tejus.gk@nutanix.com>
---
 include/migration/vmstate.h |  4 +++-
 migration/savevm.c          |  2 +-
 migration/vmstate.c         | 12 +++++++++---
 3 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index e4db910339..1a31fb7293 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -1196,9 +1196,11 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
                        void *opaque, int version_id);
 int vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
                        void *opaque, JSONWriter *vmdesc);
+int vmstate_save_state_with_err(QEMUFile *f, const VMStateDescription *vmsd,
+                       void *opaque, JSONWriter *vmdesc, Error **errp);
 int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
                          void *opaque, JSONWriter *vmdesc,
-                         int version_id);
+                         int version_id, Error **errp);
 
 bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque);
 
diff --git a/migration/savevm.c b/migration/savevm.c
index bb3e99194c..1f65294bf4 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1000,7 +1000,7 @@ static int vmstate_save(QEMUFile *f, SaveStateEntry *se, JSONWriter *vmdesc)
     if (!se->vmsd) {
         vmstate_save_old_style(f, se, vmdesc);
     } else {
-        ret = vmstate_save_state(f, se->vmsd, se->opaque, vmdesc);
+        ret = vmstate_save_state_with_err(f, se->vmsd, se->opaque, vmdesc, &local_err);
         if (ret) {
             return ret;
         }
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 438ea77cfa..dd9c76dbeb 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -315,11 +315,17 @@ bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque)
 int vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
                        void *opaque, JSONWriter *vmdesc_id)
 {
-    return vmstate_save_state_v(f, vmsd, opaque, vmdesc_id, vmsd->version_id);
+    return vmstate_save_state_v(f, vmsd, opaque, vmdesc_id, vmsd->version_id, NULL);
+}
+
+int vmstate_save_state_with_err(QEMUFile *f, const VMStateDescription *vmsd,
+                       void *opaque, JSONWriter *vmdesc_id, Error **errp)
+{
+    return vmstate_save_state_v(f, vmsd, opaque, vmdesc_id, vmsd->version_id, errp);
 }
 
 int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
-                         void *opaque, JSONWriter *vmdesc, int version_id)
+                         void *opaque, JSONWriter *vmdesc, int version_id, Error **errp)
 {
     int ret = 0;
     const VMStateField *field = vmsd->fields;
@@ -377,7 +383,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
                 } else if (field->flags & VMS_VSTRUCT) {
                     ret = vmstate_save_state_v(f, field->vmsd, curr_elem,
                                                vmdesc_loop,
-                                               field->struct_version_id);
+                                               field->struct_version_id, errp);
                 } else {
                     ret = field->info->put(f, curr_elem, size, field,
                                      vmdesc_loop);
-- 
2.22.3

Re: [PATCH v5 1/2] migration/vmstate: Introduce vmstate_save_state_with_err

[Juan Quintela]

Tejus GK <tejus.gk@nutanix.com> wrote:
> Currently, a few code paths exist in the function vmstate_save_state_v,
> which ultimately leads to a migration failure. However, an update in the
> current MigrationState for the error description is never done.
>
> vmstate.c somehow doesn't seem to allow	the use	of migrate_set_error due
> to some	dependencies for unit tests. Hence, this patch introduces a new
> function vmstate_save_state_with_err, which will eventually propagate
> the error message to savevm.c where a migrate_set_error	call can be
> eventually done.
>
> Acked-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Tejus GK <tejus.gk@nutanix.com>

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

[PATCH v5 2/2] migration: Update error description outside migration.c

[Tejus GK]

A few code paths exist in the source code,where a migration is
marked as failed via MIGRATION_STATUS_FAILED, but the failure happens
outside	of migration.c

In such	cases, an error_report() call is made, however the current
MigrationState is never	updated	with the error description, and	hence
clients	like libvirt never know	the actual reason for the failure.

This patch covers such cases outside of	migration.c and	updates	the
error description at the appropriate places.

Acked-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Tejus GK <tejus.gk@nutanix.com>
---
 migration/savevm.c  | 17 ++++++++++++++---
 migration/vmstate.c |  7 ++++---
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 1f65294bf4..60eec7c31f 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -979,6 +979,8 @@ static void save_section_footer(QEMUFile *f, SaveStateEntry *se)
 static int vmstate_save(QEMUFile *f, SaveStateEntry *se, JSONWriter *vmdesc)
 {
     int ret;
+    Error *local_err = NULL;
+    MigrationState *s = migrate_get_current();
 
     if ((!se->ops || !se->ops->save_state) && !se->vmsd) {
         return 0;
@@ -1002,6 +1004,8 @@ static int vmstate_save(QEMUFile *f, SaveStateEntry *se, JSONWriter *vmdesc)
     } else {
         ret = vmstate_save_state_with_err(f, se->vmsd, se->opaque, vmdesc, &local_err);
         if (ret) {
+            migrate_set_error(s, local_err);
+            error_report_err(local_err);
             return ret;
         }
     }
@@ -1068,10 +1072,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;
     }
 
@@ -1499,8 +1507,11 @@ 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 *local_err = NULL;
+            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 dd9c76dbeb..4cde30bf2d 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"
@@ -336,7 +337,7 @@ 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(errp, "pre-save failed: %s", vmsd->name);
             return ret;
         }
     }
@@ -389,8 +390,8 @@ 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(errp, "Save of field %s/%s failed",
+                                vmsd->name, field->name);
                     if (vmsd->post_save) {
                         vmsd->post_save(opaque);
                     }
-- 
2.22.3

Re: [PATCH v5 2/2] migration: Update error description outside migration.c

[Juan Quintela]

Tejus GK <tejus.gk@nutanix.com> wrote:
> A few code paths exist in the source code,where a migration is
> marked as failed via MIGRATION_STATUS_FAILED, but the failure happens
> outside	of migration.c
>
> In such	cases, an error_report() call is made, however the current
> MigrationState is never	updated	with the error description, and	hence
> clients	like libvirt never know	the actual reason for the failure.
>
> This patch covers such cases outside of	migration.c and	updates	the
> error description at the appropriate places.
>
> Acked-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Tejus GK <tejus.gk@nutanix.com>

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

Queued.

But I wonder.

> index 1f65294bf4..60eec7c31f 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -979,6 +979,8 @@ static void save_section_footer(QEMUFile *f, SaveStateEntry *se)
>  static int vmstate_save(QEMUFile *f, SaveStateEntry *se, JSONWriter *vmdesc)
>  {
>      int ret;
> +    Error *local_err = NULL;
> +    MigrationState *s = migrate_get_current();
>  
>      if ((!se->ops || !se->ops->save_state) && !se->vmsd) {
>          return 0;
> @@ -1002,6 +1004,8 @@ static int vmstate_save(QEMUFile *f, SaveStateEntry *se, JSONWriter *vmdesc)
>      } else {
>          ret = vmstate_save_state_with_err(f, se->vmsd, se->opaque, vmdesc, &local_err);
>          if (ret) {
> +            migrate_set_error(s, local_err);
> +            error_report_err(local_err);

We are setting the error and reporting it.

>              return ret;
>          }
>      }
> @@ -1068,10 +1072,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);

Again we set the error and we report it.

>          return -1;
>      }
>  
> @@ -1499,8 +1507,11 @@ 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 *local_err = NULL;
> +            error_setg(&local_err, "%s: bdrv_inactivate_all() failed (%d)",
> +                       __func__, ret);
> +            migrate_set_error(ms, local_err);
> +            error_report_err(local_err);

Again.

>              qemu_file_set_error(f, ret);

And we still have qemu_file_set_error() here, ouch.

>              return ret;
>          }
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index dd9c76dbeb..4cde30bf2d 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"
> @@ -336,7 +337,7 @@ 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(errp, "pre-save failed: %s", vmsd->name);

Here we only set the error

>              return ret;
>          }
>      }
> @@ -389,8 +390,8 @@ 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(errp, "Save of field %s/%s failed",
> +                                vmsd->name, field->name);

Same here.

>                      if (vmsd->post_save) {
>                          vmsd->post_save(opaque);
>                      }


So, I am wondering if it could be better to just report the error in a
single place for migration, and set it whenever we need it?

That is independent of this patch, though.

Later, Juan.

Re: [PATCH v5 2/2] migration: Update error description outside migration.c

[Tejus GK]

On 03/10/23 6:14 pm, Juan Quintela wrote:
> Tejus GK <tejus.gk@nutanix.com> wrote:
>> A few code paths exist in the source code,where a migration is
>> marked as failed via MIGRATION_STATUS_FAILED, but the failure happens
>> outside	of migration.c
>>
>> In such	cases, an error_report() call is made, however the current
>> MigrationState is never	updated	with the error description, and	hence
>> clients	like libvirt never know	the actual reason for the failure.
>>
>> This patch covers such cases outside of	migration.c and	updates	the
>> error description at the appropriate places.
>>
>> Acked-by: Peter Xu <peterx@redhat.com>
>> Signed-off-by: Tejus GK <tejus.gk@nutanix.com>
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> 
> Queued.
Thanks, will be sending out a patch with the "Reviewed by" trailer added.
> 
> But I wonder.
> 
>> index 1f65294bf4..60eec7c31f 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -979,6 +979,8 @@ static void save_section_footer(QEMUFile *f, SaveStateEntry *se)
>>   static int vmstate_save(QEMUFile *f, SaveStateEntry *se, JSONWriter *vmdesc)
>>   {
>>       int ret;
>> +    Error *local_err = NULL;
>> +    MigrationState *s = migrate_get_current();
>>   
>>       if ((!se->ops || !se->ops->save_state) && !se->vmsd) {
>>           return 0;
>> @@ -1002,6 +1004,8 @@ static int vmstate_save(QEMUFile *f, SaveStateEntry *se, JSONWriter *vmdesc)
>>       } else {
>>           ret = vmstate_save_state_with_err(f, se->vmsd, se->opaque, vmdesc, &local_err);
>>           if (ret) {
>> +            migrate_set_error(s, local_err);
>> +            error_report_err(local_err);
> 
> We are setting the error and reporting it.
> 
>>               return ret;
>>           }
>>       }
>> @@ -1068,10 +1072,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);
> 
> Again we set the error and we report it.
> 
>>           return -1;
>>       }
>>   
>> @@ -1499,8 +1507,11 @@ 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 *local_err = NULL;
>> +            error_setg(&local_err, "%s: bdrv_inactivate_all() failed (%d)",
>> +                       __func__, ret);
>> +            migrate_set_error(ms, local_err);
>> +            error_report_err(local_err);
> 
> Again.
> 
>>               qemu_file_set_error(f, ret);
> 
> And we still have qemu_file_set_error() here, ouch.
> 
>>               return ret;
>>           }
>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>> index dd9c76dbeb..4cde30bf2d 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"
>> @@ -336,7 +337,7 @@ 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(errp, "pre-save failed: %s", vmsd->name);
> 
> Here we only set the error
> 
>>               return ret;
>>           }
>>       }
>> @@ -389,8 +390,8 @@ 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(errp, "Save of field %s/%s failed",
>> +                                vmsd->name, field->name);
> 
> Same here.
You're right, I'm only setting it here and reporting it eventually in 
savevm.c. The trivial solution for this would have been directly doing a 
migrate_set_error() here, but that ended up breaking the build for the 
unit test test-vmstate.c
> 
>>                       if (vmsd->post_save) {
>>                           vmsd->post_save(opaque);
>>                       }
> 
> 
> So, I am wondering if it could be better to just report the error in a
> single place for migration, and set it whenever we need it?
Yes, that would be very convenient, for all the errors to be reported in 
lets say migration.c. Though that'd also require all the subsystems 
under migration.c to properly propagate the errors.
> 
> That is independent of this patch, though.
> 
> Later, Juan.
> 

regards,
tejus

Re: [PATCH v5 2/2] migration: Update error description outside migration.c

[Juan Quintela]

Tejus GK <tejus.gk@nutanix.com> wrote:
> On 03/10/23 6:14 pm, Juan Quintela wrote:
>> Tejus GK <tejus.gk@nutanix.com> wrote:
>>> A few code paths exist in the source code,where a migration is
>>> marked as failed via MIGRATION_STATUS_FAILED, but the failure happens
>>> outside	of migration.c
>>>
>>> In such	cases, an error_report() call is made, however the current
>>> MigrationState is never	updated	with the error description, and	hence
>>> clients	like libvirt never know	the actual reason for the failure.
>>>
>>> This patch covers such cases outside of	migration.c and	updates	the
>>> error description at the appropriate places.
>>>
>>> Acked-by: Peter Xu <peterx@redhat.com>
>>> Signed-off-by: Tejus GK <tejus.gk@nutanix.com>
>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>> Queued.
> Thanks, will be sending out a patch with the "Reviewed by" trailer added.

Send the other one.  This one is already queued.

I think that the error_report() thing, you need to review more things
than the one in this patch.

>> 
>>>               return ret;
>>>           }
>>>       }
>>> @@ -389,8 +390,8 @@ 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(errp, "Save of field %s/%s failed",
>>> +                                vmsd->name, field->name);
>> Same here.
> You're right, I'm only setting it here and reporting it eventually in
> savevm.c. The trivial solution for this would have been directly doing
> a migrate_set_error() here, but that ended up breaking the build for
> the unit test test-vmstate.c

What was the error?  Because the problem could be on the test.

>> 
>>>                       if (vmsd->post_save) {
>>>                           vmsd->post_save(opaque);
>>>                       }
>> So, I am wondering if it could be better to just report the error in
>> a
>> single place for migration, and set it whenever we need it?
> Yes, that would be very convenient, for all the errors to be reported
> in lets say migration.c. Though that'd also require all the subsystems
> under migration.c to properly propagate the errors.

Yeap, it requires auditing all the entry points, but will make easier to
know when we just need to set the error, the system will do the rest.

Thanks, Juan.

Re: [PATCH v5 2/2] migration: Update error description outside migration.c

[Tejus GK]

On 04/10/23 1:53 pm, Juan Quintela wrote:
> Tejus GK <tejus.gk@nutanix.com> wrote:
>> On 03/10/23 6:14 pm, Juan Quintela wrote:
>>> Tejus GK <tejus.gk@nutanix.com> wrote:
>>>> A few code paths exist in the source code,where a migration is
>>>> marked as failed via MIGRATION_STATUS_FAILED, but the failure happens
>>>> outside	of migration.c
>>>>
>>>> In such	cases, an error_report() call is made, however the current
>>>> MigrationState is never	updated	with the error description, and	hence
>>>> clients	like libvirt never know	the actual reason for the failure.
>>>>
>>>> This patch covers such cases outside of	migration.c and	updates	the
>>>> error description at the appropriate places.
>>>>
>>>> Acked-by: Peter Xu <peterx@redhat.com>
>>>> Signed-off-by: Tejus GK <tejus.gk@nutanix.com>
>>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>>> Queued.
>> Thanks, will be sending out a patch with the "Reviewed by" trailer added.
> 
> Send the other one.  This one is already queued.
> 
> I think that the error_report() thing, you need to review more things
> than the one in this patch.

Not sure what you mean here? The only other patch I have on the list 
apart from this one is 
https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg00280.html , 
which you marked as reviewed as well.
> 
>>>
>>>>                return ret;
>>>>            }
>>>>        }
>>>> @@ -389,8 +390,8 @@ 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(errp, "Save of field %s/%s failed",
>>>> +                                vmsd->name, field->name);
>>> Same here.
>> You're right, I'm only setting it here and reporting it eventually in
>> savevm.c. The trivial solution for this would have been directly doing
>> a migrate_set_error() here, but that ended up breaking the build for
>> the unit test test-vmstate.c
> 
> What was the error?  Because the problem could be on the test.
This is what I keep getting.

FAILED: tests/unit/test-vmstate
cc -m64 -mcx16  -o tests/unit/test-vmstate 
tests/unit/test-vmstate.p/test-vmstate.c.o -Wl,--as-needed 
-Wl,--no-undefined -pie -Wl,--whole-archive -Wl,--start-group 
libevent-loop-base.a libqom.fa libio.fa libcrypto.fa libauthz.fa 
-Wl,--no-whole-archive -fstack-protector-strong -Wl,-z,relro -Wl,-z,now 
-Wl,--warn-common libqemuutil.a 
subprojects/libvhost-user/libvhost-user-glib.a 
subprojects/libvhost-user/libvhost-user.a libmigration.fa libqom.fa 
libio.fa libcrypto.fa libauthz.fa /usr/lib64/libz.so 
/usr/lib64/libgio-2.0.so /usr/lib64/libgobject-2.0.so 
/usr/lib64/libglib-2.0.so /usr/lib64/libgmodule-2.0.so -pthread -lm 
/usr/lib64/libpixman-1.so -Wl,--end-group
libmigration.fa.p/migration_vmstate.c.o: In function `vmstate_save_state_v':
/rpmbuild/SOURCES/qemu/build/../migration/vmstate.c:333: undefined 
reference to `migrate_get_current'
/rpmbuild/SOURCES/qemu/build/../migration/vmstate.c:344: undefined 
reference to `migrate_set_error'
collect2: error: ld returned 1 exit status

I tried to figure out how the dependencies work out for unit test, but 
found no luck with that.
> 
>>>
>>>>                        if (vmsd->post_save) {
>>>>                            vmsd->post_save(opaque);
>>>>                        }
>>> So, I am wondering if it could be better to just report the error in
>>> a
>>> single place for migration, and set it whenever we need it?
>> Yes, that would be very convenient, for all the errors to be reported
>> in lets say migration.c. Though that'd also require all the subsystems
>> under migration.c to properly propagate the errors.
> 
> Yeap, it requires auditing all the entry points, but will make easier to
> know when we just need to set the error, the system will do the rest.
> 
> Thanks, Juan.
> 

regards,
tejus

Re: [PATCH v5 2/2] migration: Update error description outside migration.c

[Juan Quintela]

Tejus GK <tejus.gk@nutanix.com> wrote:
> On 04/10/23 1:53 pm, Juan Quintela wrote:
>> Tejus GK <tejus.gk@nutanix.com> wrote:
>>> On 03/10/23 6:14 pm, Juan Quintela wrote:
>>>> Tejus GK <tejus.gk@nutanix.com> wrote:
>>>>> A few code paths exist in the source code,where a migration is
>>>>> marked as failed via MIGRATION_STATUS_FAILED, but the failure happens
>>>>> outside	of migration.c
>>>>>
>>>>> In such	cases, an error_report() call is made, however the current
>>>>> MigrationState is never	updated	with the error description, and	hence
>>>>> clients	like libvirt never know	the actual reason for the failure.
>>>>>
>>>>> This patch covers such cases outside of	migration.c and	updates	the
>>>>> error description at the appropriate places.
>>>>>
>>>>> Acked-by: Peter Xu <peterx@redhat.com>
>>>>> Signed-off-by: Tejus GK <tejus.gk@nutanix.com>
>>>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>>>> Queued.
>>> Thanks, will be sending out a patch with the "Reviewed by" trailer added.
>> Send the other one.  This one is already queued.
>> I think that the error_report() thing, you need to review more
>> things
>> than the one in this patch.
>
> Not sure what you mean here? The only other patch I have on the list
> apart from this one is
> https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg00280.html ,
> which you marked as reviewed as well.
>> 
>>>>
>>>>>                return ret;
>>>>>            }
>>>>>        }
>>>>> @@ -389,8 +390,8 @@ 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(errp, "Save of field %s/%s failed",
>>>>> +                                vmsd->name, field->name);
>>>> Same here.
>>> You're right, I'm only setting it here and reporting it eventually in
>>> savevm.c. The trivial solution for this would have been directly doing
>>> a migrate_set_error() here, but that ended up breaking the build for
>>> the unit test test-vmstate.c
>> What was the error?  Because the problem could be on the test.
> This is what I keep getting.
>
> FAILED: tests/unit/test-vmstate
> cc -m64 -mcx16  -o tests/unit/test-vmstate
> tests/unit/test-vmstate.p/test-vmstate.c.o -Wl,--as-needed
> -Wl,--no-undefined -pie -Wl,--whole-archive -Wl,--start-group
> libevent-loop-base.a libqom.fa libio.fa libcrypto.fa libauthz.fa
> -Wl,--no-whole-archive -fstack-protector-strong -Wl,-z,relro
> -Wl,-z,now -Wl,--warn-common libqemuutil.a
> subprojects/libvhost-user/libvhost-user-glib.a
> subprojects/libvhost-user/libvhost-user.a libmigration.fa libqom.fa
> libio.fa libcrypto.fa libauthz.fa /usr/lib64/libz.so
> /usr/lib64/libgio-2.0.so /usr/lib64/libgobject-2.0.so
> /usr/lib64/libglib-2.0.so /usr/lib64/libgmodule-2.0.so -pthread -lm
> /usr/lib64/libpixman-1.so -Wl,--end-group
> libmigration.fa.p/migration_vmstate.c.o: In function `vmstate_save_state_v':
> /rpmbuild/SOURCES/qemu/build/../migration/vmstate.c:333: undefined
> reference to `migrate_get_current'
> /rpmbuild/SOURCES/qemu/build/../migration/vmstate.c:344: undefined
> reference to `migrate_set_error'
> collect2: error: ld returned 1 exit status
>
> I tried to figure out how the dependencies work out for unit test, but
> found no luck with that.

Ouch, that again.

I think that I know how to fix that.

Will take a look.

Later, Juan.

Re: [PATCH v5 2/2] migration: Update error description outside migration.c

[Juan Quintela]

Juan Quintela <quintela@redhat.com> wrote:
> Tejus GK <tejus.gk@nutanix.com> wrote:
>> On 04/10/23 1:53 pm, Juan Quintela wrote:
>>> Tejus GK <tejus.gk@nutanix.com> wrote:
>>>> On 03/10/23 6:14 pm, Juan Quintela wrote:
> Ouch, that again.
>
> I think that I know how to fix that.
>
> Will take a look.
>
> Later, Juan.

I first only saw that you were missing migrate_set_error().
migrate_get_current() is .... more interesting.

The problem is that migration.c has lots of things that depend of qemu
and we can't use that in qtest.
This is one disection toh help fix it, but I can't see a trivial way to
get current_migration() working as needed.  Really what we should have
is a way to have vmstate store its own error, and only set
migrate_set_error() at callers time, but that don't seem completely
obvious how to do it.

Later, Juan.


From cdbdbca65059ebb9fd6a4d354a94c3be7cf69f92 Mon Sep 17 00:00:00 2001
From: Juan Quintela <quintela@redhat.com>
Date: Wed, 4 Oct 2023 15:22:39 +0200
Subject: [PATCH] migration: Create common.c

We can (yet) add migration.c to migration_files because it brings too
many dependencies to the tests.

This file is a place where to move things that are also needed for
tests.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/common.c    | 25 +++++++++++++++++++++++++
 migration/migration.c |  8 --------
 migration/meson.build |  1 +
 3 files changed, 26 insertions(+), 8 deletions(-)
 create mode 100644 migration/common.c

diff --git a/migration/common.c b/migration/common.c
new file mode 100644
index 0000000000..1d02f37c99
--- /dev/null
+++ b/migration/common.c
@@ -0,0 +1,25 @@
+/*
+ * QEMU live migration common code for qemu and qtests
+ *
+ * Copyright (c) 2011 Red Hat Inc
+ *
+ * Authors:
+ *  Juan Quintela <quintela@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/cutils.h"
+#include "qemu/error-report.h"
+#include "qapi/error.h"
+#include "migration.h"
+
+void migrate_set_error(MigrationState *s, const Error *error)
+{
+    QEMU_LOCK_GUARD(&s->error_mutex);
+    if (!s->error) {
+        s->error = error_copy(error);
+    }
+}
diff --git a/migration/migration.c b/migration/migration.c
index 585d3c8f55..f143b3a41e 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1221,14 +1221,6 @@ static void migrate_fd_cleanup_bh(void *opaque)
     object_unref(OBJECT(s));
 }
 
-void migrate_set_error(MigrationState *s, const Error *error)
-{
-    QEMU_LOCK_GUARD(&s->error_mutex);
-    if (!s->error) {
-        s->error = error_copy(error);
-    }
-}
-
 static void migrate_error_free(MigrationState *s)
 {
     QEMU_LOCK_GUARD(&s->error_mutex);
diff --git a/migration/meson.build b/migration/meson.build
index 92b1cc4297..57b3098c46 100644
--- a/migration/meson.build
+++ b/migration/meson.build
@@ -1,5 +1,6 @@
 # Files needed by unit tests
 migration_files = files(
+  'common.c',
   'migration-stats.c',
   'page_cache.c',
   'xbzrle.c',

-- 
2.41.0