Re: [PATCH 02/14] migration: Add Error** argument to .load_setup() handler

2024-02-09 Thread Cédric Le Goater 

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

Re: [PATCH 02/14] migration: Add Error** argument to .load_setup() handler

2024-02-07 Thread Peter Xu 
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

Re: [PATCH 02/14] migration: Add Error** argument to .load_setup() handler

2024-02-07 Thread Philippe Mathieu-Daudé 

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 
---
  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é

[PATCH 02/14] migration: Add Error** argument to .load_setup() handler

2024-02-07 Thread Cédric Le Goater 
This will be useful to report errors at a higher level, mostly in VFIO
today.

Signed-off-by: Cédric Le Goater 
---
 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