Re: [PATCH for-9.1 v5 07/14] migration: Add Error** argument to .save_setup() handler

[Cédric Le Goater]

Hello Vladimir,

On 3/29/24 10:32, Vladimir Sementsov-Ogievskiy wrote:
> On 20.03.24 09:49, Cédric Le Goater wrote:
> > diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> > index 
> > 2708abf3d762de774ed294d3fdb8e56690d2974c..542a8c297b329abc30d1b3a205d29340fa59a961
> >  100644
> > --- a/migration/block-dirty-bitmap.c
> > +++ b/migration/block-dirty-bitmap.c
> > @@ -1213,12 +1213,14 @@ fail:
> >       return ret;
> >   }
> > -static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque)
> > +static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque, Error **errp)
> >   {
> >       DBMSaveState *s = &((DBMState *)opaque)->save;
> >       SaveBitmapState *dbms = NULL;
> >       if (init_dirty_bitmap_migration(s) < 0) {
> > +        error_setg(errp,
> > +                   "Failed to initialize dirty tracking bitmap for blocks");
>
> No, that's not about initializing a bitmap. This all is about migration of 
> block-dirty-bitmaps themselves.
>
> So correct would be say "Failed to initialize migration of block dirty bitmaps".
>
> with this, for block dirty bitmap migration:
> Acked-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

I had kept your previous R-b.

Should we remove it ? or is it ok if I address your comments below in a
followup patch, in which case the error message above would be removed.

> Still, a lot better is add errp to init_dirty_bitmap_migration() and to 
> add_bitmaps_to_list() too: look,
>
> init_dirty_bitmap_migration() fails only if add_bitmaps_to_list() fails
>
> in turn,
>
> add_bitmaps_to_list() have several clear failure points, where it always does 
> error_report (or error_report_err), which would be better to pass-through to 
> the user.

Good idea. Will do.

Thanks,

C.