| Message ID | 20240207133347.1115903-5-clg@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration: Improve error reporting | expand |
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 <clg@redhat.com> > --- > 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, &local_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); > }
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 <clg@redhat.com> > --- > 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, &local_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 > >
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 <clg@redhat.com> >> --- >> 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, &local_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 >> >> >
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, &local_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; }
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 <clg@redhat.com> --- migration/ram.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)