Re: [Qemu-devel] [PATCH] qcow2-bitmaps: fix qcow2_can_store_new_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

07.06.2019 21:48, Vladimir Sementsov-Ogievskiy wrote:
> qcow2_can_store_new_dirty_bitmap works wrong, as it considers only
> bitmaps already stored in the qcow2 image and ignores persistent
> BdrvDirtyBitmap objects.
> 
> So, let's instead count persistent BdrvDirtyBitmaps. We load all qcow2
> bitmaps on open, so there should not be any bitmap in the image for
> which we don't have BdrvDirtyBitmaps version. If it is - it's a kind of
> corruption, and no reason to check for corruptions here (open() and
> close() are better places for it).
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
> 
> Hi!
> 
> Patch is better than discussing I thing, so here is my counter-suggestion for
> "[PATCH 0/5] block/dirty-bitmap: check number and size constraints against 
> queued bitmaps"
> by John.
> 
> It's of course needs some additional refactoring, as first assert shows bad 
> design,
> I just wrote it in such manner to make minimum changes to fix the bug.
> 
> Of course,
> Reported-by: aihua liang <address@hidden>
> may be applied here (if I understood correctly), and I hope that
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1712636
> too.
> 
>   block/qcow2-bitmap.c | 38 +++++++++++++++++---------------------
>   1 file changed, 17 insertions(+), 21 deletions(-)
> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index b2487101ed..7d1b3eeb2b 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1619,8 +1619,11 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState 
> *bs,
>                                         Error **errp)
>   {
>       BDRVQcow2State *s = bs->opaque;
> -    bool found;
> -    Qcow2BitmapList *bm_list;
> +    BdrvDirtyBitmap *bitmap;
> +    uint64_t bitmap_directory_size = 0;
> +    uint32_t nb_bitmaps = 0;
> +
> +    assert(!bdrv_find_dirty_bitmap(bs, name));

exactly bad, this should be checked in qmp_block_dirty_bitmap_add(), before 
checks around
persistence. and aio_context_acquire may be dropped..

But anyway, idea is clear I think, consider it as RFC patch

>   
>       if (s->qcow_version < 3) {
>           /* Without autoclear_features, we would always have to assume
> @@ -1636,36 +1639,29 @@ bool 
> qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>           goto fail;
>       }
>   
> -    if (s->nb_bitmaps == 0) {
> -        return true;
> +    for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL;
> +         bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
> +    {
> +        if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
> +            nb_bitmaps++;
> +            bitmap_directory_size +=
> +                calc_dir_entry_size(strlen(bdrv_dirty_bitmap_name(bitmap)), 
> 0);
> +        }
>       }
> +    nb_bitmaps++;
> +    bitmap_directory_size += calc_dir_entry_size(strlen(name), 0);
>   
> -    if (s->nb_bitmaps >= QCOW2_MAX_BITMAPS) {
> +    if (nb_bitmaps > QCOW2_MAX_BITMAPS) {
>           error_setg(errp,
>                      "Maximum number of persistent bitmaps is already 
> reached");
>           goto fail;
>       }
>   
> -    if (s->bitmap_directory_size + calc_dir_entry_size(strlen(name), 0) >
> -        QCOW2_MAX_BITMAP_DIRECTORY_SIZE)
> -    {
> +    if (bitmap_directory_size > QCOW2_MAX_BITMAP_DIRECTORY_SIZE) {
>           error_setg(errp, "Not enough space in the bitmap directory");
>           goto fail;
>       }
>   
> -    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> -                               s->bitmap_directory_size, errp);
> -    if (bm_list == NULL) {
> -        goto fail;
> -    }
> -
> -    found = find_bitmap_by_name(bm_list, name);
> -    bitmap_list_free(bm_list);
> -    if (found) {
> -        error_setg(errp, "Bitmap with the same name is already stored");
> -        goto fail;
> -    }
> -
>       return true;
>   
>   fail:
> 


-- 
Best regards,
Vladimir