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

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