[Qemu-devel] [PATCH v6 00/29] qcow2: persistent dirty bitmaps

[Qemu-devel] [PATCH v6 00/29] qcow2: persistent dirty bitmaps

[Vladimir Sementsov-Ogievskiy]

v6:
https://src.openvz.org/users/vsementsov/repos/qemu/browse?at=refs%2Ftags%2Fqcow2-bitmap-v6
based on block-next (https://github.com/XanClic/qemu/commits/block-next)

There are a lot of changes, reorderings and additions in comparement with v5.
One principal thing: now bitmaps are removed from image after loading instead
of marking them in_use. It is simpler and we do not need to store superfluous data.
Also, we are no more interested in command line interface to dirty bitmaps.
So it is dropped.  If someone needs it I can add it later.

Vladimir Sementsov-Ogievskiy (29):
  hbitmap: fix dirty iter
  tests: add hbitmap iter test
  block: fix bdrv_dirty_bitmap_granularity signature
  block/dirty-bitmap: add deserialize_ones func
  qcow2-bitmap: structs and consts
  qcow2-bitmap: add qcow2_read_bitmaps()
  qcow2-bitmap: add qcow2_bitmap_load()
  qcow2-bitmap: delete bitmap from qcow2 after load
  qcow2-bitmap: add qcow2_bitmap_store()
  qcow2-bitmap: add IN_USE flag
  qcow2-bitmap: check constraints
  qcow2: add qcow2_delete_bitmaps
  qcow2: add dirty bitmaps extension
  qcow2-bitmap: add qcow2_bitmap_load_check()
  block/dirty-bitmap: introduce persistent bitmaps
  block: add bdrv_load_dirty_bitmap()
  qcow2-bitmap: add autoclear bit
  qcow2-bitmap: disallow storing bitmap to other bs
  block/dirty-bitmap: add autoload field to BdrvDirtyBitmap
  qcow2-bitmap: add AUTO flag
  qcow2-bitmap: add EXTRA_DATA_COMPATIBLE flag
  qmp: add persistent flag to block-dirty-bitmap-add
  qmp: add autoload parameter to block-dirty-bitmap-add
  qcow2-bitmap: maintian BlockDirtyBitmap.autoload
  qapi: add md5 checksum of last dirty bitmap level to query-block
  iotests: test qcow2 persistent dirty bitmap
  qcow2-bitmap: delete in_use bitmaps on image load
  qcow2-bitmap: do not try reloading bitmaps
  qcow2-dirty-bitmap: refcounts

 block.c                       |    2 +
 block/Makefile.objs           |    2 +-
 block/dirty-bitmap.c          |  109 +++-
 block/qcow2-bitmap.c          | 1148 +++++++++++++++++++++++++++++++++++++++++
 block/qcow2-refcount.c        |   14 +-
 block/qcow2.c                 |  109 +++-
 block/qcow2.h                 |   59 +++
 blockdev.c                    |   30 +-
 include/block/block_int.h     |    9 +
 include/block/dirty-bitmap.h  |   18 +-
 include/qemu/hbitmap.h        |   23 +
 qapi/block-core.json          |    8 +-
 qmp-commands.hx               |    7 +-
 tests/qemu-iotests/160        |   87 ++++
 tests/qemu-iotests/160.out    |    5 +
 tests/qemu-iotests/group      |    1 +
 tests/qemu-iotests/iotests.py |    6 +
 tests/test-hbitmap.c          |   19 +
 util/hbitmap.c                |   28 +-
 19 files changed, 1669 insertions(+), 15 deletions(-)
 create mode 100644 block/qcow2-bitmap.c
 create mode 100755 tests/qemu-iotests/160
 create mode 100644 tests/qemu-iotests/160.out

-- 
1.8.3.1

[Qemu-devel] [PATCH 01/29] hbitmap: fix dirty iter

[Vladimir Sementsov-Ogievskiy]

If dirty bitmap was cleared during iterator life, we can went to zero
current in hbitmap_iter_skip_words, starting from saved (and currently
wrong hbi->cur[...]).

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 util/hbitmap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/util/hbitmap.c b/util/hbitmap.c
index 6a13c12..f807f64 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -106,8 +106,9 @@ unsigned long hbitmap_iter_skip_words(HBitmapIter *hbi)
 
     unsigned long cur;
     do {
-        cur = hbi->cur[--i];
+        i--;
         pos >>= BITS_PER_LEVEL;
+        cur = hbi->cur[i] & hb->levels[i][pos];
     } while (cur == 0);
 
     /* Check for end of iteration.  We always use fewer than BITS_PER_LONG
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 01/29] hbitmap: fix dirty iter

[Kevin Wolf]

Am 08.08.2016 um 17:04 hat Vladimir Sementsov-Ogievskiy geschrieben:
> If dirty bitmap was cleared during iterator life, we can went to zero
> current in hbitmap_iter_skip_words, starting from saved (and currently
> wrong hbi->cur[...]).

I was going to suggest improved grammar, but actually I find this
description hard to understand even with correct grammar.

If I understand correctly, hbi->cur contains a copy of hb->levels that
isn't updated after the iterator entered the subtree, because this way
we avoid going backwards if concurrent operations set a new bit.
However, in the opposite case of clearing a bit, we actually want to use
the new value because we wouldn't find anything to return when going to
the lower levels. This is what the & does in your patch.

Is this explanation reasonably correct? If so, maybe you can try to make
these points in the commit message in the next version.

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> ---
>  util/hbitmap.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/util/hbitmap.c b/util/hbitmap.c
> index 6a13c12..f807f64 100644
> --- a/util/hbitmap.c
> +++ b/util/hbitmap.c
> @@ -106,8 +106,9 @@ unsigned long hbitmap_iter_skip_words(HBitmapIter *hbi)
>  
>      unsigned long cur;
>      do {
> -        cur = hbi->cur[--i];
> +        i--;
>          pos >>= BITS_PER_LEVEL;
> +        cur = hbi->cur[i] & hb->levels[i][pos];
>      } while (cur == 0);

I think you will want to update the comment for hbitmap_iter_init. This
is what it says today:

 * Concurrent setting of bits is acceptable, and will at worst cause the
 * iteration to miss some of those bits.  Resetting bits before the current
 * position of the iterator is also okay.  However, concurrent resetting of
 * bits can lead to unexpected behavior if the iterator has not yet reached
 * those bits.

Kevin

Re: [Qemu-devel] [PATCH 01/29] hbitmap: fix dirty iter

[Vladimir Sementsov-Ogievskiy]

On 10.08.2016 16:41, Kevin Wolf wrote:
> Am 08.08.2016 um 17:04 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> If dirty bitmap was cleared during iterator life, we can went to zero
>> current in hbitmap_iter_skip_words, starting from saved (and currently
>> wrong hbi->cur[...]).
> I was going to suggest improved grammar, but actually I find this
> description hard to understand even with correct grammar.
>
> If I understand correctly, hbi->cur contains a copy of hb->levels that
> isn't updated after the iterator entered the subtree, because this way
> we avoid going backwards if concurrent operations set a new bit.
> However, in the opposite case of clearing a bit, we actually want to use
> the new value because we wouldn't find anything to return when going to
> the lower levels. This is what the & does in your patch.
>
> Is this explanation reasonably correct? If so, maybe you can try to make
> these points in the commit message in the next version.

Something like this, yes.

>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> ---
>>   util/hbitmap.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/util/hbitmap.c b/util/hbitmap.c
>> index 6a13c12..f807f64 100644
>> --- a/util/hbitmap.c
>> +++ b/util/hbitmap.c
>> @@ -106,8 +106,9 @@ unsigned long hbitmap_iter_skip_words(HBitmapIter *hbi)
>>   
>>       unsigned long cur;
>>       do {
>> -        cur = hbi->cur[--i];
>> +        i--;
>>           pos >>= BITS_PER_LEVEL;
>> +        cur = hbi->cur[i] & hb->levels[i][pos];
>>       } while (cur == 0);
> I think you will want to update the comment for hbitmap_iter_init. This
> is what it says today:
>
>   * Concurrent setting of bits is acceptable, and will at worst cause the
>   * iteration to miss some of those bits.  Resetting bits before the current
>   * position of the iterator is also okay.  However, concurrent resetting of
>   * bits can lead to unexpected behavior if the iterator has not yet reached
>   * those bits.

Aha, cool. We've missed this. So this patch is a new feature but not a 
bug fix. I will update this comment.

>
> Kevin


-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH 02/29] tests: add hbitmap iter test

[Vladimir Sementsov-Ogievskiy]

Test that hbitmap iter is resistant to bitmap resetting.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 tests/test-hbitmap.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/tests/test-hbitmap.c b/tests/test-hbitmap.c
index 0ed918c..e24377f 100644
--- a/tests/test-hbitmap.c
+++ b/tests/test-hbitmap.c
@@ -881,6 +881,22 @@ static void hbitmap_test_add(const char *testpath,
                hbitmap_test_teardown);
 }
 
+static void test_hbitmap_iter_and_reset(TestHBitmapData *data,
+                                        const void *unused)
+{
+    HBitmapIter hbi;
+
+    hbitmap_test_init(data, L1 * 2, 0);
+    hbitmap_set(data->hb, 0, data->size);
+
+    hbitmap_iter_init(&hbi, data->hb, BITS_PER_LONG - 1);
+
+    hbitmap_iter_next(&hbi);
+
+    hbitmap_reset_all(data->hb);
+    hbitmap_iter_next(&hbi);
+}
+
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
@@ -938,6 +954,9 @@ int main(int argc, char **argv)
                      test_hbitmap_serialize_part);
     hbitmap_test_add("/hbitmap/serialize/zeroes",
                      test_hbitmap_serialize_zeroes);
+
+    hbitmap_test_add("/hbitmap/iter/iter_and_reset",
+                     test_hbitmap_iter_and_reset);
     g_test_run();
 
     return 0;
-- 
1.8.3.1

[Qemu-devel] [PATCH 03/29] block: fix bdrv_dirty_bitmap_granularity signature

[Vladimir Sementsov-Ogievskiy]

Make getter signature const-correct. This allows other functions with
const dirty bitmap parameter use bdrv_dirty_bitmap_granularity().

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/dirty-bitmap.c         | 2 +-
 include/block/dirty-bitmap.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 519737c..186941c 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -388,7 +388,7 @@ uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs)
     return granularity;
 }
 
-uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap)
+uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap)
 {
     return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
 }
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 9dea14b..7cbe623 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -29,7 +29,7 @@ void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
 uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
-uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap);
+uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap);
 uint32_t bdrv_dirty_bitmap_meta_granularity(BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 03/29] block: fix bdrv_dirty_bitmap_granularity signature

[Kevin Wolf]

Am 08.08.2016 um 17:04 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Make getter signature const-correct. This allows other functions with
> const dirty bitmap parameter use bdrv_dirty_bitmap_granularity().
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH 04/29] block/dirty-bitmap: add deserialize_ones func

[Vladimir Sementsov-Ogievskiy]

Add bdrv_dirty_bitmap_deserialize_ones() function, which is needed for
qcow2 bitmap loading, to handle unallocated bitmap parts, marked as
all-ones.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/dirty-bitmap.c         |  7 +++++++
 include/block/dirty-bitmap.h |  3 +++
 include/qemu/hbitmap.h       | 15 +++++++++++++++
 util/hbitmap.c               | 17 +++++++++++++++++
 4 files changed, 42 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 186941c..90af372 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -499,6 +499,13 @@ void bdrv_dirty_bitmap_deserialize_zeroes(BdrvDirtyBitmap *bitmap,
     hbitmap_deserialize_zeroes(bitmap->bitmap, start, count, finish);
 }
 
+void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap,
+                                        uint64_t start, uint64_t count,
+                                        bool finish)
+{
+    hbitmap_deserialize_ones(bitmap->bitmap, start, count, finish);
+}
+
 void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap)
 {
     hbitmap_deserialize_finish(bitmap->bitmap);
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 7cbe623..1e17729 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -70,6 +70,9 @@ void bdrv_dirty_bitmap_deserialize_part(BdrvDirtyBitmap *bitmap,
 void bdrv_dirty_bitmap_deserialize_zeroes(BdrvDirtyBitmap *bitmap,
                                           uint64_t start, uint64_t count,
                                           bool finish);
+void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap,
+                                        uint64_t start, uint64_t count,
+                                        bool finish);
 void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
 
 #endif
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index eb46475..a7b9ccc 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -216,6 +216,21 @@ void hbitmap_deserialize_zeroes(HBitmap *hb, uint64_t start, uint64_t count,
                                 bool finish);
 
 /**
+ * hbitmap_deserialize_ones
+ * @hb: HBitmap to operate on.
+ * @start: First bit to restore.
+ * @count: Number of bits to restore.
+ * @finish: Whether to call hbitmap_deserialize_finish automatically.
+ *
+ * Fills the bitmap with ones.
+ *
+ * If @finish is false, caller must call hbitmap_serialize_finish before using
+ * the bitmap.
+ */
+void hbitmap_deserialize_ones(HBitmap *hb, uint64_t start, uint64_t count,
+                              bool finish);
+
+/**
  * hbitmap_deserialize_finish
  * @hb: HBitmap to operate on.
  *
diff --git a/util/hbitmap.c b/util/hbitmap.c
index f807f64..3a3257c 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -511,6 +511,23 @@ void hbitmap_deserialize_zeroes(HBitmap *hb, uint64_t start, uint64_t count,
     }
 }
 
+void hbitmap_deserialize_ones(HBitmap *hb, uint64_t start, uint64_t count,
+                              bool finish)
+{
+    uint64_t el_count;
+    unsigned long *first;
+
+    if (!count) {
+        return;
+    }
+    serialization_chunk(hb, start, count, &first, &el_count);
+
+    memset(first, 0xff, el_count * sizeof(unsigned long));
+    if (finish) {
+        hbitmap_deserialize_finish(hb);
+    }
+}
+
 void hbitmap_deserialize_finish(HBitmap *bitmap)
 {
     int64_t i, size, prev_size;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 04/29] block/dirty-bitmap: add deserialize_ones func

[Kevin Wolf]

Am 08.08.2016 um 17:04 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Add bdrv_dirty_bitmap_deserialize_ones() function, which is needed for
> qcow2 bitmap loading, to handle unallocated bitmap parts, marked as
> all-ones.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH 05/29] qcow2-bitmap: structs and consts

[Vladimir Sementsov-Ogievskiy]

Create block/qcow2-bitmap.c
Add data structures and constraints accordingly to docs/specs/qcow2.txt

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/Makefile.objs  |  2 +-
 block/qcow2-bitmap.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.h        | 29 +++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+), 1 deletion(-)
 create mode 100644 block/qcow2-bitmap.c

diff --git a/block/Makefile.objs b/block/Makefile.objs
index 2593a2f..545d618 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -1,5 +1,5 @@
 block-obj-y += raw_bsd.o qcow.o vdi.o vmdk.o cloop.o bochs.o vpc.o vvfat.o
-block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-cache.o
+block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-cache.o qcow2-bitmap.o
 block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
 block-obj-y += qed-check.o
 block-obj-$(CONFIG_VHDX) += vhdx.o vhdx-endian.o vhdx-log.o
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
new file mode 100644
index 0000000..cd18b07
--- /dev/null
+++ b/block/qcow2-bitmap.c
@@ -0,0 +1,47 @@
+/*
+ * Bitmaps for the QCOW version 2 format
+ *
+ * Copyright (c) 2014-2016 Vladimir Sementsov-Ogievskiy
+ *
+ * This file is derived from qcow2-snapshot.c, original copyright:
+ * Copyright (c) 2004-2006 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+/* NOTICE: BME here means Bitmaps Extension and used as a namespace for
+ * _internal_ constants. Please do not use this _internal_ abbreviation for
+ * other needs and/or outside of this file. */
+
+/* Bitmap directory entry constraints */
+#define BME_MAX_TABLE_SIZE 0x8000000
+#define BME_MAX_PHYS_SIZE 0x20000000 /* 512 mb */
+#define BME_MAX_GRANULARITY_BITS 31
+#define BME_MIN_GRANULARITY_BITS 9
+#define BME_MAX_NAME_SIZE 1023
+
+/* Bitmap directory entry flags */
+#define BME_RESERVED_FLAGS 0xffffffff
+
+/* bits [1, 8] U [56, 63] are reserved */
+#define BME_TABLE_ENTRY_RESERVED_MASK 0xff000000000001fe
+
+typedef enum BitmapType {
+    BT_DIRTY_TRACKING_BITMAP = 1
+} BitmapType;
diff --git a/block/qcow2.h b/block/qcow2.h
index b36a7bf..b12cecc 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -52,6 +52,10 @@
  * space for snapshot names and IDs */
 #define QCOW_MAX_SNAPSHOTS_SIZE (1024 * QCOW_MAX_SNAPSHOTS)
 
+/* Bitmap header extension constraints */
+#define QCOW_MAX_DIRTY_BITMAPS 65535
+#define QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE (1024 * QCOW_MAX_DIRTY_BITMAPS)
+
 /* indicate that the refcount of the referenced cluster is exactly one. */
 #define QCOW_OFLAG_COPIED     (1ULL << 63)
 /* indicate that the cluster is compressed (they never have the copied flag) */
@@ -142,6 +146,22 @@ typedef struct QEMU_PACKED QCowSnapshotHeader {
     /* name follows  */
 } QCowSnapshotHeader;
 
+/* QCow2BitmapHeader is actually a bitmap directory entry */
+typedef struct QEMU_PACKED QCow2BitmapHeader {
+    /* header is 8 byte aligned */
+    uint64_t bitmap_table_offset;
+
+    uint32_t bitmap_table_size;
+    uint32_t flags;
+
+    uint8_t type;
+    uint8_t granularity_bits;
+    uint16_t name_size;
+    uint32_t extra_data_size;
+    /* extra data follows  */
+    /* name follows  */
+} QCow2BitmapHeader;
+
 typedef struct QEMU_PACKED QCowSnapshotExtraData {
     uint64_t vm_state_size_large;
     uint64_t disk_size;
@@ -222,6 +242,15 @@ typedef uint64_t Qcow2GetRefcountFunc(const void *refcount_array,
 typedef void Qcow2SetRefcountFunc(void *refcount_array,
                                   uint64_t index, uint64_t value);
 
+/* Be careful, Qcow2BitmapHeaderExt is not an extension of QCow2BitmapHeader, it
+ * is Qcow2 header extension */
+typedef struct Qcow2BitmapHeaderExt {
+    uint32_t nb_bitmaps;
+    uint32_t reserved32;
+    uint64_t bitmap_directory_size;
+    uint64_t bitmap_directory_offset;
+} QEMU_PACKED Qcow2BitmapHeaderExt;
+
 typedef struct BDRVQcow2State {
     int cluster_bits;
     int cluster_size;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 05/29] qcow2-bitmap: structs and consts

[Kevin Wolf]

Am 08.08.2016 um 17:04 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Create block/qcow2-bitmap.c
> Add data structures and constraints accordingly to docs/specs/qcow2.txt
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/Makefile.objs  |  2 +-
>  block/qcow2-bitmap.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  block/qcow2.h        | 29 +++++++++++++++++++++++++++++
>  3 files changed, 77 insertions(+), 1 deletion(-)
>  create mode 100644 block/qcow2-bitmap.c
> 
> diff --git a/block/Makefile.objs b/block/Makefile.objs
> index 2593a2f..545d618 100644
> --- a/block/Makefile.objs
> +++ b/block/Makefile.objs
> @@ -1,5 +1,5 @@
>  block-obj-y += raw_bsd.o qcow.o vdi.o vmdk.o cloop.o bochs.o vpc.o vvfat.o
> -block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-cache.o
> +block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-cache.o qcow2-bitmap.o
>  block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
>  block-obj-y += qed-check.o
>  block-obj-$(CONFIG_VHDX) += vhdx.o vhdx-endian.o vhdx-log.o
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> new file mode 100644
> index 0000000..cd18b07
> --- /dev/null
> +++ b/block/qcow2-bitmap.c
> @@ -0,0 +1,47 @@
> +/*
> + * Bitmaps for the QCOW version 2 format
> + *
> + * Copyright (c) 2014-2016 Vladimir Sementsov-Ogievskiy
> + *
> + * This file is derived from qcow2-snapshot.c, original copyright:
> + * Copyright (c) 2004-2006 Fabrice Bellard
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +/* NOTICE: BME here means Bitmaps Extension and used as a namespace for
> + * _internal_ constants. Please do not use this _internal_ abbreviation for
> + * other needs and/or outside of this file. */
> +
> +/* Bitmap directory entry constraints */
> +#define BME_MAX_TABLE_SIZE 0x8000000
> +#define BME_MAX_PHYS_SIZE 0x20000000 /* 512 mb */
> +#define BME_MAX_GRANULARITY_BITS 31
> +#define BME_MIN_GRANULARITY_BITS 9

The spec accepts 0. Should we change the spec or at least add a note
that qemu requires a minimum of 512 bytes?

> +#define BME_MAX_NAME_SIZE 1023
> +
> +/* Bitmap directory entry flags */
> +#define BME_RESERVED_FLAGS 0xffffffff
> +
> +/* bits [1, 8] U [56, 63] are reserved */
> +#define BME_TABLE_ENTRY_RESERVED_MASK 0xff000000000001fe
> +
> +typedef enum BitmapType {
> +    BT_DIRTY_TRACKING_BITMAP = 1
> +} BitmapType;
> diff --git a/block/qcow2.h b/block/qcow2.h
> index b36a7bf..b12cecc 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -52,6 +52,10 @@
>   * space for snapshot names and IDs */
>  #define QCOW_MAX_SNAPSHOTS_SIZE (1024 * QCOW_MAX_SNAPSHOTS)
>  
> +/* Bitmap header extension constraints */
> +#define QCOW_MAX_DIRTY_BITMAPS 65535
> +#define QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE (1024 * QCOW_MAX_DIRTY_BITMAPS)
> +
>  /* indicate that the refcount of the referenced cluster is exactly one. */
>  #define QCOW_OFLAG_COPIED     (1ULL << 63)
>  /* indicate that the cluster is compressed (they never have the copied flag) */
> @@ -142,6 +146,22 @@ typedef struct QEMU_PACKED QCowSnapshotHeader {
>      /* name follows  */
>  } QCowSnapshotHeader;
>  
> +/* QCow2BitmapHeader is actually a bitmap directory entry */
> +typedef struct QEMU_PACKED QCow2BitmapHeader {

Why not call it Qcow2BitmapDirEntry then?

> +    /* header is 8 byte aligned */
> +    uint64_t bitmap_table_offset;
> +
> +    uint32_t bitmap_table_size;
> +    uint32_t flags;
> +
> +    uint8_t type;
> +    uint8_t granularity_bits;
> +    uint16_t name_size;
> +    uint32_t extra_data_size;
> +    /* extra data follows  */
> +    /* name follows  */
> +} QCow2BitmapHeader;
> +
>  typedef struct QEMU_PACKED QCowSnapshotExtraData {
>      uint64_t vm_state_size_large;
>      uint64_t disk_size;
> @@ -222,6 +242,15 @@ typedef uint64_t Qcow2GetRefcountFunc(const void *refcount_array,
>  typedef void Qcow2SetRefcountFunc(void *refcount_array,
>                                    uint64_t index, uint64_t value);
>  
> +/* Be careful, Qcow2BitmapHeaderExt is not an extension of QCow2BitmapHeader, it
> + * is Qcow2 header extension */

Misunderstanding already avoided with Qcow2BitmapDirEntry. :-)

> +typedef struct Qcow2BitmapHeaderExt {
> +    uint32_t nb_bitmaps;
> +    uint32_t reserved32;
> +    uint64_t bitmap_directory_size;
> +    uint64_t bitmap_directory_offset;
> +} QEMU_PACKED Qcow2BitmapHeaderExt;

Looks good otherwise.

Kevin

[Qemu-devel] [PATCH 06/29] qcow2-bitmap: add qcow2_read_bitmaps()

[Vladimir Sementsov-Ogievskiy]

Add qcow2_read_bitmaps, reading bitmap directory as specified in
docs/specs/qcow2.txt

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2-bitmap.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.h        |   9 +++++
 2 files changed, 109 insertions(+)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index cd18b07..91ddd5f 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -25,6 +25,12 @@
  * THE SOFTWARE.
  */
 
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+
+#include "block/block_int.h"
+#include "block/qcow2.h"
+
 /* NOTICE: BME here means Bitmaps Extension and used as a namespace for
  * _internal_ constants. Please do not use this _internal_ abbreviation for
  * other needs and/or outside of this file. */
@@ -42,6 +48,100 @@
 /* bits [1, 8] U [56, 63] are reserved */
 #define BME_TABLE_ENTRY_RESERVED_MASK 0xff000000000001fe
 
+#define for_each_bitmap_header_in_dir(h, dir, size) \
+    for (h = (QCow2BitmapHeader *)(dir); \
+         h < (QCow2BitmapHeader *)((uint8_t *)(dir) + size); \
+         h = next_dir_entry(h))
+
 typedef enum BitmapType {
     BT_DIRTY_TRACKING_BITMAP = 1
 } BitmapType;
+
+static inline void bitmap_header_to_cpu(QCow2BitmapHeader *h)
+{
+    be64_to_cpus(&h->bitmap_table_offset);
+    be32_to_cpus(&h->bitmap_table_size);
+    be32_to_cpus(&h->flags);
+    be16_to_cpus(&h->name_size);
+    be32_to_cpus(&h->extra_data_size);
+}
+
+static inline int calc_dir_entry_size(size_t name_size)
+{
+    return align_offset(sizeof(QCow2BitmapHeader) + name_size, 8);
+}
+
+static inline int dir_entry_size(QCow2BitmapHeader *h)
+{
+    return calc_dir_entry_size(h->name_size);
+}
+
+static inline QCow2BitmapHeader *next_dir_entry(QCow2BitmapHeader *entry)
+{
+    return (QCow2BitmapHeader *)((uint8_t *)entry + dir_entry_size(entry));
+}
+
+/* directory_read
+ * Read bitmaps directory from bs by @offset and @size. Convert it to cpu
+ * format from BE.
+ */
+static uint8_t *directory_read(BlockDriverState *bs,
+                               uint64_t offset, uint64_t size, Error **errp)
+{
+    int ret;
+    uint8_t *dir;
+    QCow2BitmapHeader *h;
+
+    dir = g_try_malloc0(size);
+    if (dir == NULL) {
+        error_setg(errp, "Can't allocate space for bitmap directory.");
+        return NULL;
+    }
+
+    ret = bdrv_pread(bs->file, offset, dir, size);
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "Can't read bitmap directory.");
+        goto fail;
+    }
+
+    /* loop is safe because next entry offset is calculated after conversion to
+     * cpu format */
+    for_each_bitmap_header_in_dir(h, dir, size) {
+        bitmap_header_to_cpu(h);
+    }
+
+    if ((uint8_t *)h != dir + size) {
+        error_setg(errp, "Broken bitmap directory.");
+        goto fail;
+    }
+
+    return dir;
+
+fail:
+    g_free(dir);
+
+    return NULL;
+}
+
+int qcow2_read_bitmaps(BlockDriverState *bs, Error **errp)
+{
+    BDRVQcow2State *s = bs->opaque;
+
+    if (s->bitmap_directory != NULL) {
+        error_setg(errp, "Try read bitmaps, when they are already read.");
+        return -EEXIST;
+    }
+
+    if (s->nb_bitmaps == 0) {
+        /* No bitmaps - nothing to do */
+        return 0;
+    }
+
+    s->bitmap_directory = directory_read(bs, s->bitmap_directory_offset,
+                                         s->bitmap_directory_size, errp);
+    if (s->bitmap_directory == NULL) {
+        return -EINVAL;
+    }
+
+    return 0;
+}
diff --git a/block/qcow2.h b/block/qcow2.h
index b12cecc..7f6e023 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -292,6 +292,11 @@ typedef struct BDRVQcow2State {
     unsigned int nb_snapshots;
     QCowSnapshot *snapshots;
 
+    uint64_t bitmap_directory_offset;
+    uint64_t bitmap_directory_size;
+    uint8_t *bitmap_directory;
+    unsigned int nb_bitmaps;
+
     int flags;
     int qcow_version;
     bool use_lazy_refcounts;
@@ -599,6 +604,10 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs,
 void qcow2_free_snapshots(BlockDriverState *bs);
 int qcow2_read_snapshots(BlockDriverState *bs);
 
+/* qcow2-bitmap.c functions */
+void qcow2_free_bitmaps(BlockDriverState *bs);
+int qcow2_read_bitmaps(BlockDriverState *bs, Error **errp);
+
 /* qcow2-cache.c functions */
 Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables);
 int qcow2_cache_destroy(BlockDriverState* bs, Qcow2Cache *c);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 06/29] qcow2-bitmap: add qcow2_read_bitmaps()

[Kevin Wolf]

Am 08.08.2016 um 17:04 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Add qcow2_read_bitmaps, reading bitmap directory as specified in
> docs/specs/qcow2.txt
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/qcow2-bitmap.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  block/qcow2.h        |   9 +++++
>  2 files changed, 109 insertions(+)
> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index cd18b07..91ddd5f 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -25,6 +25,12 @@
>   * THE SOFTWARE.
>   */
>  
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +
> +#include "block/block_int.h"
> +#include "block/qcow2.h"
> +
>  /* NOTICE: BME here means Bitmaps Extension and used as a namespace for
>   * _internal_ constants. Please do not use this _internal_ abbreviation for
>   * other needs and/or outside of this file. */
> @@ -42,6 +48,100 @@
>  /* bits [1, 8] U [56, 63] are reserved */
>  #define BME_TABLE_ENTRY_RESERVED_MASK 0xff000000000001fe
>  
> +#define for_each_bitmap_header_in_dir(h, dir, size) \
> +    for (h = (QCow2BitmapHeader *)(dir); \
> +         h < (QCow2BitmapHeader *)((uint8_t *)(dir) + size); \
> +         h = next_dir_entry(h))

It's hard to see just from this patch (see below), but 'size' contains
user input and cannot be trusted to be a multiple of sizeof(*h).
If it isn't, I think this loop will run for a final element where only
half of the QCow2BitmapHeader is covererd by size and a buffer overflow
follows.

> +/* directory_read
> + * Read bitmaps directory from bs by @offset and @size. Convert it to cpu
> + * format from BE.
> + */
> +static uint8_t *directory_read(BlockDriverState *bs,
> +                               uint64_t offset, uint64_t size, Error **errp)
> +{
> +    int ret;
> +    uint8_t *dir;
> +    QCow2BitmapHeader *h;
> +
> +    dir = g_try_malloc0(size);

This could be g_try_malloc without 0 because you immediately overwrite
all of it anyway.

> +    if (dir == NULL) {
> +        error_setg(errp, "Can't allocate space for bitmap directory.");
> +        return NULL;
> +    }
> +
> +    ret = bdrv_pread(bs->file, offset, dir, size);
> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret, "Can't read bitmap directory.");
> +        goto fail;
> +    }
> +
> +    /* loop is safe because next entry offset is calculated after conversion to
> +     * cpu format */
> +    for_each_bitmap_header_in_dir(h, dir, size) {
> +        bitmap_header_to_cpu(h);
> +    }
> +
> +    if ((uint8_t *)h != dir + size) {
> +        error_setg(errp, "Broken bitmap directory.");
> +        goto fail;
> +    }

Aha, you check for the unaligned case, but only after the damage has
already been done (you byteswapped bytes outside the allocated memory).

> +    return dir;
> +
> +fail:
> +    g_free(dir);
> +
> +    return NULL;
> +}
> +
> +int qcow2_read_bitmaps(BlockDriverState *bs, Error **errp)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +
> +    if (s->bitmap_directory != NULL) {
> +        error_setg(errp, "Try read bitmaps, when they are already read.");
> +        return -EEXIST;
> +    }

Is this error ever supposed to happen? If not, should this be assert()?

> +    if (s->nb_bitmaps == 0) {
> +        /* No bitmaps - nothing to do */
> +        return 0;
> +    }
> +
> +    s->bitmap_directory = directory_read(bs, s->bitmap_directory_offset,
> +                                         s->bitmap_directory_size, errp);
> +    if (s->bitmap_directory == NULL) {
> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}
> diff --git a/block/qcow2.h b/block/qcow2.h
> index b12cecc..7f6e023 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -292,6 +292,11 @@ typedef struct BDRVQcow2State {
>      unsigned int nb_snapshots;
>      QCowSnapshot *snapshots;
>  
> +    uint64_t bitmap_directory_offset;
> +    uint64_t bitmap_directory_size;
> +    uint8_t *bitmap_directory;
> +    unsigned int nb_bitmaps;

I think for a good review, patch 13 must come much earlier. Currently
you declare the variables, but they aren't actually initialised, so I
would have to guess what they could mean. And when reviewing patch 13 I
must remember what my assumptions were and check whether they match the
actual code. I know that I can't reliably do this.

So as a general rule of thumb, try to introduce things in an order that
every step can be reviewed and if possible also tested on its own rather
than introducing lots of dead code and putting all of it to use only in
the final patch.

Kevin

Re: [Qemu-devel] [PATCH 06/29] qcow2-bitmap: add qcow2_read_bitmaps()

[Vladimir Sementsov-Ogievskiy]

On 11.08.2016 12:36, Kevin Wolf wrote:
> Am 08.08.2016 um 17:04 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> Add qcow2_read_bitmaps, reading bitmap directory as specified in
>> docs/specs/qcow2.txt
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/qcow2-bitmap.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>   block/qcow2.h        |   9 +++++
>>   2 files changed, 109 insertions(+)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index cd18b07..91ddd5f 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -25,6 +25,12 @@
>>    * THE SOFTWARE.
>>    */
>>   
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +
>> +#include "block/block_int.h"
>> +#include "block/qcow2.h"
>> +
>>   /* NOTICE: BME here means Bitmaps Extension and used as a namespace for
>>    * _internal_ constants. Please do not use this _internal_ abbreviation for
>>    * other needs and/or outside of this file. */
>> @@ -42,6 +48,100 @@
>>   /* bits [1, 8] U [56, 63] are reserved */
>>   #define BME_TABLE_ENTRY_RESERVED_MASK 0xff000000000001fe
>>   
>> +#define for_each_bitmap_header_in_dir(h, dir, size) \
>> +    for (h = (QCow2BitmapHeader *)(dir); \
>> +         h < (QCow2BitmapHeader *)((uint8_t *)(dir) + size); \
>> +         h = next_dir_entry(h))
> It's hard to see just from this patch (see below), but 'size' contains
> user input and cannot be trusted to be a multiple of sizeof(*h).
> If it isn't, I think this loop will run for a final element where only
> half of the QCow2BitmapHeader is covererd by size and a buffer overflow
> follows.

this macro loops through the Bitmap Directory, so, here Bitmap Directory 
is defined as pair (dir, size), and size is a size of Bitmap Directory 
and by define it must be sum of all bitmap header sizes. However, you 
are right, something should be checked..  Like this I think:

bool check_dir_iter(QCow2BitmapHeader *it, void *directory_end) {
    return ((void *)it == directory_end) || ((void *)(it + 1) <= 
directory_end) && ((void *)next_dir_entry(it) <= directory_end);
}

+#define for_each_bitmap_header_in_dir(h, dir, size) \
+    for (h = (QCow2BitmapHeader *)(dir); \
+         assert(check_dir_iter(h)), h < (QCow2BitmapHeader *)((uint8_t *)(dir) + size); \
+         h = next_dir_entry(h))


And immediately after reading bitmap from file there should be similar 
checking loop but with error output instead of assert.

>
>> +/* directory_read
>> + * Read bitmaps directory from bs by @offset and @size. Convert it to cpu
>> + * format from BE.
>> + */
>> +static uint8_t *directory_read(BlockDriverState *bs,
>> +                               uint64_t offset, uint64_t size, Error **errp)
>> +{
>> +    int ret;
>> +    uint8_t *dir;
>> +    QCow2BitmapHeader *h;
>> +
>> +    dir = g_try_malloc0(size);
> This could be g_try_malloc without 0 because you immediately overwrite
> all of it anyway.
>
>> +    if (dir == NULL) {
>> +        error_setg(errp, "Can't allocate space for bitmap directory.");
>> +        return NULL;
>> +    }
>> +
>> +    ret = bdrv_pread(bs->file, offset, dir, size);
>> +    if (ret < 0) {
>> +        error_setg_errno(errp, -ret, "Can't read bitmap directory.");
>> +        goto fail;
>> +    }
>> +
>> +    /* loop is safe because next entry offset is calculated after conversion to
>> +     * cpu format */
>> +    for_each_bitmap_header_in_dir(h, dir, size) {
>> +        bitmap_header_to_cpu(h);
>> +    }
>> +
>> +    if ((uint8_t *)h != dir + size) {
>> +        error_setg(errp, "Broken bitmap directory.");
>> +        goto fail;
>> +    }
> Aha, you check for the unaligned case, but only after the damage has
> already been done (you byteswapped bytes outside the allocated memory).
>
>> +    return dir;
>> +
>> +fail:
>> +    g_free(dir);
>> +
>> +    return NULL;
>> +}
>> +
>> +int qcow2_read_bitmaps(BlockDriverState *bs, Error **errp)
>> +{
>> +    BDRVQcow2State *s = bs->opaque;
>> +
>> +    if (s->bitmap_directory != NULL) {
>> +        error_setg(errp, "Try read bitmaps, when they are already read.");
>> +        return -EEXIST;
>> +    }
> Is this error ever supposed to happen? If not, should this be assert()?
>
>> +    if (s->nb_bitmaps == 0) {
>> +        /* No bitmaps - nothing to do */
>> +        return 0;
>> +    }
>> +
>> +    s->bitmap_directory = directory_read(bs, s->bitmap_directory_offset,
>> +                                         s->bitmap_directory_size, errp);
>> +    if (s->bitmap_directory == NULL) {
>> +        return -EINVAL;
>> +    }
>> +
>> +    return 0;
>> +}
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index b12cecc..7f6e023 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -292,6 +292,11 @@ typedef struct BDRVQcow2State {
>>       unsigned int nb_snapshots;
>>       QCowSnapshot *snapshots;
>>   
>> +    uint64_t bitmap_directory_offset;
>> +    uint64_t bitmap_directory_size;
>> +    uint8_t *bitmap_directory;
>> +    unsigned int nb_bitmaps;
> I think for a good review, patch 13 must come much earlier. Currently
> you declare the variables, but they aren't actually initialised, so I
> would have to guess what they could mean. And when reviewing patch 13 I
> must remember what my assumptions were and check whether they match the
> actual code. I know that I can't reliably do this.
>
> So as a general rule of thumb, try to introduce things in an order that
> every step can be reviewed and if possible also tested on its own rather
> than introducing lots of dead code and putting all of it to use only in
> the final patch.

Ok, thanks for explanation.

>
> Kevin

Agree with all comments, will fix in next version

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 06/29] qcow2-bitmap: add qcow2_read_bitmaps()

[Kevin Wolf]

Am 11.08.2016 um 14:00 hat Vladimir Sementsov-Ogievskiy geschrieben:
> On 11.08.2016 12:36, Kevin Wolf wrote:
> >Am 08.08.2016 um 17:04 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >>Add qcow2_read_bitmaps, reading bitmap directory as specified in
> >>docs/specs/qcow2.txt
> >>
> >>Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >>---
> >>  block/qcow2-bitmap.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  block/qcow2.h        |   9 +++++
> >>  2 files changed, 109 insertions(+)
> >>
> >>diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> >>index cd18b07..91ddd5f 100644
> >>--- a/block/qcow2-bitmap.c
> >>+++ b/block/qcow2-bitmap.c
> >>@@ -25,6 +25,12 @@
> >>   * THE SOFTWARE.
> >>   */
> >>+#include "qemu/osdep.h"
> >>+#include "qapi/error.h"
> >>+
> >>+#include "block/block_int.h"
> >>+#include "block/qcow2.h"
> >>+
> >>  /* NOTICE: BME here means Bitmaps Extension and used as a namespace for
> >>   * _internal_ constants. Please do not use this _internal_ abbreviation for
> >>   * other needs and/or outside of this file. */
> >>@@ -42,6 +48,100 @@
> >>  /* bits [1, 8] U [56, 63] are reserved */
> >>  #define BME_TABLE_ENTRY_RESERVED_MASK 0xff000000000001fe
> >>+#define for_each_bitmap_header_in_dir(h, dir, size) \
> >>+    for (h = (QCow2BitmapHeader *)(dir); \
> >>+         h < (QCow2BitmapHeader *)((uint8_t *)(dir) + size); \
> >>+         h = next_dir_entry(h))
> >It's hard to see just from this patch (see below), but 'size' contains
> >user input and cannot be trusted to be a multiple of sizeof(*h).
> >If it isn't, I think this loop will run for a final element where only
> >half of the QCow2BitmapHeader is covererd by size and a buffer overflow
> >follows.
> 
> this macro loops through the Bitmap Directory, so, here Bitmap
> Directory is defined as pair (dir, size), and size is a size of
> Bitmap Directory and by define it must be sum of all bitmap header
> sizes.

For a correct images, yes. But for a malicious image, size can be
anything.

> However, you are right, something should be checked..  Like
> this I think:
> 
> bool check_dir_iter(QCow2BitmapHeader *it, void *directory_end) {
>    return ((void *)it == directory_end) || ((void *)(it + 1) <=
> directory_end) && ((void *)next_dir_entry(it) <= directory_end);
> }
> 
> +#define for_each_bitmap_header_in_dir(h, dir, size) \
> +    for (h = (QCow2BitmapHeader *)(dir); \
> +         assert(check_dir_iter(h)), h < (QCow2BitmapHeader *)((uint8_t *)(dir) + size); \
> +         h = next_dir_entry(h))
> 
> And immediately after reading bitmap from file there should be
> similar checking loop but with error output instead of assert.

If you have the check directly after reading the bitmap, then it doesn't
really matter any more what you do in for_each_bitmap_header_in_dir().
But yes, the assertion that you suggest looks good.

Kevin

[Qemu-devel] [PATCH 07/29] qcow2-bitmap: add qcow2_bitmap_load()

[Vladimir Sementsov-Ogievskiy]

This function loads block dirty bitmap from qcow2.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2-bitmap.c      | 165 ++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c             |   2 +
 block/qcow2.h             |   3 +
 include/block/block_int.h |   4 ++
 4 files changed, 174 insertions(+)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 91ddd5f..280b7bf 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -53,6 +53,10 @@
          h < (QCow2BitmapHeader *)((uint8_t *)(dir) + size); \
          h = next_dir_entry(h))
 
+#define for_each_bitmap_header(h, s) \
+    for_each_bitmap_header_in_dir(h, s->bitmap_directory, \
+                                  s->bitmap_directory_size)
+
 typedef enum BitmapType {
     BT_DIRTY_TRACKING_BITMAP = 1
 } BitmapType;
@@ -66,6 +70,15 @@ static inline void bitmap_header_to_cpu(QCow2BitmapHeader *h)
     be32_to_cpus(&h->extra_data_size);
 }
 
+static inline void bitmap_table_to_cpu(uint64_t *bitmap_table, size_t size)
+{
+    size_t i;
+
+    for (i = 0; i < size; ++i) {
+        be64_to_cpus(&bitmap_table[i]);
+    }
+}
+
 static inline int calc_dir_entry_size(size_t name_size)
 {
     return align_offset(sizeof(QCow2BitmapHeader) + name_size, 8);
@@ -145,3 +158,155 @@ int qcow2_read_bitmaps(BlockDriverState *bs, Error **errp)
 
     return 0;
 }
+
+static QCow2BitmapHeader *find_bitmap_by_name(BlockDriverState *bs,
+                                              const char *name)
+{
+    BDRVQcow2State *s = bs->opaque;
+    QCow2BitmapHeader *h;
+
+    for_each_bitmap_header(h, s) {
+        if (strncmp((char *)(h + 1), name, h->name_size) == 0) {
+            return h;
+        }
+    }
+
+    return NULL;
+}
+
+/* dirty sectors in cluster is a number of sectors in the image, corresponding
+ * to one cluster of bitmap data */
+static uint64_t dirty_sectors_in_cluster(const BDRVQcow2State *s,
+                                         const BdrvDirtyBitmap *bitmap)
+{
+    uint32_t sector_granularity =
+            bdrv_dirty_bitmap_granularity(bitmap) >> BDRV_SECTOR_BITS;
+
+    return (uint64_t)sector_granularity * (s->cluster_size << 3);
+}
+
+static int load_bitmap_data(BlockDriverState *bs,
+                            const uint64_t *dirty_bitmap_table,
+                            uint32_t dirty_bitmap_table_size,
+                            BdrvDirtyBitmap *bitmap)
+{
+    int ret = 0;
+    BDRVQcow2State *s = bs->opaque;
+    uint64_t sector, dsc;
+    uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
+    int cl_size = s->cluster_size;
+    uint8_t *buf = NULL;
+    uint32_t i, tab_size =
+            size_to_clusters(s,
+                bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));
+
+    if (tab_size != dirty_bitmap_table_size) {
+        return -EINVAL;
+    }
+
+    bdrv_clear_dirty_bitmap(bitmap, NULL);
+
+    buf = g_malloc0(cl_size);
+    dsc = dirty_sectors_in_cluster(s, bitmap);
+    for (i = 0, sector = 0; i < tab_size; ++i, sector += dsc) {
+        uint64_t end = MIN(bm_size, sector + dsc);
+        uint64_t offset = dirty_bitmap_table[i];
+
+        if (offset == 1) {
+            bdrv_dirty_bitmap_deserialize_ones(bitmap, sector, end, false);
+        } else if (offset > 1) {
+            ret = bdrv_pread(bs->file, offset, buf, cl_size);
+            if (ret < 0) {
+                goto finish;
+            }
+            bdrv_dirty_bitmap_deserialize_part(bitmap, buf, sector, end, false);
+        }
+    }
+    ret = 0;
+
+    bdrv_dirty_bitmap_deserialize_finish(bitmap);
+
+finish:
+    g_free(buf);
+
+    return ret;
+}
+
+static int bitmap_table_load(BlockDriverState *bs, QCow2BitmapHeader *bmh,
+                             uint64_t **table)
+{
+    int ret;
+
+    *table = g_try_new(uint64_t, bmh->bitmap_table_size);
+    if (*table == NULL) {
+        return -ENOMEM;
+    }
+
+    ret = bdrv_pread(bs->file, bmh->bitmap_table_offset,
+                     *table, bmh->bitmap_table_size * sizeof(uint64_t));
+    if (ret < 0) {
+        g_free(*table);
+        *table = NULL;
+        return ret;
+    }
+
+    bitmap_table_to_cpu(*table, bmh->bitmap_table_size);
+
+    return 0;
+}
+
+static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
+                                    QCow2BitmapHeader *bmh, Error **errp)
+{
+    int ret;
+    uint64_t *bitmap_table = NULL;
+    uint32_t granularity;
+    BdrvDirtyBitmap *bitmap = NULL;
+    char *name = g_strndup((char *)(bmh + 1), bmh->name_size);
+
+    ret = bitmap_table_load(bs, bmh, &bitmap_table);
+    if (ret < 0) {
+        error_setg_errno(errp, -ret,
+                         "Could not read bitmap_table table from image");
+        goto fail;
+    }
+
+    granularity = 1U << bmh->granularity_bits;
+    bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
+    if (bitmap == NULL) {
+        goto fail;
+    }
+
+    ret = load_bitmap_data(bs, bitmap_table, bmh->bitmap_table_size,
+                           bitmap);
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "Could not read bitmap from image");
+        goto fail;
+    }
+
+    g_free(name);
+    g_free(bitmap_table);
+    return bitmap;
+
+fail:
+    g_free(name);
+    g_free(bitmap_table);
+    if (bitmap != NULL) {
+        bdrv_release_dirty_bitmap(bs, bitmap);
+    }
+
+    return NULL;
+}
+
+BdrvDirtyBitmap *qcow2_bitmap_load(BlockDriverState *bs, const char *name,
+                                   Error **errp)
+{
+    QCow2BitmapHeader *h = find_bitmap_by_name(bs, name);
+    if (h == NULL) {
+        error_setg(errp, "Could not find bitmap '%s' in the node '%s'", name,
+                   bdrv_get_device_or_node_name(bs));
+        return NULL;
+    }
+
+    return load_bitmap(bs, h, errp);
+}
diff --git a/block/qcow2.c b/block/qcow2.c
index 91ef4df..74dab08 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3423,6 +3423,8 @@ BlockDriver bdrv_qcow2 = {
     .bdrv_get_info          = qcow2_get_info,
     .bdrv_get_specific_info = qcow2_get_specific_info,
 
+    .bdrv_dirty_bitmap_load = qcow2_bitmap_load,
+
     .bdrv_save_vmstate    = qcow2_save_vmstate,
     .bdrv_load_vmstate    = qcow2_load_vmstate,
 
diff --git a/block/qcow2.h b/block/qcow2.h
index 7f6e023..573fc36 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -608,6 +608,9 @@ int qcow2_read_snapshots(BlockDriverState *bs);
 void qcow2_free_bitmaps(BlockDriverState *bs);
 int qcow2_read_bitmaps(BlockDriverState *bs, Error **errp);
 
+BdrvDirtyBitmap *qcow2_bitmap_load(BlockDriverState *bs, const char *name,
+                                   Error **errp);
+
 /* qcow2-cache.c functions */
 Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables);
 int qcow2_cache_destroy(BlockDriverState* bs, Qcow2Cache *c);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 1fe0fd9..2c11ad7 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -224,6 +224,10 @@ struct BlockDriver {
     int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi);
     ImageInfoSpecific *(*bdrv_get_specific_info)(BlockDriverState *bs);
 
+    BdrvDirtyBitmap *(*bdrv_dirty_bitmap_load)(BlockDriverState *bs,
+                                               const char *name,
+                                               Error **errp);
+
     int coroutine_fn (*bdrv_save_vmstate)(BlockDriverState *bs,
                                           QEMUIOVector *qiov,
                                           int64_t pos);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 07/29] qcow2-bitmap: add qcow2_bitmap_load()

[Kevin Wolf]

Am 08.08.2016 um 17:04 hat Vladimir Sementsov-Ogievskiy geschrieben:
> This function loads block dirty bitmap from qcow2.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

As you said this is really unused at the moment, I'll mostly skip this
patch. Just one thing:

> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 1fe0fd9..2c11ad7 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -224,6 +224,10 @@ struct BlockDriver {
>      int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi);
>      ImageInfoSpecific *(*bdrv_get_specific_info)(BlockDriverState *bs);
>  
> +    BdrvDirtyBitmap *(*bdrv_dirty_bitmap_load)(BlockDriverState *bs,
> +                                               const char *name,
> +                                               Error **errp);
> +

I would prefer adding BlockDriver callbacks in the same patch as the
wrapper function, and keeping them separate from the first format driver
implementation. The reason for this is that sometimes people want to
backport the infrastructure, but not the driver change.

Similar to my comment on an earlier series, this means doing patch 16
(which this hunk added) first and only then implementing the function in
qcow2.

This specific instance probably disappears anyway as you remove the dead
code for now, but just to clarify what the preferred order is generally
(and I think you're going to re-submit it later, right?)

Kevin

[Qemu-devel] [PATCH 08/29] qcow2-bitmap: delete bitmap from qcow2 after load

[Vladimir Sementsov-Ogievskiy]

If we load bitmap for r/w bds, it's data in the image should be
considered inconsistent from this point. Therefore it is safe to remove
it from the image.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2-bitmap.c | 227 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 227 insertions(+)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 280b7bf..e677c31 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -61,6 +61,11 @@ typedef enum BitmapType {
     BT_DIRTY_TRACKING_BITMAP = 1
 } BitmapType;
 
+static inline bool can_write(BlockDriverState *bs)
+{
+    return !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
+}
+
 static inline void bitmap_header_to_cpu(QCow2BitmapHeader *h)
 {
     be64_to_cpus(&h->bitmap_table_offset);
@@ -70,6 +75,15 @@ static inline void bitmap_header_to_cpu(QCow2BitmapHeader *h)
     be32_to_cpus(&h->extra_data_size);
 }
 
+static inline void bitmap_header_to_be(QCow2BitmapHeader *h)
+{
+    cpu_to_be64s(&h->bitmap_table_offset);
+    cpu_to_be32s(&h->bitmap_table_size);
+    cpu_to_be32s(&h->flags);
+    cpu_to_be16s(&h->name_size);
+    cpu_to_be32s(&h->extra_data_size);
+}
+
 static inline void bitmap_table_to_cpu(uint64_t *bitmap_table, size_t size)
 {
     size_t i;
@@ -94,6 +108,17 @@ static inline QCow2BitmapHeader *next_dir_entry(QCow2BitmapHeader *entry)
     return (QCow2BitmapHeader *)((uint8_t *)entry + dir_entry_size(entry));
 }
 
+static inline void bitmap_directory_to_be(uint8_t *dir, size_t size)
+{
+    uint8_t *end = dir + size;
+    while (dir < end) {
+        QCow2BitmapHeader *h = (QCow2BitmapHeader *)dir;
+        dir += dir_entry_size(h);
+
+        bitmap_header_to_be(h);
+    }
+}
+
 /* directory_read
  * Read bitmaps directory from bs by @offset and @size. Convert it to cpu
  * format from BE.
@@ -174,6 +199,35 @@ static QCow2BitmapHeader *find_bitmap_by_name(BlockDriverState *bs,
     return NULL;
 }
 
+static void clear_bitmap_table(BlockDriverState *bs, uint64_t *bitmap_table,
+                               uint32_t bitmap_table_size)
+{
+    BDRVQcow2State *s = bs->opaque;
+    int cl_size = s->cluster_size;
+    int i;
+
+    for (i = 0; i < bitmap_table_size; ++i) {
+        uint64_t addr = bitmap_table[i];
+        if (addr <= 1) {
+            continue;
+        }
+
+        qcow2_free_clusters(bs, addr, cl_size, QCOW2_DISCARD_ALWAYS);
+        bitmap_table[i] = 0;
+    }
+}
+
+static void do_free_bitmap_clusters(BlockDriverState *bs,
+                                    uint64_t table_offset,
+                                    uint32_t table_size,
+                                    uint64_t *bitmap_table)
+{
+    clear_bitmap_table(bs, bitmap_table, table_size);
+
+    qcow2_free_clusters(bs, table_offset, table_size * sizeof(uint64_t),
+                        QCOW2_DISCARD_ALWAYS);
+}
+
 /* dirty sectors in cluster is a number of sectors in the image, corresponding
  * to one cluster of bitmap data */
 static uint64_t dirty_sectors_in_cluster(const BDRVQcow2State *s,
@@ -255,6 +309,165 @@ static int bitmap_table_load(BlockDriverState *bs, QCow2BitmapHeader *bmh,
     return 0;
 }
 
+static int update_header_sync(BlockDriverState *bs)
+{
+    int ret;
+
+    ret = qcow2_update_header(bs);
+    if (ret < 0) {
+        return ret;
+    }
+
+    ret = bdrv_flush(bs);
+    if (ret < 0) {
+        return ret;
+    }
+
+    return 0;
+}
+
+/* write bitmap directory from the state to new allocated clusters */
+static int64_t directory_write(BlockDriverState *bs, const uint8_t *dir,
+                               size_t size)
+{
+    int ret = 0;
+    uint8_t *dir_be = NULL;
+    int64_t dir_offset = 0;
+
+    dir_be = g_try_malloc(size);
+    if (dir_be == NULL) {
+        return -ENOMEM;
+    }
+    memcpy(dir_be, dir, size);
+    bitmap_directory_to_be(dir_be, size);
+
+    /* Allocate space for the new bitmap directory */
+    dir_offset = qcow2_alloc_clusters(bs, size);
+    if (dir_offset < 0) {
+        ret = dir_offset;
+        goto out;
+    }
+
+    /* The bitmap directory position has not yet been updated, so these
+     * clusters must indeed be completely free */
+    ret = qcow2_pre_write_overlap_check(bs, 0, dir_offset, size);
+    if (ret < 0) {
+        goto out;
+    }
+
+    ret = bdrv_pwrite(bs->file, dir_offset, dir_be, size);
+    if (ret < 0) {
+        goto out;
+    }
+
+out:
+    g_free(dir_be);
+
+    if (ret < 0) {
+        if (dir_offset > 0) {
+            qcow2_free_clusters(bs, dir_offset, size, QCOW2_DISCARD_ALWAYS);
+        }
+
+        return ret;
+    }
+
+    return dir_offset;
+}
+
+static int directory_update(BlockDriverState *bs, uint8_t *new_dir,
+                            size_t new_size, uint32_t new_nb_bitmaps)
+{
+    BDRVQcow2State *s = bs->opaque;
+    int ret;
+    int64_t new_offset = 0;
+    uint64_t old_offset = s->bitmap_directory_offset;
+    uint64_t old_size = s->bitmap_directory_size;
+    uint32_t old_nb_bitmaps = s->nb_bitmaps;
+    uint64_t old_autocl = s->autoclear_features;
+
+    if (new_size > QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE) {
+        return -EINVAL;
+    }
+
+    if ((new_size == 0) != (new_nb_bitmaps == 0)) {
+        return -EINVAL;
+    }
+
+    if (new_nb_bitmaps > 0) {
+        new_offset = directory_write(bs, new_dir, new_size);
+        if (new_offset < 0) {
+            return new_offset;
+        }
+
+        ret = bdrv_flush(bs);
+        if (ret < 0) {
+            goto fail;
+        }
+    }
+    s->bitmap_directory_offset = new_offset;
+    s->bitmap_directory_size = new_size;
+    s->nb_bitmaps = new_nb_bitmaps;
+
+    ret = update_header_sync(bs);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    if (old_size) {
+        qcow2_free_clusters(bs, old_offset, old_size, QCOW2_DISCARD_ALWAYS);
+    }
+
+    g_free(s->bitmap_directory);
+    s->bitmap_directory = new_dir;
+
+    return 0;
+
+fail:
+    if (new_offset > 0) {
+        qcow2_free_clusters(bs, new_offset, new_size, QCOW2_DISCARD_ALWAYS);
+        s->bitmap_directory_offset = old_offset;
+        s->bitmap_directory_size = old_size;
+        s->nb_bitmaps = old_nb_bitmaps;
+        s->autoclear_features = old_autocl;
+    }
+
+    return ret;
+}
+
+static int directory_del(BlockDriverState *bs, QCow2BitmapHeader *bmh)
+{
+    int ret;
+    BDRVQcow2State *s = bs->opaque;
+    uint8_t *new_dir = NULL;
+
+    size_t sz1 = (uint8_t *)bmh - s->bitmap_directory;
+    size_t sz2 = dir_entry_size(bmh);
+    size_t sz3 = s->bitmap_directory_size - sz1 - sz2;
+
+    uint64_t new_size = sz1 + sz3;
+
+    if (new_size > 0) {
+        new_dir = g_try_malloc(new_size);
+        if (new_dir == NULL) {
+            return -ENOMEM;
+        }
+        memcpy(new_dir, s->bitmap_directory, sz1);
+        memcpy(new_dir + sz1, s->bitmap_directory + sz1 + sz2, sz3);
+    }
+
+    ret = directory_update(bs, new_dir, new_size, s->nb_bitmaps - 1);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    return 0;
+
+fail:
+    g_free(new_dir);
+
+    return ret;
+}
+
 static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
                                     QCow2BitmapHeader *bmh, Error **errp)
 {
@@ -284,6 +497,20 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
         goto fail;
     }
 
+    if (can_write(bs)) {
+        uint64_t off = bmh->bitmap_table_offset;
+        uint32_t sz = bmh->bitmap_table_size;
+
+        ret = directory_del(bs, bmh);
+        if (ret < 0) {
+            error_setg_errno(errp, -ret,
+                             "Could not free bitmap after read.");
+            goto fail;
+        }
+
+        do_free_bitmap_clusters(bs, off, sz, bitmap_table);
+    }
+
     g_free(name);
     g_free(bitmap_table);
     return bitmap;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 08/29] qcow2-bitmap: delete bitmap from qcow2 after load

[Kevin Wolf]

Am 08.08.2016 um 17:04 hat Vladimir Sementsov-Ogievskiy geschrieben:
> If we load bitmap for r/w bds, it's data in the image should be
> considered inconsistent from this point. Therefore it is safe to remove
> it from the image.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

The approach that you're taking (keeping the complete bitmap directory
in memory in its on-disk format) means that we can't keep pointers to a
QCow2BitmapHeader anywhere because modifying the directory would move it
around.

Did you consider using a different in-memory representation like a
normal list of structs? This would also allow you to use the normal list
iteration mechanisms (i.e. the existing *_FOREACH macros) instead of the
rather complex ones you need now.

For example, qcow2 snapshots use a different on-disk and in-memory
format and the on-disk format is only implemented in the actual
read/write functions, and anything else can deal with the simpler
in-memory structures.

Kevin

Re: [Qemu-devel] [PATCH 08/29] qcow2-bitmap: delete bitmap from qcow2 after load

[Vladimir Sementsov-Ogievskiy]

On 11.08.2016 16:18, Kevin Wolf wrote:
> Am 08.08.2016 um 17:04 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> If we load bitmap for r/w bds, it's data in the image should be
>> considered inconsistent from this point. Therefore it is safe to remove
>> it from the image.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> The approach that you're taking (keeping the complete bitmap directory
> in memory in its on-disk format) means that we can't keep pointers to a
> QCow2BitmapHeader anywhere because modifying the directory would move it
> around.
>
> Did you consider using a different in-memory representation like a
> normal list of structs? This would also allow you to use the normal list
> iteration mechanisms (i.e. the existing *_FOREACH macros) instead of the
> rather complex ones you need now.
>
> For example, qcow2 snapshots use a different on-disk and in-memory
> format and the on-disk format is only implemented in the actual
> read/write functions, and anything else can deal with the simpler
> in-memory structures.
>
> Kevin

Hmm. It was some kind of optimization (not to rewrite the whole 
directory when rewriting one entry).. But for now, after deciding to 
clear bitmap directory after reading bitmaps, it(my approach) obviously 
becomes unnecessary complication.. So, I will change this.

-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH 09/29] qcow2-bitmap: add qcow2_bitmap_store()

[Vladimir Sementsov-Ogievskiy]

This function stores block dirty bitmap to qcow2. If the bitmap with
the same name, size and granularity already exists, it will be
rewritten, if the bitmap with the same name exists but granularity or
size does not match, an error will be generated.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2-bitmap.c      | 297 ++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c             |   1 +
 block/qcow2.h             |   2 +
 include/block/block_int.h |   3 +
 4 files changed, 303 insertions(+)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index e677c31..43a9bb9 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -27,6 +27,7 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qemu/cutils.h"
 
 #include "block/block_int.h"
 #include "block/qcow2.h"
@@ -93,6 +94,15 @@ static inline void bitmap_table_to_cpu(uint64_t *bitmap_table, size_t size)
     }
 }
 
+static inline void bitmap_table_to_be(uint64_t *bitmap_table, size_t size)
+{
+    size_t i;
+
+    for (i = 0; i < size; ++i) {
+        cpu_to_be64s(&bitmap_table[i]);
+    }
+}
+
 static inline int calc_dir_entry_size(size_t name_size)
 {
     return align_offset(sizeof(QCow2BitmapHeader) + name_size, 8);
@@ -537,3 +547,290 @@ BdrvDirtyBitmap *qcow2_bitmap_load(BlockDriverState *bs, const char *name,
 
     return load_bitmap(bs, h, errp);
 }
+
+/* store_bitmap_data()
+ * Store bitmap to image, filling bitamp table accordingly.
+ */
+static int store_bitmap_data(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+                             uint64_t *bitmap_table, uint32_t bitmap_table_size)
+{
+    int ret;
+    BDRVQcow2State *s = bs->opaque;
+    uint64_t sector, dsc;
+    uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
+    int cl_size = s->cluster_size;
+    uint8_t *buf = NULL;
+    uint32_t tb_size =
+            size_to_clusters(s,
+                bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));
+
+    BdrvDirtyBitmapIter *dbi;
+
+    if (tb_size != bitmap_table_size) {
+        return -EINVAL;
+    }
+
+    memset(bitmap_table, 0, bitmap_table_size * sizeof(bitmap_table[0]));
+
+    dbi = bdrv_dirty_iter_new(bitmap, 0);
+    buf = g_malloc(cl_size);
+    dsc = dirty_sectors_in_cluster(s, bitmap);
+
+    while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
+        uint64_t cluster = sector / dsc;
+        sector = cluster * dsc;
+        uint64_t end = MIN(bm_size, sector + dsc);
+        uint64_t write_size =
+            bdrv_dirty_bitmap_serialization_size(bitmap, sector, end - sector);
+
+        int64_t off = qcow2_alloc_clusters(bs, cl_size);
+        if (off < 0) {
+            ret = off;
+            goto finish;
+        }
+        bitmap_table[cluster] = off;
+
+        bdrv_dirty_bitmap_serialize_part(bitmap, buf, sector, end);
+        if (write_size < cl_size) {
+            memset(buf + write_size, 0, cl_size - write_size);
+        }
+
+        ret = bdrv_pwrite(bs->file, off, buf, cl_size);
+        if (ret < 0) {
+            goto finish;
+        }
+
+        if (end >= bm_size) {
+            break;
+        }
+
+        bdrv_set_dirty_iter(dbi, end);
+    }
+    ret = 0; /* writes */
+
+finish:
+    if (ret < 0) {
+        clear_bitmap_table(bs, bitmap_table, bitmap_table_size);
+    }
+    g_free(buf);
+    bdrv_dirty_iter_free(dbi);
+
+    return ret;
+}
+
+/* store_bitmap()
+ * Store bitmap to qcow2 and set bitmap_table. bitmap_table itself is not
+ * stored to qcow2.
+ */
+static int store_bitmap(BlockDriverState *bs,
+                        BdrvDirtyBitmap *bitmap,
+                        uint64_t **bitmap_table,
+                        uint64_t *bitmap_table_offset,
+                        uint32_t *bitmap_table_size)
+{
+    int ret;
+    BDRVQcow2State *s = bs->opaque;
+    uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
+
+    uint64_t *tb;
+    int64_t tb_offset;
+    uint32_t tb_size =
+            size_to_clusters(s,
+                bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));
+
+    tb = g_try_new(uint64_t, tb_size);
+    if (tb == NULL) {
+        return -ENOMEM;
+    }
+
+    ret = store_bitmap_data(bs, bitmap, tb, tb_size);
+    if (ret < 0) {
+        g_free(tb);
+        return ret;
+    }
+
+    tb_offset = qcow2_alloc_clusters(bs, tb_size * sizeof(tb[0]));
+    if (tb_offset < 0) {
+        ret = tb_offset;
+        goto fail;
+    }
+
+    bitmap_table_to_be(tb, tb_size);
+    ret = bdrv_pwrite(bs->file, tb_offset, tb, tb_size * sizeof(tb[0]));
+    if (ret < 0) {
+        goto fail;
+    }
+
+    if (bitmap_table != NULL) {
+        bitmap_table_to_cpu(tb, tb_size);
+        *bitmap_table = tb;
+    } else {
+        g_free(tb);
+    }
+    if (bitmap_table_offset != NULL) {
+        *bitmap_table_offset = tb_offset;
+    }
+    if (bitmap_table_size != NULL) {
+        *bitmap_table_size = tb_size;
+    }
+
+    return 0;
+
+fail:
+    clear_bitmap_table(bs, tb, tb_size);
+
+    if (tb_offset > 0) {
+        qcow2_free_clusters(bs, tb_offset, tb_size, QCOW2_DISCARD_ALWAYS);
+    }
+
+    g_free(tb);
+
+    return ret;
+}
+
+static int directory_push(BlockDriverState *bs, const char *name,
+        uint32_t granularity, uint64_t table_offset, uint32_t table_size,
+        uint32_t flags)
+{
+    int ret;
+    BDRVQcow2State *s = bs->opaque;
+    size_t name_size = strlen(name);
+    size_t entry_size = calc_dir_entry_size(name_size);
+    QCow2BitmapHeader *bmh = NULL;
+    uint64_t new_size = s->bitmap_directory_size + entry_size;
+    uint8_t *new_dir;
+
+    if (s->nb_bitmaps >= QCOW_MAX_DIRTY_BITMAPS) {
+        return -EFBIG;
+    }
+
+    if (new_size > QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE) {
+        return -EINVAL;
+    }
+
+    new_dir = g_try_malloc(new_size);
+    if (new_dir == NULL) {
+        return -ENOMEM;
+    }
+    memcpy(new_dir, s->bitmap_directory, s->bitmap_directory_size);
+
+    bmh = (QCow2BitmapHeader *)(new_dir + s->bitmap_directory_size);
+    bmh->bitmap_table_offset = table_offset;
+    bmh->bitmap_table_size = table_size;
+    bmh->flags = flags;
+    bmh->type = BT_DIRTY_TRACKING_BITMAP;
+    bmh->granularity_bits = ctz32(granularity);
+    bmh->name_size = name_size;
+    bmh->extra_data_size = 0;
+    memcpy(bmh + 1, name, name_size);
+
+    ret = directory_update(bs, new_dir, new_size, s->nb_bitmaps + 1);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    return 0;
+
+fail:
+    g_free(new_dir);
+
+    return ret;
+}
+
+static int directory_set(BlockDriverState *bs, QCow2BitmapHeader *bmh,
+        uint32_t granularity, uint64_t table_offset, uint32_t table_size,
+        uint32_t flags)
+{
+    int ret;
+    BDRVQcow2State *s = bs->opaque;
+    uint8_t *new_dir;
+
+    assert((uint8_t *)bmh >= s->bitmap_directory &&
+           (uint8_t *)bmh < s->bitmap_directory + s->bitmap_directory_size);
+
+    new_dir = g_memdup(s->bitmap_directory, s->bitmap_directory_size);
+    if (new_dir == NULL) {
+        return -ENOMEM;
+    }
+
+    bmh = (QCow2BitmapHeader *)
+            (new_dir + ((uint8_t *)bmh - s->bitmap_directory));
+    bmh->bitmap_table_offset = table_offset;
+    bmh->bitmap_table_size = table_size;
+    bmh->flags = flags;
+    bmh->granularity_bits = ctz32(granularity);
+
+    ret = directory_update(bs, new_dir, s->bitmap_directory_size,
+                           s->nb_bitmaps);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    return 0;
+
+fail:
+    g_free(new_dir);
+
+    return ret;
+}
+
+void qcow2_bitmap_store(BlockDriverState *bs,
+                        BdrvDirtyBitmap *bitmap, Error **errp)
+{
+    int ret = 0;
+    QCow2BitmapHeader *bmh;
+    const char *bm_name = bdrv_dirty_bitmap_name(bitmap);
+    uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
+    uint64_t table_offset;
+    uint32_t table_size;
+    uint64_t *bitmap_table;
+
+    /* find/create dirty bitmap */
+    bmh = find_bitmap_by_name(bs, bm_name);
+    if (bmh != NULL) {
+        if (granularity != (1U << bmh->granularity_bits)) {
+            error_setg(errp,
+                       "The bitmap with same name (but other granularity) "
+                       "already exists.");
+            return;
+        }
+
+        if (bmh->bitmap_table_offset) {
+            error_setg(errp,
+                       "The bitmap with same name already exists, but was"
+                       "not loaded.");
+            return;
+        }
+
+        assert(bmh->bitmap_table_size == 0);
+    }
+
+    ret = store_bitmap(bs, bitmap, &bitmap_table, &table_offset, &table_size);
+    if (ret < 0) {
+        error_setg_errno(errp, ret, "Can't store bitmap table.");
+        return;
+    }
+
+    if (bmh == NULL) {
+        ret = directory_push(bs, bm_name, granularity, table_offset,
+                             table_size, 0);
+        if (ret < 0) {
+            error_setg_errno(errp, ret, "Can't create dirty bitmap in qcow2.");
+            goto fail;
+        }
+    } else {
+        ret = directory_set(bs, bmh, granularity, table_offset, table_size,
+                            bmh->flags);
+        if (ret < 0) {
+            error_setg_errno(errp, ret, "Can't update dirty bitmap in qcow2.");
+            goto fail;
+        }
+    }
+
+    g_free(bitmap_table);
+    return;
+
+fail:
+    do_free_bitmap_clusters(bs, table_offset, table_size, bitmap_table);
+    g_free(bitmap_table);
+}
diff --git a/block/qcow2.c b/block/qcow2.c
index 74dab08..bad7b83 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3424,6 +3424,7 @@ BlockDriver bdrv_qcow2 = {
     .bdrv_get_specific_info = qcow2_get_specific_info,
 
     .bdrv_dirty_bitmap_load = qcow2_bitmap_load,
+    .bdrv_dirty_bitmap_store = qcow2_bitmap_store,
 
     .bdrv_save_vmstate    = qcow2_save_vmstate,
     .bdrv_load_vmstate    = qcow2_load_vmstate,
diff --git a/block/qcow2.h b/block/qcow2.h
index 573fc36..87b0a32 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -610,6 +610,8 @@ int qcow2_read_bitmaps(BlockDriverState *bs, Error **errp);
 
 BdrvDirtyBitmap *qcow2_bitmap_load(BlockDriverState *bs, const char *name,
                                    Error **errp);
+void qcow2_bitmap_store(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+                        Error **errp);
 
 /* qcow2-cache.c functions */
 Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 2c11ad7..ba002f3 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -227,6 +227,9 @@ struct BlockDriver {
     BdrvDirtyBitmap *(*bdrv_dirty_bitmap_load)(BlockDriverState *bs,
                                                const char *name,
                                                Error **errp);
+    void (*bdrv_dirty_bitmap_store)(BlockDriverState *bs,
+                                    BdrvDirtyBitmap *bitmap,
+                                    Error **errp);
 
     int coroutine_fn (*bdrv_save_vmstate)(BlockDriverState *bs,
                                           QEMUIOVector *qiov,
-- 
1.8.3.1

[Qemu-devel] [PATCH 10/29] qcow2-bitmap: add IN_USE flag

[Vladimir Sementsov-Ogievskiy]

This flag means that the bitmap is now in use by the software or was not
successfully saved. In any way, with this flag set the bitmap data must
be considered inconsistent and should not be loaded.
With current implementation this flag is never set, as we just remove
bitmaps from the image after loading. But it defined in qcow2 spec and
must be handled. Also, it can be used in future, if async schemes of
bitmap loading/saving are implemented.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2-bitmap.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 43a9bb9..19f8203 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -44,7 +44,8 @@
 #define BME_MAX_NAME_SIZE 1023
 
 /* Bitmap directory entry flags */
-#define BME_RESERVED_FLAGS 0xffffffff
+#define BME_RESERVED_FLAGS 0xfffffffe
+#define BME_FLAG_IN_USE 1
 
 /* bits [1, 8] U [56, 63] are reserved */
 #define BME_TABLE_ENTRY_RESERVED_MASK 0xff000000000001fe
@@ -487,6 +488,11 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
     BdrvDirtyBitmap *bitmap = NULL;
     char *name = g_strndup((char *)(bmh + 1), bmh->name_size);
 
+    if (bmh->flags & BME_FLAG_IN_USE) {
+        error_setg(errp, "Bitmap '%s' is in use", name);
+        goto fail;
+    }
+
     ret = bitmap_table_load(bs, bmh, &bitmap_table);
     if (ret < 0) {
         error_setg_errno(errp, -ret,
@@ -795,7 +801,8 @@ void qcow2_bitmap_store(BlockDriverState *bs,
             return;
         }
 
-        if (bmh->bitmap_table_offset) {
+        if ((bmh->bitmap_table_offset != 0) ||
+                !(bmh->flags & BME_FLAG_IN_USE)) {
             error_setg(errp,
                        "The bitmap with same name already exists, but was"
                        "not loaded.");
@@ -820,7 +827,7 @@ void qcow2_bitmap_store(BlockDriverState *bs,
         }
     } else {
         ret = directory_set(bs, bmh, granularity, table_offset, table_size,
-                            bmh->flags);
+                            bmh->flags & ~BME_FLAG_IN_USE);
         if (ret < 0) {
             error_setg_errno(errp, ret, "Can't update dirty bitmap in qcow2.");
             goto fail;
-- 
1.8.3.1

[Qemu-devel] [PATCH 11/29] qcow2-bitmap: check constraints

[Vladimir Sementsov-Ogievskiy]

Check bitmap header constraints as specified in docs/specs/qcow2.txt

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2-bitmap.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 19f8203..0c0cb7c 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -130,6 +130,34 @@ static inline void bitmap_directory_to_be(uint8_t *dir, size_t size)
     }
 }
 
+static int check_constraints(BlockDriverState *bs, QCow2BitmapHeader *h)
+{
+    BDRVQcow2State *s = bs->opaque;
+    uint64_t phys_bitmap_bytes =
+        (uint64_t)h->bitmap_table_size * s->cluster_size;
+    uint64_t max_virtual_bits = (phys_bitmap_bytes * 8) << h->granularity_bits;
+    int64_t nb_sectors = bdrv_nb_sectors(bs);
+
+    if (nb_sectors < 0) {
+        return nb_sectors;
+    }
+
+    int fail =
+            ((h->bitmap_table_size == 0) != (h->bitmap_table_offset == 0)) ||
+            (h->bitmap_table_offset % s->cluster_size) ||
+            (h->bitmap_table_size > BME_MAX_TABLE_SIZE) ||
+            (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) ||
+            (h->bitmap_table_offset != 0 &&
+                (nb_sectors << BDRV_SECTOR_BITS) > max_virtual_bits) ||
+            (h->granularity_bits > BME_MAX_GRANULARITY_BITS) ||
+            (h->granularity_bits < BME_MIN_GRANULARITY_BITS) ||
+            (h->flags & BME_RESERVED_FLAGS) ||
+            (h->name_size > BME_MAX_NAME_SIZE) ||
+            (h->type != BT_DIRTY_TRACKING_BITMAP);
+
+    return fail ? -EINVAL : 0;
+}
+
 /* directory_read
  * Read bitmaps directory from bs by @offset and @size. Convert it to cpu
  * format from BE.
@@ -157,6 +185,12 @@ static uint8_t *directory_read(BlockDriverState *bs,
      * cpu format */
     for_each_bitmap_header_in_dir(h, dir, size) {
         bitmap_header_to_cpu(h);
+
+        ret = check_constraints(bs, h);
+        if (ret < 0) {
+            error_setg(errp, "Bitmap doesn't satisfy the constraints.");
+            goto fail;
+        }
     }
 
     if ((uint8_t *)h != dir + size) {
@@ -730,6 +764,11 @@ static int directory_push(BlockDriverState *bs, const char *name,
     bmh->extra_data_size = 0;
     memcpy(bmh + 1, name, name_size);
 
+    ret = check_constraints(bs, bmh);
+    if (ret < 0) {
+        goto fail;
+    }
+
     ret = directory_update(bs, new_dir, new_size, s->nb_bitmaps + 1);
     if (ret < 0) {
         goto fail;
-- 
1.8.3.1

[Qemu-devel] [PATCH 12/29] qcow2: add qcow2_delete_bitmaps

[Vladimir Sementsov-Ogievskiy]

Add function to delete dirty bitmaps from image. It will be used in
truncate.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 block/qcow2-bitmap.c | 34 ++++++++++++++++++++++++++++++++++
 block/qcow2.h        |  1 +
 2 files changed, 35 insertions(+)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 0c0cb7c..92d95c5 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -273,6 +273,24 @@ static void do_free_bitmap_clusters(BlockDriverState *bs,
                         QCOW2_DISCARD_ALWAYS);
 }
 
+static int bitmap_table_load(BlockDriverState *bs, QCow2BitmapHeader *bmh,
+                             uint64_t **table);
+static int free_bitmap_clusters(BlockDriverState *bs, QCow2BitmapHeader *bmh)
+{
+    int ret;
+    uint64_t *bitmap_table;
+
+    ret = bitmap_table_load(bs, bmh, &bitmap_table);
+    if (ret < 0 || bitmap_table == NULL) {
+        return ret;
+    }
+
+    do_free_bitmap_clusters(bs, bmh->bitmap_table_offset,
+                            bmh->bitmap_table_size, bitmap_table);
+
+    return 0;
+}
+
 /* dirty sectors in cluster is a number of sectors in the image, corresponding
  * to one cluster of bitmap data */
 static uint64_t dirty_sectors_in_cluster(const BDRVQcow2State *s,
@@ -588,6 +606,22 @@ BdrvDirtyBitmap *qcow2_bitmap_load(BlockDriverState *bs, const char *name,
     return load_bitmap(bs, h, errp);
 }
 
+int qcow2_delete_bitmaps(BlockDriverState *bs)
+{
+    BDRVQcow2State *s = bs->opaque;
+    QCow2BitmapHeader *h;
+
+    if (s->nb_bitmaps == 0) {
+        return 0;
+    }
+
+    for_each_bitmap_header(h, s) {
+        free_bitmap_clusters(bs, h);
+    }
+
+    return directory_update(bs, NULL, 0, 0);
+}
+
 /* store_bitmap_data()
  * Store bitmap to image, filling bitamp table accordingly.
  */
diff --git a/block/qcow2.h b/block/qcow2.h
index 87b0a32..0ec0b31 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -612,6 +612,7 @@ BdrvDirtyBitmap *qcow2_bitmap_load(BlockDriverState *bs, const char *name,
                                    Error **errp);
 void qcow2_bitmap_store(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
                         Error **errp);
+int qcow2_delete_bitmaps(BlockDriverState *bs);
 
 /* qcow2-cache.c functions */
 Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables);
-- 
1.8.3.1

[Qemu-devel] [PATCH 13/29] qcow2: add dirty bitmaps extension

[Vladimir Sementsov-Ogievskiy]

Add dirty bitmap extension as specified in docs/specs/qcow2.txt.

Load bitmap headers on open. Handle close and update_header.

Handle resize: just delete all bitmaps from the image. Not loaded
bitmaps will be lost (for now - all except autoloading ones). This may
be fixed in future.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2-bitmap.c |  8 +++++
 block/qcow2.c        | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 101 insertions(+)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 92d95c5..a34cfe8 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -68,6 +68,14 @@ static inline bool can_write(BlockDriverState *bs)
     return !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
 }
 
+void qcow2_free_bitmaps(BlockDriverState *bs)
+{
+    BDRVQcow2State *s = bs->opaque;
+
+    g_free(s->bitmap_directory);
+    s->bitmap_directory = NULL;
+}
+
 static inline void bitmap_header_to_cpu(QCow2BitmapHeader *h)
 {
     be64_to_cpus(&h->bitmap_table_offset);
diff --git a/block/qcow2.c b/block/qcow2.c
index bad7b83..3e9b47e 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -63,6 +63,7 @@ typedef struct {
 #define  QCOW2_EXT_MAGIC_END 0
 #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
 #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
+#define  QCOW2_EXT_MAGIC_DIRTY_BITMAPS 0x23852875
 
 static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
 {
@@ -92,6 +93,7 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
     QCowExtension ext;
     uint64_t offset;
     int ret;
+    Qcow2BitmapHeaderExt bitmaps_ext;
 
 #ifdef DEBUG_EXT
     printf("qcow2_read_extensions: start=%ld end=%ld\n", start_offset, end_offset);
@@ -162,6 +164,67 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
             }
             break;
 
+        case QCOW2_EXT_MAGIC_DIRTY_BITMAPS:
+            ret = bdrv_pread(bs->file, offset, &bitmaps_ext, ext.len);
+            if (ret < 0) {
+                error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
+                                 "Could not read ext header");
+                return ret;
+            }
+
+            if (bitmaps_ext.reserved32 != 0) {
+                error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
+                                 "Reserved field is not zero.");
+                return -EINVAL;
+            }
+
+            be32_to_cpus(&bitmaps_ext.nb_bitmaps);
+            be64_to_cpus(&bitmaps_ext.bitmap_directory_size);
+            be64_to_cpus(&bitmaps_ext.bitmap_directory_offset);
+
+            if (bitmaps_ext.nb_bitmaps > QCOW_MAX_DIRTY_BITMAPS) {
+                error_setg(errp, "ERROR: bitmaps_ext: "
+                                 "too many dirty bitmaps");
+                return -EINVAL;
+            }
+
+            if (bitmaps_ext.nb_bitmaps == 0) {
+                error_setg(errp, "ERROR: bitmaps_ext: "
+                                 "found bitmaps extension with zero bitmaps");
+                return -EINVAL;
+            }
+
+            if (bitmaps_ext.bitmap_directory_offset & (s->cluster_size - 1)) {
+                error_setg(errp, "ERROR: bitmaps_ext: "
+                                 "wrong dirty bitmap directory offset");
+                return -EINVAL;
+            }
+
+            if (bitmaps_ext.bitmap_directory_size >
+                QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE) {
+                error_setg(errp, "ERROR: bitmaps_ext: "
+                                 "too large dirty bitmap directory");
+                return -EINVAL;
+            }
+
+            s->nb_bitmaps = bitmaps_ext.nb_bitmaps;
+            s->bitmap_directory_offset =
+                    bitmaps_ext.bitmap_directory_offset;
+            s->bitmap_directory_size =
+                    bitmaps_ext.bitmap_directory_size;
+
+            ret = qcow2_read_bitmaps(bs, errp);
+            if (ret < 0) {
+                return ret;
+            }
+
+#ifdef DEBUG_EXT
+            printf("Qcow2: Got dirty bitmaps extension:"
+                   " offset=%" PRIu64 " nb_bitmaps=%" PRIu32 "\n",
+                   s->bitmap_directory_offset, s->nb_bitmaps);
+#endif
+            break;
+
         default:
             /* unknown magic - save it in case we need to rewrite the header */
             {
@@ -1179,6 +1242,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     g_free(s->unknown_header_fields);
     cleanup_unknown_header_ext(bs);
     qcow2_free_snapshots(bs);
+    qcow2_free_bitmaps(bs);
     qcow2_refcount_close(bs);
     qemu_vfree(s->l1_table);
     /* else pre-write overlap checks in cache_destroy may crash */
@@ -1749,6 +1813,7 @@ static void qcow2_close(BlockDriverState *bs)
     qemu_vfree(s->cluster_data);
     qcow2_refcount_close(bs);
     qcow2_free_snapshots(bs);
+    qcow2_free_bitmaps(bs);
 }
 
 static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
@@ -1939,6 +2004,24 @@ int qcow2_update_header(BlockDriverState *bs)
         buflen -= ret;
     }
 
+    if (s->nb_bitmaps > 0) {
+        Qcow2BitmapHeaderExt bitmaps_header = {
+            .nb_bitmaps = cpu_to_be32(s->nb_bitmaps),
+            .bitmap_directory_size =
+                    cpu_to_be64(s->bitmap_directory_size),
+            .bitmap_directory_offset =
+                    cpu_to_be64(s->bitmap_directory_offset)
+        };
+        ret = header_ext_add(buf, QCOW2_EXT_MAGIC_DIRTY_BITMAPS,
+                             &bitmaps_header, sizeof(bitmaps_header),
+                             buflen);
+        if (ret < 0) {
+            goto fail;
+        }
+        buf += ret;
+        buflen -= ret;
+    }
+
     /* Keep unknown header extensions */
     QLIST_FOREACH(uext, &s->unknown_header_ext, next) {
         ret = header_ext_add(buf, uext->magic, uext->data, uext->len, buflen);
@@ -2509,6 +2592,16 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
         return -ENOTSUP;
     }
 
+    /* cannot proceed if image has bitmaps */
+    if (s->nb_bitmaps) {
+        /* FIXME: not loaded bitmaps will be lost */
+        ret = qcow2_delete_bitmaps(bs);
+        if (ret < 0) {
+            error_report("Can't remove bitmaps from qcow2 on truncate");
+            return ret;
+        }
+    }
+
     /* shrinking is currently not supported */
     if (offset < bs->total_sectors * 512) {
         error_report("qcow2 doesn't support shrinking images yet");
-- 
1.8.3.1

[Qemu-devel] [PATCH 14/29] qcow2-bitmap: add qcow2_bitmap_load_check()

[Vladimir Sementsov-Ogievskiy]

The function checks the existance of the bitmap without loading it.
Will be used in future patches.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/dirty-bitmap.c         | 15 +++++++++++++++
 block/qcow2-bitmap.c         |  5 +++++
 block/qcow2.c                |  1 +
 block/qcow2.h                |  1 +
 include/block/block_int.h    |  2 ++
 include/block/dirty-bitmap.h |  2 ++
 6 files changed, 26 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 90af372..83415e1 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -540,3 +540,18 @@ int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
 {
     return hbitmap_count(bitmap->meta);
 }
+
+bool bdrv_load_check_dirty_bitmap(BlockDriverState *file, const char *name)
+{
+    BlockDriver *drv = file->drv;
+    if (!drv) {
+        return false;
+    }
+    if (drv->bdrv_dirty_bitmap_load_check) {
+        return drv->bdrv_dirty_bitmap_load_check(file, name);
+    }
+    if (file->file)  {
+        return bdrv_load_check_dirty_bitmap(file->file->bs, name);
+    }
+    return false;
+}
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index a34cfe8..32f1a50 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -252,6 +252,11 @@ static QCow2BitmapHeader *find_bitmap_by_name(BlockDriverState *bs,
     return NULL;
 }
 
+bool qcow2_bitmap_load_check(BlockDriverState *file, const char *name)
+{
+    return find_bitmap_by_name(file, name) != NULL;
+}
+
 static void clear_bitmap_table(BlockDriverState *bs, uint64_t *bitmap_table,
                                uint32_t bitmap_table_size)
 {
diff --git a/block/qcow2.c b/block/qcow2.c
index 3e9b47e..60b0acb 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3517,6 +3517,7 @@ BlockDriver bdrv_qcow2 = {
     .bdrv_get_specific_info = qcow2_get_specific_info,
 
     .bdrv_dirty_bitmap_load = qcow2_bitmap_load,
+    .bdrv_dirty_bitmap_load_check = qcow2_bitmap_load_check,
     .bdrv_dirty_bitmap_store = qcow2_bitmap_store,
 
     .bdrv_save_vmstate    = qcow2_save_vmstate,
diff --git a/block/qcow2.h b/block/qcow2.h
index 0ec0b31..d4515fb 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -608,6 +608,7 @@ int qcow2_read_snapshots(BlockDriverState *bs);
 void qcow2_free_bitmaps(BlockDriverState *bs);
 int qcow2_read_bitmaps(BlockDriverState *bs, Error **errp);
 
+bool qcow2_bitmap_load_check(BlockDriverState *file, const char *name);
 BdrvDirtyBitmap *qcow2_bitmap_load(BlockDriverState *bs, const char *name,
                                    Error **errp);
 void qcow2_bitmap_store(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index ba002f3..da1f843 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -224,6 +224,8 @@ struct BlockDriver {
     int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi);
     ImageInfoSpecific *(*bdrv_get_specific_info)(BlockDriverState *bs);
 
+    bool (*bdrv_dirty_bitmap_load_check)(BlockDriverState *file,
+                                         const char *name);
     BdrvDirtyBitmap *(*bdrv_dirty_bitmap_load)(BlockDriverState *bs,
                                                const char *name,
                                                Error **errp);
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 1e17729..6007c64 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -75,4 +75,6 @@ void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap,
                                         bool finish);
 void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
 
+bool bdrv_load_check_dirty_bitmap(BlockDriverState *file, const char *name);
+
 #endif
-- 
1.8.3.1

[Qemu-devel] [PATCH 15/29] block/dirty-bitmap: introduce persistent bitmaps

[Vladimir Sementsov-Ogievskiy]

Add field 'persistent' to BdrvDirtyBitmap. Store all persistent bitmaps
of the BDS in bdrv_close().

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c                      |  2 ++
 block/dirty-bitmap.c         | 42 ++++++++++++++++++++++++++++++++++++++++++
 include/block/dirty-bitmap.h |  5 +++++
 3 files changed, 49 insertions(+)

diff --git a/block.c b/block.c
index 30d64e6..fdef1b2 100644
--- a/block.c
+++ b/block.c
@@ -2155,6 +2155,8 @@ static void bdrv_close(BlockDriverState *bs)
     bdrv_flush(bs);
     bdrv_drain(bs); /* in case flush left pending I/O */
 
+    /* save and release persistent dirty bitmaps */
+    bdrv_finalize_persistent_dirty_bitmaps(bs);
     bdrv_release_named_dirty_bitmaps(bs);
     assert(QLIST_EMPTY(&bs->dirty_bitmaps));
 
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 83415e1..6df7fe1 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -44,6 +44,7 @@ struct BdrvDirtyBitmap {
     int64_t size;               /* Size of the bitmap (Number of sectors) */
     bool disabled;              /* Bitmap is read-only */
     int active_iterators;       /* How many iterators are active */
+    bool persistent;            /* bitmap must be saved to owner disk image */
     QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
 
@@ -70,6 +71,8 @@ void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap)
     assert(!bdrv_dirty_bitmap_frozen(bitmap));
     g_free(bitmap->name);
     bitmap->name = NULL;
+
+    bitmap->persistent = false;
 }
 
 BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
@@ -238,6 +241,8 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
     bitmap->name = NULL;
     successor->name = name;
     bitmap->successor = NULL;
+    successor->persistent = bitmap->persistent;
+    bitmap->persistent = false;
     bdrv_release_dirty_bitmap(bs, bitmap);
 
     return successor;
@@ -555,3 +560,40 @@ bool bdrv_load_check_dirty_bitmap(BlockDriverState *file, const char *name)
     }
     return false;
 }
+
+void bdrv_store_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+                             Error **errp)
+{
+    if (bs == NULL || bs->drv == NULL ||
+            bs->drv->bdrv_dirty_bitmap_store == NULL) {
+        error_setg(errp, "Storing bitmap is unsupported for the format.");
+        return;
+    }
+
+    bs->drv->bdrv_dirty_bitmap_store(bs, bitmap, errp);
+}
+
+void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
+                                                bool persistent)
+{
+    bitmap->persistent = persistent;
+}
+
+void bdrv_finalize_persistent_dirty_bitmaps(BlockDriverState *bs)
+{
+    BdrvDirtyBitmap *bm, *bm_next;
+
+    QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, bm_next) {
+        if (bm->persistent) {
+            if (!bdrv_is_read_only(bs) &&
+                     !(bdrv_get_flags(bs) & BDRV_O_INACTIVE)) {
+                Error *local_err = NULL;
+                bdrv_store_dirty_bitmap(bs, bm, &local_err);
+                if (local_err) {
+                    error_report_err(local_err);
+                }
+            }
+            bdrv_release_dirty_bitmap(bs, bm);
+        }
+    }
+}
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 6007c64..05aa7f9 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -76,5 +76,10 @@ void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap,
 void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
 
 bool bdrv_load_check_dirty_bitmap(BlockDriverState *file, const char *name);
+void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
+                                                bool persistent);
+void bdrv_store_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+                             Error **errp);
+void bdrv_finalize_persistent_dirty_bitmaps(BlockDriverState *bs);
 
 #endif
-- 
1.8.3.1

[Qemu-devel] [PATCH 16/29] block: add bdrv_load_dirty_bitmap()

[Vladimir Sementsov-Ogievskiy]

The funcion loads dirty bitmap from file, using underlying driver
function.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/dirty-bitmap.c         | 16 ++++++++++++++++
 include/block/dirty-bitmap.h |  2 ++
 2 files changed, 18 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 6df7fe1..1d0ea25 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -597,3 +597,19 @@ void bdrv_finalize_persistent_dirty_bitmaps(BlockDriverState *bs)
         }
     }
 }
+
+BdrvDirtyBitmap *bdrv_load_dirty_bitmap(BlockDriverState *bs, const char *name,
+                                        Error **errp)
+{
+    BlockDriver *drv = bs->drv;
+    if (!drv) {
+        return NULL;
+    }
+    if (drv->bdrv_dirty_bitmap_load) {
+        return drv->bdrv_dirty_bitmap_load(bs, name, errp);
+    }
+    if (bs->file)  {
+        return bdrv_load_dirty_bitmap(bs, name, errp);
+    }
+    return NULL;
+}
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 05aa7f9..d482098 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -81,5 +81,7 @@ void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
 void bdrv_store_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
                              Error **errp);
 void bdrv_finalize_persistent_dirty_bitmaps(BlockDriverState *bs);
+BdrvDirtyBitmap *bdrv_load_dirty_bitmap(BlockDriverState *bs, const char *name,
+                                        Error **errp);
 
 #endif
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 16/29] block: add bdrv_load_dirty_bitmap()

[Kevin Wolf]

Am 08.08.2016 um 17:05 hat Vladimir Sementsov-Ogievskiy geschrieben:
> The funcion loads dirty bitmap from file, using underlying driver
> function.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/dirty-bitmap.c         | 16 ++++++++++++++++
>  include/block/dirty-bitmap.h |  2 ++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 6df7fe1..1d0ea25 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -597,3 +597,19 @@ void bdrv_finalize_persistent_dirty_bitmaps(BlockDriverState *bs)
>          }
>      }
>  }
> +
> +BdrvDirtyBitmap *bdrv_load_dirty_bitmap(BlockDriverState *bs, const char *name,
> +                                        Error **errp)
> +{
> +    BlockDriver *drv = bs->drv;
> +    if (!drv) {
> +        return NULL;
> +    }
> +    if (drv->bdrv_dirty_bitmap_load) {
> +        return drv->bdrv_dirty_bitmap_load(bs, name, errp);

Why the inconsistency between load_dirty and dirty_load? The bdrv_*
wrappers usually have exactly the same name as the BlockDriver fields.

> +    }
> +    if (bs->file)  {
> +        return bdrv_load_dirty_bitmap(bs, name, errp);
> +    }
> +    return NULL;
> +}

Let me ask a general question about this series: What is the expected
state after it is applied?

I'm asking because even after the full series is applied, I don't see a
single user of bdrv_load_dirty_bitmap(), bdrv_load_check_dirty_bitmap()
or bdrv_store_dirty_bitmap(). Is all of this dead code?

Kevin

Re: [Qemu-devel] [PATCH 16/29] block: add bdrv_load_dirty_bitmap()

[Vladimir Sementsov-Ogievskiy]

On 11.08.2016 14:24, Kevin Wolf wrote:
> Am 08.08.2016 um 17:05 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> The funcion loads dirty bitmap from file, using underlying driver
>> function.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/dirty-bitmap.c         | 16 ++++++++++++++++
>>   include/block/dirty-bitmap.h |  2 ++
>>   2 files changed, 18 insertions(+)
>>
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 6df7fe1..1d0ea25 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -597,3 +597,19 @@ void bdrv_finalize_persistent_dirty_bitmaps(BlockDriverState *bs)
>>           }
>>       }
>>   }
>> +
>> +BdrvDirtyBitmap *bdrv_load_dirty_bitmap(BlockDriverState *bs, const char *name,
>> +                                        Error **errp)
>> +{
>> +    BlockDriver *drv = bs->drv;
>> +    if (!drv) {
>> +        return NULL;
>> +    }
>> +    if (drv->bdrv_dirty_bitmap_load) {
>> +        return drv->bdrv_dirty_bitmap_load(bs, name, errp);
> Why the inconsistency between load_dirty and dirty_load? The bdrv_*
> wrappers usually have exactly the same name as the BlockDriver fields.
>
>> +    }
>> +    if (bs->file)  {
>> +        return bdrv_load_dirty_bitmap(bs, name, errp);
>> +    }
>> +    return NULL;
>> +}
> Let me ask a general question about this series: What is the expected
> state after it is applied?
>
> I'm asking because even after the full series is applied, I don't see a
> single user of bdrv_load_dirty_bitmap(), bdrv_load_check_dirty_bitmap()
> or bdrv_store_dirty_bitmap(). Is all of this dead code?

Hmm, you are right, it's my mistake. It is dead code after removing 
command line options for dirty bitmaps from the series. Also, for now 
only autoloading bitmaps can be loaded. I'll remove these functions in 
next version, they may be added later, when we need loading bitmaps by 
request (qmp command or cmd line).

>
> Kevin


-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH 17/29] qcow2-bitmap: add autoclear bit

[Vladimir Sementsov-Ogievskiy]

Add autoclear bit for handling rewriting image by old qemu version.

If autoclear bit is not set, but bitmaps extension is found it
would not be loaded and warning will be generated.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2-bitmap.c |  4 ++++
 block/qcow2.c        | 12 ++++++++++--
 block/qcow2.h        |  9 +++++++++
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 32f1a50..0e95121 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -479,6 +479,10 @@ static int directory_update(BlockDriverState *bs, uint8_t *new_dir,
         if (ret < 0) {
             goto fail;
         }
+
+        s->autoclear_features |= QCOW2_AUTOCLEAR_DIRTY_BITMAPS;
+    } else {
+        s->autoclear_features &= ~(uint64_t)QCOW2_AUTOCLEAR_DIRTY_BITMAPS;
     }
     s->bitmap_directory_offset = new_offset;
     s->bitmap_directory_size = new_size;
diff --git a/block/qcow2.c b/block/qcow2.c
index 60b0acb..76ad7c6 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -165,6 +165,13 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
             break;
 
         case QCOW2_EXT_MAGIC_DIRTY_BITMAPS:
+            if (!(s->autoclear_features & QCOW2_AUTOCLEAR_DIRTY_BITMAPS)) {
+                fprintf(stderr,
+                        "WARNING: bitmaps_ext: autoclear flag is not "
+                        "set, all bitmaps will be considered as inconsistent");
+                break;
+            }
+
             ret = bdrv_pread(bs->file, offset, &bitmaps_ext, ext.len);
             if (ret < 0) {
                 error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
@@ -1206,8 +1213,9 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     /* Clear unknown autoclear feature bits */
-    if (!bs->read_only && !(flags & BDRV_O_INACTIVE) && s->autoclear_features) {
-        s->autoclear_features = 0;
+    if (!bs->read_only && !(flags & BDRV_O_INACTIVE) &&
+        (s->autoclear_features & ~QCOW2_AUTOCLEAR_MASK)) {
+        s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
         ret = qcow2_update_header(bs);
         if (ret < 0) {
             error_setg_errno(errp, -ret, "Could not update qcow2 header");
diff --git a/block/qcow2.h b/block/qcow2.h
index d4515fb..74d395b 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -215,6 +215,15 @@ enum {
     QCOW2_COMPAT_FEAT_MASK            = QCOW2_COMPAT_LAZY_REFCOUNTS,
 };
 
+/* Autoclear feature bits */
+enum {
+    QCOW2_AUTOCLEAR_DIRTY_BITMAPS_BITNR = 0,
+    QCOW2_AUTOCLEAR_DIRTY_BITMAPS       =
+        1 << QCOW2_AUTOCLEAR_DIRTY_BITMAPS_BITNR,
+
+    QCOW2_AUTOCLEAR_MASK                = QCOW2_AUTOCLEAR_DIRTY_BITMAPS,
+};
+
 enum qcow2_discard_type {
     QCOW2_DISCARD_NEVER = 0,
     QCOW2_DISCARD_ALWAYS,
-- 
1.8.3.1

[Qemu-devel] [PATCH 18/29] qcow2-bitmap: disallow storing bitmap to other bs

[Vladimir Sementsov-Ogievskiy]

Check, that bitmap is stored to the owning bs.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/dirty-bitmap.c         | 12 ++++++++++++
 block/qcow2-bitmap.c         |  5 +++++
 include/block/dirty-bitmap.h |  2 ++
 3 files changed, 19 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 1d0ea25..90ac794 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -613,3 +613,15 @@ BdrvDirtyBitmap *bdrv_load_dirty_bitmap(BlockDriverState *bs, const char *name,
     }
     return NULL;
 }
+
+bool bdrv_has_dirty_bitmap(BlockDriverState *bs, const BdrvDirtyBitmap *bitmap)
+{
+    BdrvDirtyBitmap *bm, *next;
+    QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
+        if (bm == bitmap) {
+            return true;
+        }
+    }
+
+    return false;
+}
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 0e95121..bfc1db8 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -881,6 +881,11 @@ void qcow2_bitmap_store(BlockDriverState *bs,
     uint32_t table_size;
     uint64_t *bitmap_table;
 
+    if (!bdrv_has_dirty_bitmap(bs, bitmap)) {
+        error_setg(errp, "Can't store bitmap to the other node.");
+        return;
+    }
+
     /* find/create dirty bitmap */
     bmh = find_bitmap_by_name(bs, bm_name);
     if (bmh != NULL) {
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index d482098..1af3890 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -84,4 +84,6 @@ void bdrv_finalize_persistent_dirty_bitmaps(BlockDriverState *bs);
 BdrvDirtyBitmap *bdrv_load_dirty_bitmap(BlockDriverState *bs, const char *name,
                                         Error **errp);
 
+bool bdrv_has_dirty_bitmap(BlockDriverState *bs, const BdrvDirtyBitmap *bitmap);
+
 #endif
-- 
1.8.3.1

[Qemu-devel] [PATCH 19/29] block/dirty-bitmap: add autoload field to BdrvDirtyBitmap

[Vladimir Sementsov-Ogievskiy]

Add a new field. For now it is meaningless. Will be used in future
patches.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 block/dirty-bitmap.c         | 14 ++++++++++++++
 include/block/dirty-bitmap.h |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 90ac794..bd802ac 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -45,6 +45,7 @@ struct BdrvDirtyBitmap {
     bool disabled;              /* Bitmap is read-only */
     int active_iterators;       /* How many iterators are active */
     bool persistent;            /* bitmap must be saved to owner disk image */
+    bool autoload;              /* bitmap must be autoloaded on image opening */
     QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
 
@@ -73,6 +74,7 @@ void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap)
     bitmap->name = NULL;
 
     bitmap->persistent = false;
+    bitmap->autoload = false;
 }
 
 BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
@@ -243,6 +245,8 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
     bitmap->successor = NULL;
     successor->persistent = bitmap->persistent;
     bitmap->persistent = false;
+    successor->autoload = bitmap->autoload;
+    bitmap->autoload = false;
     bdrv_release_dirty_bitmap(bs, bitmap);
 
     return successor;
@@ -579,6 +583,16 @@ void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
     bitmap->persistent = persistent;
 }
 
+void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload)
+{
+    bitmap->autoload = autoload;
+}
+
+bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap)
+{
+    return bitmap->autoload;
+}
+
 void bdrv_finalize_persistent_dirty_bitmaps(BlockDriverState *bs)
 {
     BdrvDirtyBitmap *bm, *bm_next;
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 1af3890..f5c2e3d 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -78,6 +78,8 @@ void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
 bool bdrv_load_check_dirty_bitmap(BlockDriverState *file, const char *name);
 void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
                                                 bool persistent);
+void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload);
+bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
 void bdrv_store_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
                              Error **errp);
 void bdrv_finalize_persistent_dirty_bitmaps(BlockDriverState *bs);
-- 
1.8.3.1

[Qemu-devel] [PATCH 20/29] qcow2-bitmap: add AUTO flag

[Vladimir Sementsov-Ogievskiy]

The bitmap should be auto-loaded if auto flag is set.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2-bitmap.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index bfc1db8..1c46dcb 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -44,8 +44,9 @@
 #define BME_MAX_NAME_SIZE 1023
 
 /* Bitmap directory entry flags */
-#define BME_RESERVED_FLAGS 0xfffffffe
+#define BME_RESERVED_FLAGS 0xfffffffc
 #define BME_FLAG_IN_USE 1
+#define BME_FLAG_AUTO   (1U << 1)
 
 /* bits [1, 8] U [56, 63] are reserved */
 #define BME_TABLE_ENTRY_RESERVED_MASK 0xff000000000001fe
@@ -68,6 +69,9 @@ static inline bool can_write(BlockDriverState *bs)
     return !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
 }
 
+static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs, QCow2BitmapHeader *h,
+                                    Error **errp);
+
 void qcow2_free_bitmaps(BlockDriverState *bs)
 {
     BDRVQcow2State *s = bs->opaque;
@@ -214,8 +218,31 @@ fail:
     return NULL;
 }
 
+static int load_autoload_bitmaps(BlockDriverState *bs, Error **errp)
+{
+    BDRVQcow2State *s = bs->opaque;
+    QCow2BitmapHeader *h;
+
+    for_each_bitmap_header(h, s) {
+        uint32_t fl = h->flags;
+
+        if ((fl & BME_FLAG_AUTO) && !(fl & BME_FLAG_IN_USE)) {
+            BdrvDirtyBitmap *bitmap = load_bitmap(bs, h, errp);
+            if (bitmap == NULL) {
+                return -1;
+            }
+
+            bdrv_dirty_bitmap_set_persistance(bitmap, true);
+            bdrv_dirty_bitmap_set_autoload(bitmap, true);
+        }
+    }
+
+    return 0;
+}
+
 int qcow2_read_bitmaps(BlockDriverState *bs, Error **errp)
 {
+    int ret;
     BDRVQcow2State *s = bs->opaque;
 
     if (s->bitmap_directory != NULL) {
@@ -234,7 +261,18 @@ int qcow2_read_bitmaps(BlockDriverState *bs, Error **errp)
         return -EINVAL;
     }
 
+    ret = load_autoload_bitmaps(bs, errp);
+    if (ret < 0) {
+        goto fail;
+    }
+
     return 0;
+
+fail:
+    g_free(s->bitmap_directory);
+    s->bitmap_directory = NULL;
+
+    return ret;
 }
 
 static QCow2BitmapHeader *find_bitmap_by_name(BlockDriverState *bs,
-- 
1.8.3.1

[Qemu-devel] [PATCH 21/29] qcow2-bitmap: add EXTRA_DATA_COMPATIBLE flag

[Vladimir Sementsov-Ogievskiy]

If this flag is unset and extra data is present the bitmap should be
read-only. For now just return error for this case.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2-bitmap.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 1c46dcb..a00eae4 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -47,6 +47,7 @@
 #define BME_RESERVED_FLAGS 0xfffffffc
 #define BME_FLAG_IN_USE 1
 #define BME_FLAG_AUTO   (1U << 1)
+#define BME_FLAG_EXTRA_DATA_COMPATIBLE   (1U << 1)
 
 /* bits [1, 8] U [56, 63] are reserved */
 #define BME_TABLE_ENTRY_RESERVED_MASK 0xff000000000001fe
@@ -600,6 +601,13 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
         goto fail;
     }
 
+    if (!(bmh->flags & BME_FLAG_EXTRA_DATA_COMPATIBLE) &&
+            bmh->extra_data_size != 0) {
+        error_setg(errp, "Incompatible extra data found for bitmap '%s'",
+                   name);
+        goto fail;
+    }
+
     ret = bitmap_table_load(bs, bmh, &bitmap_table);
     if (ret < 0) {
         error_setg_errno(errp, -ret,
-- 
1.8.3.1

[Qemu-devel] [PATCH 22/29] qmp: add persistent flag to block-dirty-bitmap-add

[Vladimir Sementsov-Ogievskiy]

Add optional 'persistent' flag to qmp command block-dirty-bitmap-add.
Default is false.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 blockdev.c           | 12 +++++++++++-
 qapi/block-core.json |  3 ++-
 qmp-commands.hx      |  5 ++++-
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index eafeba9..cf5ebb9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2031,6 +2031,7 @@ static void block_dirty_bitmap_add_prepare(BlkActionState *common,
     /* AIO context taken and released within qmp_block_dirty_bitmap_add */
     qmp_block_dirty_bitmap_add(action->node, action->name,
                                action->has_granularity, action->granularity,
+                               action->has_persistent, action->persistent,
                                &local_err);
 
     if (!local_err) {
@@ -2734,10 +2735,12 @@ out:
 
 void qmp_block_dirty_bitmap_add(const char *node, const char *name,
                                 bool has_granularity, uint32_t granularity,
+                                bool has_persistent, bool persistent,
                                 Error **errp)
 {
     AioContext *aio_context;
     BlockDriverState *bs;
+    BdrvDirtyBitmap *bitmap;
 
     if (!name || name[0] == '\0') {
         error_setg(errp, "Bitmap name cannot be empty");
@@ -2763,7 +2766,14 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
         granularity = bdrv_get_default_bitmap_granularity(bs);
     }
 
-    bdrv_create_dirty_bitmap(bs, granularity, name, errp);
+    if (!has_persistent) {
+        persistent = false;
+    }
+
+    bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
+    if (bitmap != NULL) {
+        bdrv_dirty_bitmap_set_persistance(bitmap, persistent);
+    }
 
  out:
     aio_context_release(aio_context);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 2bbc027..73c1aaf 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1224,7 +1224,8 @@
 # Since 2.4
 ##
 { 'struct': 'BlockDirtyBitmapAdd',
-  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32' } }
+  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
+  '*persistent': 'bool' } }
 
 ##
 # @block-dirty-bitmap-add
diff --git a/qmp-commands.hx b/qmp-commands.hx
index c8d360a..f7d75bb 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1435,7 +1435,7 @@ EQMP
 
     {
         .name       = "block-dirty-bitmap-add",
-        .args_type  = "node:B,name:s,granularity:i?",
+        .args_type  = "node:B,name:s,granularity:i?,persistent:b?",
         .mhandler.cmd_new = qmp_marshal_block_dirty_bitmap_add,
     },
 
@@ -1452,6 +1452,9 @@ Arguments:
 - "node": device/node on which to create dirty bitmap (json-string)
 - "name": name of the new dirty bitmap (json-string)
 - "granularity": granularity to track writes with (int, optional)
+- "persistent": bitmap will be saved to corresponding block device
+                on quit. Block driver should maintain persistent bitmaps
+                (json-bool, optional, default false)
 
 Example:
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 23/29] qmp: add autoload parameter to block-dirty-bitmap-add

[Vladimir Sementsov-Ogievskiy]

Optional. Default is false.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 blockdev.c           | 22 ++++++++++++++++++++--
 qapi/block-core.json |  2 +-
 qmp-commands.hx      |  4 +++-
 3 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index cf5ebb9..f2416e9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2032,6 +2032,7 @@ static void block_dirty_bitmap_add_prepare(BlkActionState *common,
     qmp_block_dirty_bitmap_add(action->node, action->name,
                                action->has_granularity, action->granularity,
                                action->has_persistent, action->persistent,
+                               action->has_autoload, action->autoload,
                                &local_err);
 
     if (!local_err) {
@@ -2736,6 +2737,7 @@ out:
 void qmp_block_dirty_bitmap_add(const char *node, const char *name,
                                 bool has_granularity, uint32_t granularity,
                                 bool has_persistent, bool persistent,
+                                bool has_autoload, bool autoload,
                                 Error **errp)
 {
     AioContext *aio_context;
@@ -2769,10 +2771,26 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
     if (!has_persistent) {
         persistent = false;
     }
+    if (!has_autoload) {
+        autoload = false;
+    }
+
+    if (autoload && !persistent) {
+        error_setg(errp, "Autoload flag must be used only for persistent"
+                         "bitmaps");
+        goto out;
+    }
 
     bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
-    if (bitmap != NULL) {
-        bdrv_dirty_bitmap_set_persistance(bitmap, persistent);
+    if (bitmap == NULL) {
+        goto out;
+    }
+
+    if (persistent) {
+        bdrv_dirty_bitmap_set_persistance(bitmap, true);
+        if (autoload) {
+            bdrv_dirty_bitmap_set_autoload(bitmap, true);
+        }
     }
 
  out:
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 73c1aaf..c1d4bdf 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1225,7 +1225,7 @@
 ##
 { 'struct': 'BlockDirtyBitmapAdd',
   'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
-  '*persistent': 'bool' } }
+  '*persistent': 'bool', '*autoload': 'bool' } }
 
 ##
 # @block-dirty-bitmap-add
diff --git a/qmp-commands.hx b/qmp-commands.hx
index f7d75bb..90205c8 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1435,7 +1435,7 @@ EQMP
 
     {
         .name       = "block-dirty-bitmap-add",
-        .args_type  = "node:B,name:s,granularity:i?,persistent:b?",
+        .args_type  = "node:B,name:s,granularity:i?,persistent:b?,autoload:b?",
         .mhandler.cmd_new = qmp_marshal_block_dirty_bitmap_add,
     },
 
@@ -1455,6 +1455,8 @@ Arguments:
 - "persistent": bitmap will be saved to corresponding block device
                 on quit. Block driver should maintain persistent bitmaps
                 (json-bool, optional, default false)
+- "autoload": only for persistent bitmaps. Bitmap will be autoloaded on it's
+              storage image open. (json-bool, optional, default false)
 
 Example:
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 24/29] qcow2-bitmap: maintian BlockDirtyBitmap.autoload

[Vladimir Sementsov-Ogievskiy]

Sync qcow2 dirty bitmap autload flag and BlockDirtyBitmap autoload flag

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 block/qcow2-bitmap.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index a00eae4..49066bd 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -960,15 +960,22 @@ void qcow2_bitmap_store(BlockDriverState *bs,
     }
 
     if (bmh == NULL) {
+        uint32_t flags =
+                bdrv_dirty_bitmap_get_autoload(bitmap) ? BME_FLAG_AUTO : 0;
         ret = directory_push(bs, bm_name, granularity, table_offset,
-                             table_size, 0);
+                             table_size, flags);
         if (ret < 0) {
             error_setg_errno(errp, ret, "Can't create dirty bitmap in qcow2.");
             goto fail;
         }
     } else {
+        uint32_t flags = bmh->flags;
+        flags &= ~(BME_FLAG_IN_USE | BME_FLAG_AUTO);
+        if (bdrv_dirty_bitmap_get_autoload(bitmap)) {
+            flags |= BME_FLAG_AUTO;
+        }
         ret = directory_set(bs, bmh, granularity, table_offset, table_size,
-                            bmh->flags & ~BME_FLAG_IN_USE);
+                            flags);
         if (ret < 0) {
             error_setg_errno(errp, ret, "Can't update dirty bitmap in qcow2.");
             goto fail;
-- 
1.8.3.1

[Qemu-devel] [PATCH 25/29] qapi: add md5 checksum of last dirty bitmap level to query-block

[Vladimir Sementsov-Ogievskiy]

Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/dirty-bitmap.c   | 1 +
 include/qemu/hbitmap.h | 8 ++++++++
 qapi/block-core.json   | 5 ++++-
 util/hbitmap.c         | 8 ++++++++
 4 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index bd802ac..6e2dd0d 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -359,6 +359,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
         info->has_name = !!bm->name;
         info->name = g_strdup(bm->name);
         info->status = bdrv_dirty_bitmap_status(bm);
+        info->md5 = hbitmap_md5(bm->bitmap);
         entry->value = info;
         *plist = entry;
         plist = &entry->next;
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index a7b9ccc..39b7f87 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -240,6 +240,14 @@ void hbitmap_deserialize_ones(HBitmap *hb, uint64_t start, uint64_t count,
 void hbitmap_deserialize_finish(HBitmap *hb);
 
 /**
+ * hbitmap_md5:
+ * @bitmap: HBitmap to operate on.
+ *
+ * Returns md5 checksum of the last level.
+ */
+char *hbitmap_md5(const HBitmap *bitmap);
+
+/**
  * hbitmap_free:
  * @hb: HBitmap to operate on.
  *
diff --git a/qapi/block-core.json b/qapi/block-core.json
index c1d4bdf..457472d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -419,11 +419,14 @@
 #
 # @status: current status of the dirty bitmap (since 2.4)
 #
+# @md5: md5 checksum (as a hexadecimal string) of the last bitmap level
+#       (since 2.8)
+#
 # Since: 1.3
 ##
 { 'struct': 'BlockDirtyInfo',
   'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
-           'status': 'DirtyBitmapStatus'} }
+           'status': 'DirtyBitmapStatus', 'md5': 'str'} }
 
 ##
 # @BlockInfo:
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 3a3257c..c2b8f0e 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -687,3 +687,11 @@ void hbitmap_free_meta(HBitmap *hb)
     hbitmap_free(hb->meta);
     hb->meta = NULL;
 }
+
+char *hbitmap_md5(const HBitmap *bitmap)
+{
+    uint64_t size =
+        MAX((bitmap->size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
+    const guchar *data = (const guchar *)bitmap->levels[HBITMAP_LEVELS - 1];
+    return g_compute_checksum_for_data(G_CHECKSUM_MD5, data, size);
+}
-- 
1.8.3.1

[Qemu-devel] [PATCH 26/29] iotests: test qcow2 persistent dirty bitmap

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/160        | 87 +++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/160.out    |  5 +++
 tests/qemu-iotests/group      |  1 +
 tests/qemu-iotests/iotests.py |  6 +++
 4 files changed, 99 insertions(+)
 create mode 100755 tests/qemu-iotests/160
 create mode 100644 tests/qemu-iotests/160.out

diff --git a/tests/qemu-iotests/160 b/tests/qemu-iotests/160
new file mode 100755
index 0000000..a69799c
--- /dev/null
+++ b/tests/qemu-iotests/160
@@ -0,0 +1,87 @@
+#!/usr/bin/env python
+#
+# Tests for persistent dirty bitmaps.
+#
+# Copyright: Vladimir Sementsov-Ogievskiy 2015-2016
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import os
+import iotests
+from iotests import qemu_img
+
+disk = os.path.join(iotests.test_dir, 'disk')
+disk_size = 0x40000000 # 1G
+
+# regions for qemu_io: (start, count) in bytes
+regions1 = ((0,        0x100000),
+            (0x200000, 0x100000))
+
+regions2 = ((0x10000000, 0x20000),
+            (0x39990000, 0x10000))
+
+class TestPersistentDirtyBitmap(iotests.QMPTestCase):
+
+    def setUp(self):
+        qemu_img('create', '-f', iotests.imgfmt, disk, str(disk_size))
+
+    def tearDown(self):
+        os.remove(disk)
+
+    def mkVm(self):
+        return iotests.VM().add_drive(disk)
+
+    def getMd5(self):
+        result = self.vm.qmp('query-block');
+        return result['return'][0]['dirty-bitmaps'][0]['md5']
+
+    def checkBitmap(self, md5):
+        result = self.vm.qmp('query-block');
+        self.assert_qmp(result, 'return[0]/dirty-bitmaps[0]/md5', md5);
+
+    def writeRegions(self, regions):
+        for r in regions:
+          self.vm.hmp_qemu_io('drive0',
+                              'write %d %d' % r)
+
+    def qmpAddBitmap(self):
+        self.vm.qmp('block-dirty-bitmap-add', node='drive0',
+                    name='bitmap0', persistent=True, autoload=True)
+
+    def test_persistent(self):
+        self.vm = self.mkVm()
+        self.vm.launch()
+        self.qmpAddBitmap()
+
+        self.writeRegions(regions1)
+        md5 = self.getMd5()
+
+        self.vm.shutdown()
+        self.vm = self.mkVm()
+        self.vm.launch()
+
+        self.checkBitmap(md5)
+        self.writeRegions(regions2)
+        md5 = self.getMd5()
+
+        self.vm.shutdown()
+        self.vm.launch()
+
+        self.checkBitmap(md5)
+
+        self.vm.shutdown()
+
+if __name__ == '__main__':
+    iotests.main(supported_fmts=['qcow2'])
diff --git a/tests/qemu-iotests/160.out b/tests/qemu-iotests/160.out
new file mode 100644
index 0000000..ae1213e
--- /dev/null
+++ b/tests/qemu-iotests/160.out
@@ -0,0 +1,5 @@
+.
+----------------------------------------------------------------------
+Ran 1 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 3a3973e..b5cd18b 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -157,3 +157,4 @@
 155 rw auto
 156 rw auto quick
 157 auto
+160 rw auto quick
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index dbe0ee5..95da7a0 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -159,6 +159,12 @@ class VM(qtest.QEMUQtestMachine):
         self._num_drives += 1
         return self
 
+    def add_dirty_bitmap(self, name, node, create=False):
+        '''Add dirty bitmap parameter to VM cmd'''
+        self._args.append('-dirty-bitmap')
+        self._args.append('name=%s,node=%s,create=%s' % (name, node, 'on' if create else 'off'))
+        return self
+
     def pause_drive(self, drive, event=None):
         '''Pause drive r/w operations'''
         if not event:
-- 
1.8.3.1

[Qemu-devel] [PATCH 27/29] qcow2-bitmap: delete in_use bitmaps on image load

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 block/qcow2-bitmap.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 95 insertions(+)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 49066bd..e94019c 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -28,6 +28,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qemu/cutils.h"
+#include "qemu/log.h"
 
 #include "block/block_int.h"
 #include "block/qcow2.h"
@@ -132,6 +133,17 @@ static inline QCow2BitmapHeader *next_dir_entry(QCow2BitmapHeader *entry)
     return (QCow2BitmapHeader *)((uint8_t *)entry + dir_entry_size(entry));
 }
 
+static inline void bitmap_directory_to_cpu(uint8_t *dir, size_t size)
+{
+    QCow2BitmapHeader *h;
+
+    /* loop is safe because next entry offset is calculated after conversion to
+     * cpu format */
+    for_each_bitmap_header_in_dir(h, dir, size) {
+        bitmap_header_to_cpu(h);
+    }
+}
+
 static inline void bitmap_directory_to_be(uint8_t *dir, size_t size)
 {
     uint8_t *end = dir + size;
@@ -241,6 +253,7 @@ static int load_autoload_bitmaps(BlockDriverState *bs, Error **errp)
     return 0;
 }
 
+static int handle_in_use_bitmaps(BlockDriverState *bs);
 int qcow2_read_bitmaps(BlockDriverState *bs, Error **errp)
 {
     int ret;
@@ -262,6 +275,19 @@ int qcow2_read_bitmaps(BlockDriverState *bs, Error **errp)
         return -EINVAL;
     }
 
+    if (can_write(bs)) {
+        ret = handle_in_use_bitmaps(bs);
+        if (ret < 0) {
+            error_setg_errno(errp, -ret, "Can't delete in_use bitmaps.");
+            goto fail;
+        }
+
+        if (s->nb_bitmaps == 0) {
+            /* No bitmaps - nothing to do */
+            return 0;
+        }
+    }
+
     ret = load_autoload_bitmaps(bs, errp);
     if (ret < 0) {
         goto fail;
@@ -685,6 +711,75 @@ int qcow2_delete_bitmaps(BlockDriverState *bs)
     return directory_update(bs, NULL, 0, 0);
 }
 
+static int handle_in_use_bitmaps(BlockDriverState *bs)
+{
+    int ret;
+    BDRVQcow2State *s = bs->opaque;
+    uint64_t new_size = 0;
+    uint32_t new_nb_bitmaps = 0;
+    uint8_t *new_dir = NULL, *p;
+    QCow2BitmapHeader *h;
+
+    /* free in_use bitmaps clusters */
+    for_each_bitmap_header(h, s) {
+        if (h->flags & BME_FLAG_IN_USE) {
+            free_bitmap_clusters(bs, h);
+            h->bitmap_table_offset = 0;
+            h->bitmap_table_size = 0;
+        } else {
+            new_size += dir_entry_size(h);
+            new_nb_bitmaps++;
+        }
+    }
+
+    /* no in_use bitmaps */
+    if (new_nb_bitmaps == s->nb_bitmaps) {
+        return 0;
+    }
+
+    /* just update bitmap directory with freed out in_use bitmaps */
+    if (!(bdrv_get_flags(bs) & BDRV_O_CHECK)) {
+        bitmap_directory_to_be(s->bitmap_directory, s->bitmap_directory_size);
+        ret = bdrv_pwrite(bs->file,
+                          s->bitmap_directory_offset,
+                          s->bitmap_directory,
+                          s->bitmap_directory_size);
+        bitmap_directory_to_cpu(s->bitmap_directory, s->bitmap_directory_size);
+
+        return ret;
+    }
+
+    if (new_nb_bitmaps == 0) {
+        goto update;
+    }
+
+    p = new_dir = g_try_malloc(new_size);
+    if (new_dir == NULL) {
+        return -ENOMEM;
+    }
+
+    for_each_bitmap_header(h, s) {
+        if (h->flags & BME_FLAG_IN_USE) {
+            char *bitmap_name = g_strndup((char *)(h + 1), h->name_size);
+            qemu_log("Delete corrupted (in_use) bitmap '%s' from device '%s'\n",
+                     bitmap_name, bdrv_get_device_or_node_name(bs));
+            g_free(bitmap_name);
+        } else {
+            memcpy(p, h, dir_entry_size(h));
+            p += dir_entry_size(h);
+        }
+    }
+
+update:
+    ret = directory_update(bs, new_dir, new_size, new_nb_bitmaps);
+    if (ret < 0) {
+        g_free(new_dir);
+        return ret;
+    }
+
+    return 0;
+}
+
 /* store_bitmap_data()
  * Store bitmap to image, filling bitamp table accordingly.
  */
-- 
1.8.3.1

[Qemu-devel] [PATCH 28/29] qcow2-bitmap: do not try reloading bitmaps

[Vladimir Sementsov-Ogievskiy]

Sometimes image is being reopened without removing bdrv state and
therefore bitmaps. So here we allow not loading qcow2 bitmap if it
already exists. It may lead to problem, if existing of such bitmap is a
mistake - this mistake would be skipped.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 block/qcow2-bitmap.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index e94019c..def2005 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -642,15 +642,23 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
     }
 
     granularity = 1U << bmh->granularity_bits;
-    bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
+
+    bitmap = bdrv_find_dirty_bitmap(bs, name);
     if (bitmap == NULL) {
-        goto fail;
-    }
+        bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
+        if (bitmap == NULL) {
+            goto fail;
+        }
 
-    ret = load_bitmap_data(bs, bitmap_table, bmh->bitmap_table_size,
-                           bitmap);
-    if (ret < 0) {
-        error_setg_errno(errp, -ret, "Could not read bitmap from image");
+        ret = load_bitmap_data(bs, bitmap_table, bmh->bitmap_table_size,
+                               bitmap);
+        if (ret < 0) {
+            error_setg_errno(errp, -ret, "Could not read bitmap from image");
+            goto fail;
+        }
+    } else if (granularity != bdrv_dirty_bitmap_granularity(bitmap)) {
+        error_setg(errp, "Bitmap '%s' already exists with other granularity",
+                   name);
         goto fail;
     }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 29/29] qcow2-dirty-bitmap: refcounts

[Vladimir Sementsov-Ogievskiy]

Calculate refcounts for dirty bitmaps.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 block/qcow2-bitmap.c   | 58 ++++++++++++++++++++++++++++++++++++++++++++++++--
 block/qcow2-refcount.c | 14 +++++++-----
 block/qcow2.h          |  5 +++++
 3 files changed, 70 insertions(+), 7 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index def2005..b24fa27 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -253,6 +253,62 @@ static int load_autoload_bitmaps(BlockDriverState *bs, Error **errp)
     return 0;
 }
 
+static int bitmap_table_load(BlockDriverState *bs, QCow2BitmapHeader *bmh,
+                             uint64_t **table);
+int qcow2_dirty_bitmaps_refcounts(BlockDriverState *bs,
+                                  BdrvCheckResult *res,
+                                  void **refcount_table,
+                                  int64_t *refcount_table_size)
+{
+    BDRVQcow2State *s = bs->opaque;
+    QCow2BitmapHeader *h,
+                      *begin = (QCow2BitmapHeader *)s->bitmap_directory,
+                      *end = (QCow2BitmapHeader *)
+                          (s->bitmap_directory + s->bitmap_directory_size);
+
+    int i;
+    int ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
+                        s->bitmap_directory_offset,
+                        s->bitmap_directory_size);
+    if (ret < 0) {
+        return ret;
+    }
+
+    for (h = begin; h < end; h = next_dir_entry(h)) {
+        uint64_t *bitmap_table = NULL;
+
+        ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
+                            h->bitmap_table_offset,
+                            h->bitmap_table_size);
+        if (ret < 0) {
+            return ret;
+        }
+
+        ret = bitmap_table_load(bs, h, &bitmap_table);
+        if (ret < 0) {
+            return ret;
+        }
+
+        for (i = 0; i < h->bitmap_table_size; ++i) {
+            uint64_t off = be64_to_cpu(bitmap_table[i]);
+            if (off <= 1) {
+                continue;
+            }
+
+            ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
+                                off, s->cluster_size);
+            if (ret < 0) {
+                g_free(bitmap_table);
+                return ret;
+            }
+        }
+
+        g_free(bitmap_table);
+    }
+
+    return 0;
+}
+
 static int handle_in_use_bitmaps(BlockDriverState *bs);
 int qcow2_read_bitmaps(BlockDriverState *bs, Error **errp)
 {
@@ -351,8 +407,6 @@ static void do_free_bitmap_clusters(BlockDriverState *bs,
                         QCOW2_DISCARD_ALWAYS);
 }
 
-static int bitmap_table_load(BlockDriverState *bs, QCow2BitmapHeader *bmh,
-                             uint64_t **table);
 static int free_bitmap_clusters(BlockDriverState *bs, QCow2BitmapHeader *bmh)
 {
     int ret;
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index cbfb3fe..05bcc57 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1309,11 +1309,9 @@ static int realloc_refcount_array(BDRVQcow2State *s, void **array,
  *
  * Modifies the number of errors in res.
  */
-static int inc_refcounts(BlockDriverState *bs,
-                         BdrvCheckResult *res,
-                         void **refcount_table,
-                         int64_t *refcount_table_size,
-                         int64_t offset, int64_t size)
+int inc_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
+                  void **refcount_table, int64_t *refcount_table_size,
+                  int64_t offset, int64_t size)
 {
     BDRVQcow2State *s = bs->opaque;
     uint64_t start, last, cluster_offset, k, refcount;
@@ -1843,6 +1841,12 @@ static int calculate_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
         return ret;
     }
 
+    /* dirty bitmaps */
+    ret = qcow2_dirty_bitmaps_refcounts(bs, res, refcount_table, nb_clusters);
+    if (ret < 0) {
+        return ret;
+    }
+
     return check_refblocks(bs, res, fix, rebuild, refcount_table, nb_clusters);
 }
 
diff --git a/block/qcow2.h b/block/qcow2.h
index 74d395b..3359c14 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -564,6 +564,9 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset,
                                  int64_t size);
 int qcow2_pre_write_overlap_check(BlockDriverState *bs, int ign, int64_t offset,
                                   int64_t size);
+int inc_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
+                  void **refcount_table, int64_t *refcount_table_size,
+                  int64_t offset, int64_t size);
 
 int qcow2_change_refcount_order(BlockDriverState *bs, int refcount_order,
                                 BlockDriverAmendStatusCB *status_cb,
@@ -623,6 +626,8 @@ BdrvDirtyBitmap *qcow2_bitmap_load(BlockDriverState *bs, const char *name,
 void qcow2_bitmap_store(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
                         Error **errp);
 int qcow2_delete_bitmaps(BlockDriverState *bs);
+int qcow2_dirty_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
+    void **refcount_table, int64_t *refcount_table_size);
 
 /* qcow2-cache.c functions */
 Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables);
-- 
1.8.3.1