This will be useful to report errors at a higher level, mostly in VFIO
today.

Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 include/migration/register.h |  2 +-
 hw/vfio/migration.c          |  2 +-
 migration/ram.c              |  2 +-
 migration/savevm.c           | 10 ++++++----
 4 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/include/migration/register.h b/include/migration/register.h
index 831600a00eae4efd0464b60925d65de4d9dbcff8..e6bc226c98b27c1fb0f9e2b56d8aff491aa14d65 100644
--- a/include/migration/register.h
+++ b/include/migration/register.h
@@ -72,7 +72,7 @@ typedef struct SaveVMHandlers {
     void (*state_pending_exact)(void *opaque, uint64_t *must_precopy,
                                 uint64_t *can_postcopy);
     LoadStateHandler *load_state;
-    int (*load_setup)(QEMUFile *f, void *opaque);
+    int (*load_setup)(QEMUFile *f, void *opaque, Error **errp);
     int (*load_cleanup)(void *opaque);
     /* Called when postcopy migration wants to resume from failure */
     int (*resume_prepare)(MigrationState *s, void *opaque);
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 8bcb4bc73cd5ba5338e3ffa4d907d0e6bfbb9485..2dfbe671f6f45aa530c7341177bb532d8292cecd 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -580,7 +580,7 @@ static void vfio_save_state(QEMUFile *f, void *opaque)
     }
 }
 
-static int vfio_load_setup(QEMUFile *f, void *opaque)
+static int vfio_load_setup(QEMUFile *f, void *opaque, Error **errp)
 {
     VFIODevice *vbasedev = opaque;
 
diff --git a/migration/ram.c b/migration/ram.c
index 136c237f4079f68d4e578cf1c72eec2efc815bc8..8dac9bac2fe8b8c19e102c771a7ef6e976252906 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3498,7 +3498,7 @@ void colo_release_ram_cache(void)
  * @f: QEMUFile where to receive the data
  * @opaque: RAMState pointer
  */
-static int ram_load_setup(QEMUFile *f, void *opaque)
+static int ram_load_setup(QEMUFile *f, void *opaque, Error **errp)
 {
     xbzrle_load_setup();
     ramblock_recv_map_init();
diff --git a/migration/savevm.c b/migration/savevm.c
index f2ae799bad13e631bccf733a34c3a8fd22e8dd48..990f4249a26d28117ee365d8b20fc5bbca0d43d6 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2737,7 +2737,7 @@ static void qemu_loadvm_state_switchover_ack_needed(MigrationIncomingState *mis)
     trace_loadvm_state_switchover_ack_needed(mis->switchover_ack_pending_num);
 }
 
-static int qemu_loadvm_state_setup(QEMUFile *f)
+static int qemu_loadvm_state_setup(QEMUFile *f, Error **errp)
 {
     SaveStateEntry *se;
     int ret;
@@ -2753,10 +2753,11 @@ static int qemu_loadvm_state_setup(QEMUFile *f)
             }
         }
 
-        ret = se->ops->load_setup(f, se->opaque);
+        ret = se->ops->load_setup(f, se->opaque, errp);
         if (ret < 0) {
+            error_prepend(errp, "Load state of device %s failed: ",
+                          se->idstr);
             qemu_file_set_error(f, ret);
-            error_report("Load state of device %s failed", se->idstr);
             return ret;
         }
     }
@@ -2937,7 +2938,8 @@ int qemu_loadvm_state(QEMUFile *f)
         return ret;
     }
 
-    if (qemu_loadvm_state_setup(f) != 0) {
+    if (qemu_loadvm_state_setup(f, &local_err) != 0) {
+        error_report_err(local_err);
         return -EINVAL;
     }
 
-- 
2.43.0

On Wed, Feb 07, 2024 at 02:33:35PM +0100, Cédric Le Goater wrote:
> diff --git a/migration/ram.c b/migration/ram.c
> index 136c237f4079f68d4e578cf1c72eec2efc815bc8..8dac9bac2fe8b8c19e102c771a7ef6e976252906 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -3498,7 +3498,7 @@ void colo_release_ram_cache(void)
>   * @f: QEMUFile where to receive the data
>   * @opaque: RAMState pointer

Another one may need touch up..

>   */
> -static int ram_load_setup(QEMUFile *f, void *opaque)
> +static int ram_load_setup(QEMUFile *f, void *opaque, Error **errp)
>  {
>      xbzrle_load_setup();
>      ramblock_recv_map_init();
> diff --git a/migration/savevm.c b/migration/savevm.c
> index f2ae799bad13e631bccf733a34c3a8fd22e8dd48..990f4249a26d28117ee365d8b20fc5bbca0d43d6 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2737,7 +2737,7 @@ static void qemu_loadvm_state_switchover_ack_needed(MigrationIncomingState *mis)
>      trace_loadvm_state_switchover_ack_needed(mis->switchover_ack_pending_num);
>  }
>  
> -static int qemu_loadvm_state_setup(QEMUFile *f)
> +static int qemu_loadvm_state_setup(QEMUFile *f, Error **errp)
>  {
>      SaveStateEntry *se;
>      int ret;
> @@ -2753,10 +2753,11 @@ static int qemu_loadvm_state_setup(QEMUFile *f)
>              }
>          }
>  
> -        ret = se->ops->load_setup(f, se->opaque);
> +        ret = se->ops->load_setup(f, se->opaque, errp);
>          if (ret < 0) {
> +            error_prepend(errp, "Load state of device %s failed: ",
> +                          se->idstr);
>              qemu_file_set_error(f, ret);

Do we also want to switch to _set_error_obj()?  Or even use
migrate_set_error() (the latter may apply to previous patch too if it
works)?

> -            error_report("Load state of device %s failed", se->idstr);
>              return ret;
>          }
>      }
> @@ -2937,7 +2938,8 @@ int qemu_loadvm_state(QEMUFile *f)
>          return ret;
>      }
>  
> -    if (qemu_loadvm_state_setup(f) != 0) {
> +    if (qemu_loadvm_state_setup(f, &local_err) != 0) {
> +        error_report_err(local_err);
>          return -EINVAL;
>      }
>  
> -- 
> 2.43.0
> 
> 

-- 
Peter Xu

On 2/8/24 05:30, Peter Xu wrote:
> On Wed, Feb 07, 2024 at 02:33:35PM +0100, Cédric Le Goater wrote:
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 136c237f4079f68d4e578cf1c72eec2efc815bc8..8dac9bac2fe8b8c19e102c771a7ef6e976252906 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -3498,7 +3498,7 @@ void colo_release_ram_cache(void)
>>    * @f: QEMUFile where to receive the data
>>    * @opaque: RAMState pointer
> 
> Another one may need touch up..
> 
>>    */
>> -static int ram_load_setup(QEMUFile *f, void *opaque)
>> +static int ram_load_setup(QEMUFile *f, void *opaque, Error **errp)
>>   {
>>       xbzrle_load_setup();
>>       ramblock_recv_map_init();
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index f2ae799bad13e631bccf733a34c3a8fd22e8dd48..990f4249a26d28117ee365d8b20fc5bbca0d43d6 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -2737,7 +2737,7 @@ static void qemu_loadvm_state_switchover_ack_needed(MigrationIncomingState *mis)
>>       trace_loadvm_state_switchover_ack_needed(mis->switchover_ack_pending_num);
>>   }
>>   
>> -static int qemu_loadvm_state_setup(QEMUFile *f)
>> +static int qemu_loadvm_state_setup(QEMUFile *f, Error **errp)
>>   {
>>       SaveStateEntry *se;
>>       int ret;
>> @@ -2753,10 +2753,11 @@ static int qemu_loadvm_state_setup(QEMUFile *f)
>>               }
>>           }
>>   
>> -        ret = se->ops->load_setup(f, se->opaque);
>> +        ret = se->ops->load_setup(f, se->opaque, errp);
>>           if (ret < 0) {
>> +            error_prepend(errp, "Load state of device %s failed: ",
>> +                          se->idstr);
>>               qemu_file_set_error(f, ret);
> 
> Do we also want to switch to _set_error_obj()? 

yes. possible.

> Or even use migrate_set_error() 

It seems so and may be even remove it completely.

What we could do first is add an Errp ** argument to qemu_loadvm_state()
which would improve qmp_xen_load_devices_state() and load_snapshot().
It is less obvious for process_incoming_migration_co().

> (the latter may apply to previous patch too if it works)?

It seems safe to use migrate_set_error for both migration_thread() and
bg_migration_thread() because migration_detect_error() is called after
calling qemu_savevm_state_setup().

However, qemu_savevm_state() relies only on qemu_file_get_error() and
there would be a problem there I think.

Thanks,

C.


> 
>> -            error_report("Load state of device %s failed", se->idstr);
>>               return ret;
>>           }
>>       }
>> @@ -2937,7 +2938,8 @@ int qemu_loadvm_state(QEMUFile *f)
>>           return ret;
>>       }
>>   
>> -    if (qemu_loadvm_state_setup(f) != 0) {
>> +    if (qemu_loadvm_state_setup(f, &local_err) != 0) {
>> +        error_report_err(local_err);
>>           return -EINVAL;
>>       }
>>   
>> -- 
>> 2.43.0
>>
>>
> 

On 7/2/24 14:33, Cédric Le Goater wrote:
> This will be useful to report errors at a higher level, mostly in VFIO
> today.
> 
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   include/migration/register.h |  2 +-
>   hw/vfio/migration.c          |  2 +-
>   migration/ram.c              |  2 +-
>   migration/savevm.c           | 10 ++++++----
>   4 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/include/migration/register.h b/include/migration/register.h
> index 831600a00eae4efd0464b60925d65de4d9dbcff8..e6bc226c98b27c1fb0f9e2b56d8aff491aa14d65 100644
> --- a/include/migration/register.h
> +++ b/include/migration/register.h
> @@ -72,7 +72,7 @@ typedef struct SaveVMHandlers {
>       void (*state_pending_exact)(void *opaque, uint64_t *must_precopy,
>                                   uint64_t *can_postcopy);
>       LoadStateHandler *load_state;
> -    int (*load_setup)(QEMUFile *f, void *opaque);
> +    int (*load_setup)(QEMUFile *f, void *opaque, Error **errp);

Please document this prototype. Otherwise:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>