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

[Vladimir Sementsov-Ogievskiy]

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>


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.

>           return -1;
>       }
>   
> diff --git a/migrat

--
Best regards,
Vladimir