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

[Vladimir Sementsov-Ogievskiy]

On 29.03.24 13:53, Cédric Le Goater wrote:
> 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.

Oh, I missed that)

> 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.
 
Yes of course, you can keep my old r-b. Followup patch is appreciated

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

--
Best regards,
Vladimir