Re: [PATCH 04/14] migration: Modify ram_init_bitmaps() to report dirty tracking errors

2024-02-12 Thread Cédric Le Goater 

On 2/12/24 09:51, Avihai Horon wrote:

Hi Cedric,

On 07/02/2024 15:33, Cédric Le Goater wrote:

External email: Use caution opening links or attachments


The .save_setup() handler has now an Error** argument that we can use
to propagate errors reported by the .log_global_start() handler. Do
that for the RAM. qemu_savevm_state_setup() will store the error under
the migration stream for later detection in the migration sequence.

Signed-off-by: Cédric Le Goater 
---
  migration/ram.c | 19 ++-
  1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 
d86626bb1c704b2d3497b323a702ca6ca8939a79..b87245466bb46937fd0358d0c66432bcc6280018
 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2802,19 +2802,17 @@ static void 
migration_bitmap_clear_discarded_pages(RAMState *rs)
  }
  }

-static void ram_init_bitmaps(RAMState *rs)
+static void ram_init_bitmaps(RAMState *rs, Error **errp)
  {
-    Error *local_err = NULL;
-
  qemu_mutex_lock_ramlist();

  WITH_RCU_READ_LOCK_GUARD() {
  ram_list_init_bitmaps();
  /* We don't use dirty log with background snapshots */
  if (!migrate_background_snapshot()) {
-    memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, _err);
-    if (local_err) {
-    error_report_err(local_err);
+    memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, errp);
+    if (*errp) {


I think we should use ERRP_GUARD() or a local error here and also below at 
ram_init_bitmaps() (or return bool like Philippe suggested).


yes. I will rework that part.

Thanks,

C.




Thanks.


+    break;
  }
  migration_bitmap_sync_precopy(rs, false);
  }
@@ -2828,7 +2826,7 @@ static void ram_init_bitmaps(RAMState *rs)
  migration_bitmap_clear_discarded_pages(rs);
  }

-static int ram_init_all(RAMState **rsp)
+static int ram_init_all(RAMState **rsp, Error **errp)
  {
  if (ram_state_init(rsp)) {
  return -1;
@@ -2839,7 +2837,10 @@ static int ram_init_all(RAMState **rsp)
  return -1;
  }

-    ram_init_bitmaps(*rsp);
+    ram_init_bitmaps(*rsp, errp);
+    if (*errp) {
+    return -1;
+    }

  return 0;
  }
@@ -2952,7 +2953,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque, 
Error **errp)

  /* migration has already setup the bitmap, reuse it. */
  if (!migration_in_colo_state()) {
-    if (ram_init_all(rsp) != 0) {
+    if (ram_init_all(rsp, errp) != 0) {
  compress_threads_save_cleanup();
  return -1;
  }
--
2.43.0

Re: [PATCH 04/14] migration: Modify ram_init_bitmaps() to report dirty tracking errors

2024-02-12 Thread Avihai Horon 

Hi Cedric,

On 07/02/2024 15:33, Cédric Le Goater wrote:

External email: Use caution opening links or attachments


The .save_setup() handler has now an Error** argument that we can use
to propagate errors reported by the .log_global_start() handler. Do
that for the RAM. qemu_savevm_state_setup() will store the error under
the migration stream for later detection in the migration sequence.

Signed-off-by: Cédric Le Goater 
---
  migration/ram.c | 19 ++-
  1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 
d86626bb1c704b2d3497b323a702ca6ca8939a79..b87245466bb46937fd0358d0c66432bcc6280018
 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2802,19 +2802,17 @@ static void 
migration_bitmap_clear_discarded_pages(RAMState *rs)
  }
  }

-static void ram_init_bitmaps(RAMState *rs)
+static void ram_init_bitmaps(RAMState *rs, Error **errp)
  {
-Error *local_err = NULL;
-
  qemu_mutex_lock_ramlist();

  WITH_RCU_READ_LOCK_GUARD() {
  ram_list_init_bitmaps();
  /* We don't use dirty log with background snapshots */
  if (!migrate_background_snapshot()) {
-memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, _err);
-if (local_err) {
-error_report_err(local_err);
+memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, errp);
+if (*errp) {


I think we should use ERRP_GUARD() or a local error here and also below 
at ram_init_bitmaps() (or return bool like Philippe suggested).


Thanks.


+break;
  }
  migration_bitmap_sync_precopy(rs, false);
  }
@@ -2828,7 +2826,7 @@ static void ram_init_bitmaps(RAMState *rs)
  migration_bitmap_clear_discarded_pages(rs);
  }

-static int ram_init_all(RAMState **rsp)
+static int ram_init_all(RAMState **rsp, Error **errp)
  {
  if (ram_state_init(rsp)) {
  return -1;
@@ -2839,7 +2837,10 @@ static int ram_init_all(RAMState **rsp)
  return -1;
  }

-ram_init_bitmaps(*rsp);
+ram_init_bitmaps(*rsp, errp);
+if (*errp) {
+return -1;
+}

  return 0;
  }
@@ -2952,7 +2953,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque, 
Error **errp)

  /* migration has already setup the bitmap, reuse it. */
  if (!migration_in_colo_state()) {
-if (ram_init_all(rsp) != 0) {
+if (ram_init_all(rsp, errp) != 0) {
  compress_threads_save_cleanup();
  return -1;
  }
--
2.43.0

Re: [PATCH 04/14] migration: Modify ram_init_bitmaps() to report dirty tracking errors

2024-02-07 Thread Philippe Mathieu-Daudé 

On 7/2/24 14:33, Cédric Le Goater wrote:

The .save_setup() handler has now an Error** argument that we can use
to propagate errors reported by the .log_global_start() handler. Do
that for the RAM. qemu_savevm_state_setup() will store the error under
the migration stream for later detection in the migration sequence.

Signed-off-by: Cédric Le Goater 
---
  migration/ram.c | 19 ++-
  1 file changed, 10 insertions(+), 9 deletions(-)




-static void ram_init_bitmaps(RAMState *rs)
+static void ram_init_bitmaps(RAMState *rs, Error **errp)


Please return a boolean.


  {
-Error *local_err = NULL;
-
  qemu_mutex_lock_ramlist();
  
  WITH_RCU_READ_LOCK_GUARD() {

  ram_list_init_bitmaps();
  /* We don't use dirty log with background snapshots */
  if (!migrate_background_snapshot()) {
-memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, _err);
-if (local_err) {
-error_report_err(local_err);
+memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, errp);
+if (*errp) {
+break;
  }
  migration_bitmap_sync_precopy(rs, false);
  }
@@ -2828,7 +2826,7 @@ static void ram_init_bitmaps(RAMState *rs)
  migration_bitmap_clear_discarded_pages(rs);
  }

[PATCH 04/14] migration: Modify ram_init_bitmaps() to report dirty tracking errors

2024-02-07 Thread Cédric Le Goater 
The .save_setup() handler has now an Error** argument that we can use
to propagate errors reported by the .log_global_start() handler. Do
that for the RAM. qemu_savevm_state_setup() will store the error under
the migration stream for later detection in the migration sequence.

Signed-off-by: Cédric Le Goater 
---
 migration/ram.c | 19 ++-
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 
d86626bb1c704b2d3497b323a702ca6ca8939a79..b87245466bb46937fd0358d0c66432bcc6280018
 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2802,19 +2802,17 @@ static void 
migration_bitmap_clear_discarded_pages(RAMState *rs)
 }
 }
 
-static void ram_init_bitmaps(RAMState *rs)
+static void ram_init_bitmaps(RAMState *rs, Error **errp)
 {
-Error *local_err = NULL;
-
 qemu_mutex_lock_ramlist();
 
 WITH_RCU_READ_LOCK_GUARD() {
 ram_list_init_bitmaps();
 /* We don't use dirty log with background snapshots */
 if (!migrate_background_snapshot()) {
-memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, _err);
-if (local_err) {
-error_report_err(local_err);
+memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, errp);
+if (*errp) {
+break;
 }
 migration_bitmap_sync_precopy(rs, false);
 }
@@ -2828,7 +2826,7 @@ static void ram_init_bitmaps(RAMState *rs)
 migration_bitmap_clear_discarded_pages(rs);
 }
 
-static int ram_init_all(RAMState **rsp)
+static int ram_init_all(RAMState **rsp, Error **errp)
 {
 if (ram_state_init(rsp)) {
 return -1;
@@ -2839,7 +2837,10 @@ static int ram_init_all(RAMState **rsp)
 return -1;
 }
 
-ram_init_bitmaps(*rsp);
+ram_init_bitmaps(*rsp, errp);
+if (*errp) {
+return -1;
+}
 
 return 0;
 }
@@ -2952,7 +2953,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque, 
Error **errp)
 
 /* migration has already setup the bitmap, reuse it. */
 if (!migration_in_colo_state()) {
-if (ram_init_all(rsp) != 0) {
+if (ram_init_all(rsp, errp) != 0) {
 compress_threads_save_cleanup();
 return -1;
 }
-- 
2.43.0