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

["Cédric Le Goater" <clg@redhat.com>]

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.