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

* [Qemu-devel] [PATCH] qcow2-bitmaps: fix qcow2_can_store_new_dirty_bitmap
@ 2019-06-07 18:48 Vladimir Sementsov-Ogievskiy
  2019-06-07 18:53 ` Vladimir Sementsov-Ogievskiy
  0 siblings, 1 reply; 6+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-06-07 18:48 UTC (permalink / raw)
  To: qemu-devel, qemu-block; +Cc: kwolf, vsementsov, jsnow, mreitz

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 <vsementsov@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 <aliang@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));
 
     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:
-- 
2.18.0



^ permalink raw reply related	[flat|nested] 6+ messages in thread