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 <vsement...@virtuozzo.com>
> ---
>
> 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 <ali...@redhat.com>
> 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
