[PATCH 0/5] parallels: load bitmap extension

[PATCH 0/5] parallels: load bitmap extension

[Vladimir Sementsov-Ogievskiy]

Hi all!

Suddenly we need to load bitmaps from parallels image in our product.
So here is a feature.

Vladimir Sementsov-Ogievskiy (5):
  qcow2-bitmap: make bytes_covered_by_bitmap_cluster() public
  parallels.txt: fix bitmap L1 table description
  parallels: support bitmap extension for read-only mode
  iotests.py: add unarchive_sample_image() helper
  iotests: add parallels-read-bitmap test

 docs/interop/parallels.txt                    |  15 +-
 block/parallels.h                             |   6 +-
 include/block/dirty-bitmap.h                  |   2 +
 block/dirty-bitmap.c                          |  13 +
 block/parallels-ext.c                         | 286 ++++++++++++++++++
 block/parallels.c                             |  18 ++
 block/qcow2-bitmap.c                          |  16 +-
 block/meson.build                             |   3 +-
 tests/qemu-iotests/iotests.py                 |  10 +
 .../sample_images/parallels-with-bitmap.bz2   | Bin 0 -> 203 bytes
 .../sample_images/parallels-with-bitmap.sh    |  33 ++
 .../qemu-iotests/tests/parallels-read-bitmap  |  57 ++++
 .../tests/parallels-read-bitmap.out           |   6 +
 13 files changed, 443 insertions(+), 22 deletions(-)
 create mode 100644 block/parallels-ext.c
 create mode 100644 tests/qemu-iotests/sample_images/parallels-with-bitmap.bz2
 create mode 100755 tests/qemu-iotests/sample_images/parallels-with-bitmap.sh
 create mode 100755 tests/qemu-iotests/tests/parallels-read-bitmap
 create mode 100644 tests/qemu-iotests/tests/parallels-read-bitmap.out

-- 
2.29.2

[PATCH 1/5] qcow2-bitmap: make bytes_covered_by_bitmap_cluster() public

[Vladimir Sementsov-Ogievskiy]

Rename bytes_covered_by_bitmap_cluster() to
bdrv_dirty_bitmap_serialization_coverage() and make it public. It is
needed as we are going to make load_bitmap_data() public in the next
commit.

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

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 36e8da4fc2..f581cf9fd7 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -57,6 +57,8 @@ void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);
 uint64_t bdrv_dirty_bitmap_serialization_size(const BdrvDirtyBitmap *bitmap,
                                               uint64_t offset, uint64_t bytes);
 uint64_t bdrv_dirty_bitmap_serialization_align(const BdrvDirtyBitmap *bitmap);
+uint64_t bdrv_dirty_bitmap_serialization_coverage(int serialized_chunk_size,
+        const BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_bitmap_serialize_part(const BdrvDirtyBitmap *bitmap,
                                       uint8_t *buf, uint64_t offset,
                                       uint64_t bytes);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 9b9cd71238..a0eaa28785 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -726,6 +726,19 @@ uint64_t bdrv_dirty_bitmap_serialization_align(const BdrvDirtyBitmap *bitmap)
     return hbitmap_serialization_align(bitmap->bitmap);
 }
 
+/* Return the disk size covered by a chunk of serialized bitmap data. */
+uint64_t bdrv_dirty_bitmap_serialization_coverage(int serialized_chunk_size,
+                                                  const BdrvDirtyBitmap *bitmap)
+{
+    uint64_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
+    uint64_t limit = granularity * (serialized_chunk_size << 3);
+
+    assert(QEMU_IS_ALIGNED(limit,
+                           bdrv_dirty_bitmap_serialization_align(bitmap)));
+    return limit;
+}
+
+
 void bdrv_dirty_bitmap_serialize_part(const BdrvDirtyBitmap *bitmap,
                                       uint8_t *buf, uint64_t offset,
                                       uint64_t bytes)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 5eef82fa55..42d81c44cd 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -278,18 +278,6 @@ static int free_bitmap_clusters(BlockDriverState *bs, Qcow2BitmapTable *tb)
     return 0;
 }
 
-/* Return the disk size covered by a single qcow2 cluster of bitmap data. */
-static uint64_t bytes_covered_by_bitmap_cluster(const BDRVQcow2State *s,
-                                                const BdrvDirtyBitmap *bitmap)
-{
-    uint64_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
-    uint64_t limit = granularity * (s->cluster_size << 3);
-
-    assert(QEMU_IS_ALIGNED(limit,
-                           bdrv_dirty_bitmap_serialization_align(bitmap)));
-    return limit;
-}
-
 /* load_bitmap_data
  * @bitmap_table entries must satisfy specification constraints.
  * @bitmap must be cleared */
@@ -312,7 +300,7 @@ static int load_bitmap_data(BlockDriverState *bs,
     }
 
     buf = g_malloc(s->cluster_size);
-    limit = bytes_covered_by_bitmap_cluster(s, bitmap);
+    limit = bdrv_dirty_bitmap_serialization_coverage(s->cluster_size, bitmap);
     for (i = 0, offset = 0; i < tab_size; ++i, offset += limit) {
         uint64_t count = MIN(bm_size - offset, limit);
         uint64_t entry = bitmap_table[i];
@@ -1303,7 +1291,7 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
     }
 
     buf = g_malloc(s->cluster_size);
-    limit = bytes_covered_by_bitmap_cluster(s, bitmap);
+    limit = bdrv_dirty_bitmap_serialization_coverage(s->cluster_size, bitmap);
     assert(DIV_ROUND_UP(bm_size, limit) == tb_size);
 
     offset = 0;
-- 
2.29.2

Re: [PATCH 1/5] qcow2-bitmap: make bytes_covered_by_bitmap_cluster() public

[Eric Blake]

On 2/16/21 10:45 AM, Vladimir Sementsov-Ogievskiy wrote:
> Rename bytes_covered_by_bitmap_cluster() to
> bdrv_dirty_bitmap_serialization_coverage() and make it public. It is
> needed as we are going to make load_bitmap_data() public in the next
> commit.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/dirty-bitmap.h |  2 ++
>  block/dirty-bitmap.c         | 13 +++++++++++++
>  block/qcow2-bitmap.c         | 16 ++--------------
>  3 files changed, 17 insertions(+), 14 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 1/5] qcow2-bitmap: make bytes_covered_by_bitmap_cluster() public

[Vladimir Sementsov-Ogievskiy]

16.02.2021 19:45, Vladimir Sementsov-Ogievskiy wrote:
> Rename bytes_covered_by_bitmap_cluster() to
> bdrv_dirty_bitmap_serialization_coverage() and make it public. It is
> needed as we are going to make load_bitmap_data() public in the next
> commit.

Not we are not. So the last sentence should be changed to

   It is needed as we are going to share it with bitmap loading in parallels format.

I started from sharing load_bitmap_data() function, but than dropped this idea.

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   include/block/dirty-bitmap.h |  2 ++
>   block/dirty-bitmap.c         | 13 +++++++++++++
>   block/qcow2-bitmap.c         | 16 ++--------------
>   3 files changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 36e8da4fc2..f581cf9fd7 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -57,6 +57,8 @@ void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);
>   uint64_t bdrv_dirty_bitmap_serialization_size(const BdrvDirtyBitmap *bitmap,
>                                                 uint64_t offset, uint64_t bytes);
>   uint64_t bdrv_dirty_bitmap_serialization_align(const BdrvDirtyBitmap *bitmap);
> +uint64_t bdrv_dirty_bitmap_serialization_coverage(int serialized_chunk_size,
> +        const BdrvDirtyBitmap *bitmap);
>   void bdrv_dirty_bitmap_serialize_part(const BdrvDirtyBitmap *bitmap,
>                                         uint8_t *buf, uint64_t offset,
>                                         uint64_t bytes);
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 9b9cd71238..a0eaa28785 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -726,6 +726,19 @@ uint64_t bdrv_dirty_bitmap_serialization_align(const BdrvDirtyBitmap *bitmap)
>       return hbitmap_serialization_align(bitmap->bitmap);
>   }
>   
> +/* Return the disk size covered by a chunk of serialized bitmap data. */
> +uint64_t bdrv_dirty_bitmap_serialization_coverage(int serialized_chunk_size,
> +                                                  const BdrvDirtyBitmap *bitmap)
> +{
> +    uint64_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
> +    uint64_t limit = granularity * (serialized_chunk_size << 3);
> +
> +    assert(QEMU_IS_ALIGNED(limit,
> +                           bdrv_dirty_bitmap_serialization_align(bitmap)));
> +    return limit;
> +}
> +
> +
>   void bdrv_dirty_bitmap_serialize_part(const BdrvDirtyBitmap *bitmap,
>                                         uint8_t *buf, uint64_t offset,
>                                         uint64_t bytes)
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 5eef82fa55..42d81c44cd 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -278,18 +278,6 @@ static int free_bitmap_clusters(BlockDriverState *bs, Qcow2BitmapTable *tb)
>       return 0;
>   }
>   
> -/* Return the disk size covered by a single qcow2 cluster of bitmap data. */
> -static uint64_t bytes_covered_by_bitmap_cluster(const BDRVQcow2State *s,
> -                                                const BdrvDirtyBitmap *bitmap)
> -{
> -    uint64_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
> -    uint64_t limit = granularity * (s->cluster_size << 3);
> -
> -    assert(QEMU_IS_ALIGNED(limit,
> -                           bdrv_dirty_bitmap_serialization_align(bitmap)));
> -    return limit;
> -}
> -
>   /* load_bitmap_data
>    * @bitmap_table entries must satisfy specification constraints.
>    * @bitmap must be cleared */
> @@ -312,7 +300,7 @@ static int load_bitmap_data(BlockDriverState *bs,
>       }
>   
>       buf = g_malloc(s->cluster_size);
> -    limit = bytes_covered_by_bitmap_cluster(s, bitmap);
> +    limit = bdrv_dirty_bitmap_serialization_coverage(s->cluster_size, bitmap);
>       for (i = 0, offset = 0; i < tab_size; ++i, offset += limit) {
>           uint64_t count = MIN(bm_size - offset, limit);
>           uint64_t entry = bitmap_table[i];
> @@ -1303,7 +1291,7 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
>       }
>   
>       buf = g_malloc(s->cluster_size);
> -    limit = bytes_covered_by_bitmap_cluster(s, bitmap);
> +    limit = bdrv_dirty_bitmap_serialization_coverage(s->cluster_size, bitmap);
>       assert(DIV_ROUND_UP(bm_size, limit) == tb_size);
>   
>       offset = 0;
> 


-- 
Best regards,
Vladimir

Re: [PATCH 1/5] qcow2-bitmap: make bytes_covered_by_bitmap_cluster() public

[Denis V. Lunev]

On 2/16/21 7:45 PM, Vladimir Sementsov-Ogievskiy wrote:
> Rename bytes_covered_by_bitmap_cluster() to
> bdrv_dirty_bitmap_serialization_coverage() and make it public. It is
> needed as we are going to make load_bitmap_data() public in the next
> commit.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/dirty-bitmap.h |  2 ++
>  block/dirty-bitmap.c         | 13 +++++++++++++
>  block/qcow2-bitmap.c         | 16 ++--------------
>  3 files changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 36e8da4fc2..f581cf9fd7 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -57,6 +57,8 @@ void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);
>  uint64_t bdrv_dirty_bitmap_serialization_size(const BdrvDirtyBitmap *bitmap,
>                                                uint64_t offset, uint64_t bytes);
>  uint64_t bdrv_dirty_bitmap_serialization_align(const BdrvDirtyBitmap *bitmap);
> +uint64_t bdrv_dirty_bitmap_serialization_coverage(int serialized_chunk_size,
> +        const BdrvDirtyBitmap *bitmap);
>  void bdrv_dirty_bitmap_serialize_part(const BdrvDirtyBitmap *bitmap,
>                                        uint8_t *buf, uint64_t offset,
>                                        uint64_t bytes);
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 9b9cd71238..a0eaa28785 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -726,6 +726,19 @@ uint64_t bdrv_dirty_bitmap_serialization_align(const BdrvDirtyBitmap *bitmap)
>      return hbitmap_serialization_align(bitmap->bitmap);
>  }
>  
> +/* Return the disk size covered by a chunk of serialized bitmap data. */
> +uint64_t bdrv_dirty_bitmap_serialization_coverage(int serialized_chunk_size,
> +                                                  const BdrvDirtyBitmap *bitmap)
> +{
> +    uint64_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
> +    uint64_t limit = granularity * (serialized_chunk_size << 3);
> +
> +    assert(QEMU_IS_ALIGNED(limit,
> +                           bdrv_dirty_bitmap_serialization_align(bitmap)));
> +    return limit;
> +}
> +
> +
>  void bdrv_dirty_bitmap_serialize_part(const BdrvDirtyBitmap *bitmap,
>                                        uint8_t *buf, uint64_t offset,
>                                        uint64_t bytes)
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 5eef82fa55..42d81c44cd 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -278,18 +278,6 @@ static int free_bitmap_clusters(BlockDriverState *bs, Qcow2BitmapTable *tb)
>      return 0;
>  }
>  
> -/* Return the disk size covered by a single qcow2 cluster of bitmap data. */
> -static uint64_t bytes_covered_by_bitmap_cluster(const BDRVQcow2State *s,
> -                                                const BdrvDirtyBitmap *bitmap)
> -{
> -    uint64_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
> -    uint64_t limit = granularity * (s->cluster_size << 3);
> -
> -    assert(QEMU_IS_ALIGNED(limit,
> -                           bdrv_dirty_bitmap_serialization_align(bitmap)));
> -    return limit;
> -}
> -
>  /* load_bitmap_data
>   * @bitmap_table entries must satisfy specification constraints.
>   * @bitmap must be cleared */
> @@ -312,7 +300,7 @@ static int load_bitmap_data(BlockDriverState *bs,
>      }
>  
>      buf = g_malloc(s->cluster_size);
> -    limit = bytes_covered_by_bitmap_cluster(s, bitmap);
> +    limit = bdrv_dirty_bitmap_serialization_coverage(s->cluster_size, bitmap);
>      for (i = 0, offset = 0; i < tab_size; ++i, offset += limit) {
>          uint64_t count = MIN(bm_size - offset, limit);
>          uint64_t entry = bitmap_table[i];
> @@ -1303,7 +1291,7 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
>      }
>  
>      buf = g_malloc(s->cluster_size);
> -    limit = bytes_covered_by_bitmap_cluster(s, bitmap);
> +    limit = bdrv_dirty_bitmap_serialization_coverage(s->cluster_size, bitmap);
>      assert(DIV_ROUND_UP(bm_size, limit) == tb_size);
>  
>      offset = 0;
Reviewed-by: Denis V. Lunev <den@openvz.org>

[PATCH 2/5] parallels.txt: fix bitmap L1 table description

[Vladimir Sementsov-Ogievskiy]

Actually L1 table entry offset is in 512 bytes sectors. Fix the spec.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 docs/interop/parallels.txt | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/docs/interop/parallels.txt b/docs/interop/parallels.txt
index f15bf35bd1..ebbdd1b25b 100644
--- a/docs/interop/parallels.txt
+++ b/docs/interop/parallels.txt
@@ -209,15 +209,14 @@ of its data area are:
               The number of entries in the L1 table of the bitmap.
 
   variable:   l1_table (8 * l1_size bytes)
-              L1 offset table (in bytes)
 
 A dirty bitmap is stored using a one-level structure for the mapping to host
-clusters - an L1 table.
+clusters - an L1 table. Each L1 table entry is a 64bit integer described
+below:
 
-Given an offset in bytes into the bitmap data, the offset in bytes into the
-image file can be obtained as follows:
+Given an offset in bytes into the bitmap data, corresponding L1 entry is
 
-    offset = l1_table[offset / cluster_size] + (offset % cluster_size)
+    l1_table[offset / cluster_size]
 
 If an L1 table entry is 0, the corresponding cluster of the bitmap is assumed
 to be zero.
@@ -225,4 +224,8 @@ to be zero.
 If an L1 table entry is 1, the corresponding cluster of the bitmap is assumed
 to have all bits set.
 
-If an L1 table entry is not 0 or 1, it allocates a cluster from the data area.
+If an L1 table entry is not 0 or 1, it contains corresponding cluster offset
+(in 512b sectors). Given an offset in bytes into the bitmap data the offset in
+bytes into the image file can be obtained as follows:
+
+    offset = l1_table[offset / cluster_size] * 512 + (offset % cluster_size)
-- 
2.29.2

Re: [PATCH 2/5] parallels.txt: fix bitmap L1 table description

[Denis V. Lunev]

On 2/16/21 7:45 PM, Vladimir Sementsov-Ogievskiy wrote:
> Actually L1 table entry offset is in 512 bytes sectors. Fix the spec.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  docs/interop/parallels.txt | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/docs/interop/parallels.txt b/docs/interop/parallels.txt
> index f15bf35bd1..ebbdd1b25b 100644
> --- a/docs/interop/parallels.txt
> +++ b/docs/interop/parallels.txt
> @@ -209,15 +209,14 @@ of its data area are:
>                The number of entries in the L1 table of the bitmap.
>  
>    variable:   l1_table (8 * l1_size bytes)
> -              L1 offset table (in bytes)
this change is unclear. First, we have specified here the size of this
table. It is (8 * l1_size bytes). Thus it would be MUCH better to say

l1_table (size: 8 * l1_size bytes)

or

L1 offset table (l1_table), size: 8 * l1_size bytes
or something like this.


>  
>  A dirty bitmap is stored using a one-level structure for the mapping to host
> -clusters - an L1 table.
> +clusters - an L1 table. Each L1 table entry is a 64bit integer described
> +below:
>  
> -Given an offset in bytes into the bitmap data, the offset in bytes into the
> -image file can be obtained as follows:
> +Given an offset in bytes into the bitmap data, corresponding L1 entry is
>  
> -    offset = l1_table[offset / cluster_size] + (offset % cluster_size)
> +    l1_table[offset / cluster_size]
Dirty bitmap is stored in the array of clusters inside Parallels Image file.
Offsets of these clusters are saved in L1 offset table here.


>  
>  If an L1 table entry is 0, the corresponding cluster of the bitmap is assumed
>  to be zero.
If L1 table entry is 0, all bits in the corresponding cluster of the bitmap
are assumed to be 0.


> @@ -225,4 +224,8 @@ to be zero.
>  If an L1 table entry is 1, the corresponding cluster of the bitmap is assumed
>  to have all bits set.
If L1 table entry is 1, all bits in the corresponding cluster of the bitmap
are assumed to be 1.


>  
> -If an L1 table entry is not 0 or 1, it allocates a cluster from the data area.
> +If an L1 table entry is not 0 or 1, it contains corresponding cluster offset
> +(in 512b sectors). Given an offset in bytes into the bitmap data the offset in
> +bytes into the image file can be obtained as follows:
> +
> +    offset = l1_table[offset / cluster_size] * 512 + (offset % cluster_size)
looks good

[PATCH 3/5] parallels: support bitmap extension for read-only mode

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/parallels.h     |   6 +-
 block/parallels-ext.c | 286 ++++++++++++++++++++++++++++++++++++++++++
 block/parallels.c     |  18 +++
 block/meson.build     |   3 +-
 4 files changed, 311 insertions(+), 2 deletions(-)
 create mode 100644 block/parallels-ext.c

diff --git a/block/parallels.h b/block/parallels.h
index 5aa101cfc8..2dbb7668a3 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -48,7 +48,8 @@ typedef struct ParallelsHeader {
     uint64_t nb_sectors;
     uint32_t inuse;
     uint32_t data_off;
-    char padding[12];
+    uint32_t flags;
+    uint64_t ext_off;
 } QEMU_PACKED ParallelsHeader;
 
 typedef enum ParallelsPreallocMode {
@@ -84,4 +85,7 @@ typedef struct BDRVParallelsState {
     Error *migration_blocker;
 } BDRVParallelsState;
 
+int parallels_read_format_extension(BlockDriverState *bs,
+                                    int64_t ext_off, Error **errp);
+
 #endif
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
new file mode 100644
index 0000000000..b825b55124
--- /dev/null
+++ b/block/parallels-ext.c
@@ -0,0 +1,286 @@
+/*
+ * Support for Parallels Format Extansion. It's a part of parallels format
+ * driver.
+ *
+ * Copyright (c) 2021 Virtuozzo International GmbH
+ *
+ * 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.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "block/block_int.h"
+#include "parallels.h"
+#include "crypto/hash.h"
+#include "qemu/uuid.h"
+
+#define PARALLELS_FORMAT_EXTENSION_MAGIC 0xAB234CEF23DCEA87ULL
+
+#define PARALLELS_END_OF_FEATURES_MAGIC 0x0ULL
+#define PARALLELS_DIRTY_BITMAP_FEATURE_MAGIC 0x20385FAE252CB34AULL
+
+typedef struct ParallelsFormatExtensionHeader {
+    uint64_t magic; /* PARALLELS_FORMAT_EXTENSION_MAGIC */
+    uint8_t check_sum[16];
+} QEMU_PACKED ParallelsFormatExtensionHeader;
+
+typedef struct ParallelsFeatureHeader {
+    uint64_t magic;
+    uint64_t flags;
+    uint32_t data_size;
+    uint32_t _unused;
+} QEMU_PACKED ParallelsFeatureHeader;
+
+typedef struct ParallelsDirtyBitmapFeature {
+    uint64_t size;
+    uint8_t id[16];
+    uint32_t granularity;
+    uint32_t l1_size;
+    /* L1 table follows */
+} QEMU_PACKED ParallelsDirtyBitmapFeature;
+
+/* Given L1 table read bitmap data from the image and populate @bitmap */
+static int parallels_load_bitmap_data(BlockDriverState *bs,
+                                      const uint64_t *l1_table,
+                                      uint32_t l1_size,
+                                      BdrvDirtyBitmap *bitmap,
+                                      Error **errp)
+{
+    BDRVParallelsState *s = bs->opaque;
+    int ret = 0;
+    uint64_t offset, limit;
+    int cluster_size = s->tracks << BDRV_SECTOR_BITS;
+    uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
+    uint8_t *buf = NULL;
+    uint64_t i, tab_size =
+        DIV_ROUND_UP(bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size),
+                     cluster_size);
+
+    if (tab_size != l1_size) {
+        error_setg(errp, "Bitmap table size %" PRIu32 " does not correspond "
+                   "to bitmap size and cluster size. Expected %" PRIu64,
+                   l1_size, tab_size);
+        return -EINVAL;
+    }
+
+    buf = qemu_blockalign(bs, cluster_size);
+    limit = bdrv_dirty_bitmap_serialization_coverage(cluster_size, bitmap);
+    for (i = 0, offset = 0; i < tab_size; ++i, offset += limit) {
+        uint64_t count = MIN(bm_size - offset, limit);
+        uint64_t entry = l1_table[i];
+
+        if (entry == 0) {
+            /* No need to deserialize zeros because @bitmap is cleared. */
+            continue;
+        }
+
+        if (entry == 1) {
+            bdrv_dirty_bitmap_deserialize_ones(bitmap, offset, count, false);
+        } else {
+            ret = bdrv_pread(bs->file, entry << BDRV_SECTOR_BITS, buf,
+                             cluster_size);
+            if (ret < 0) {
+                error_setg_errno(errp, -ret,
+                                 "Failed to read bitmap data cluster");
+                goto finish;
+            }
+            bdrv_dirty_bitmap_deserialize_part(bitmap, buf, offset, count,
+                                               false);
+        }
+    }
+    ret = 0;
+
+    bdrv_dirty_bitmap_deserialize_finish(bitmap);
+
+finish:
+    qemu_vfree(buf);
+
+    return ret;
+}
+
+/*
+ * @data buffer (of @data_size size) is the Dirty bitmaps feature which
+ * consists of ParallelsDirtyBitmapFeature followed by L1 table.
+ */
+static BdrvDirtyBitmap *parallels_load_bitmap(BlockDriverState *bs,
+                                              uint8_t *data,
+                                              size_t data_size,
+                                              Error **errp)
+{
+    int ret;
+    ParallelsDirtyBitmapFeature bf;
+    g_autofree uint64_t *l1_table = NULL;
+    BdrvDirtyBitmap *bitmap;
+    QemuUUID uuid;
+    char uuidstr[UUID_FMT_LEN + 1];
+
+    memcpy(&bf, data, sizeof(bf));
+    bf.size = le64_to_cpu(bf.size);
+    bf.granularity = le32_to_cpu(bf.granularity) << BDRV_SECTOR_BITS;
+    bf.l1_size = le32_to_cpu(bf.l1_size);
+    data += sizeof(bf);
+    data_size -= sizeof(bf);
+
+    if (bf.size != bs->total_sectors) {
+        error_setg(errp, "Bitmap size (in sectors) %" PRId64 " differs from "
+                   "disk size in sectors %" PRId64, bf.size, bs->total_sectors);
+        return NULL;
+    }
+
+    if (bf.l1_size * sizeof(uint64_t) > data_size) {
+        error_setg(errp, "Bitmaps feature corrupted: l1 table exceeds "
+                   "extension data_size");
+        return NULL;
+    }
+
+    memcpy(&uuid, bf.id, sizeof(uuid));
+    qemu_uuid_unparse(&uuid, uuidstr);
+    bitmap = bdrv_create_dirty_bitmap(bs, bf.granularity, uuidstr, errp);
+    if (!bitmap) {
+        return NULL;
+    }
+
+    l1_table = g_memdup(data, bf.l1_size * sizeof(uint64_t));
+
+    ret = parallels_load_bitmap_data(bs, l1_table, bf.l1_size, bitmap, errp);
+    if (ret < 0) {
+        bdrv_release_dirty_bitmap(bitmap);
+        return NULL;
+    }
+
+    /* We support format extension only for RO parallels images. */
+    assert(!(bs->open_flags & BDRV_O_RDWR));
+    bdrv_dirty_bitmap_set_readonly(bitmap, true);
+
+    return bitmap;
+}
+
+static int parallels_parse_format_extension(BlockDriverState *bs,
+                                            uint8_t *ext_cluster, Error **errp)
+{
+    BDRVParallelsState *s = bs->opaque;
+    int ret;
+    int remaining = s->tracks << BDRV_SECTOR_BITS; /* one cluster */
+    uint8_t *pos = ext_cluster;
+    ParallelsFormatExtensionHeader eh;
+    g_autofree uint8_t *hash = NULL;
+    size_t hash_len = 0;
+    BdrvDirtyBitmap *bitmap = NULL;
+
+    memcpy(&eh, pos, sizeof(eh));
+    eh.magic = le64_to_cpu(eh.magic);
+    pos += sizeof(eh);
+    remaining -= sizeof(eh);
+
+    if (eh.magic != PARALLELS_FORMAT_EXTENSION_MAGIC) {
+        error_setg(errp, "Wrong parallels Format Extension magic: 0x%" PRIx64
+                   ", expected: 0x%llx", eh.magic,
+                   PARALLELS_FORMAT_EXTENSION_MAGIC);
+        goto fail;
+    }
+
+    ret = qcrypto_hash_bytes(QCRYPTO_HASH_ALG_MD5, (char *)pos, remaining,
+                             &hash, &hash_len, errp);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    if (hash_len != sizeof(eh.check_sum) ||
+        memcmp(hash, eh.check_sum, sizeof(eh.check_sum)) != 0) {
+        error_setg(errp, "Wrong checksum in Format Extension header. Format "
+                   "extension is corrupted.");
+        goto fail;
+    }
+
+    while (true) {
+        ParallelsFeatureHeader fh;
+
+        memcpy(&fh, pos, sizeof(fh));
+        pos += sizeof(fh);
+        remaining -= sizeof(fh);
+
+        fh.magic = le64_to_cpu(fh.magic);
+        fh.flags = le64_to_cpu(fh.flags);
+        fh.data_size = le32_to_cpu(fh.data_size);
+
+        if (fh.flags) {
+            error_setg(errp, "Flags for extension feature are unsupported");
+            goto fail;
+        }
+
+        if (fh.data_size > remaining) {
+            error_setg(errp, "Feature data_size exceedes Format Extension "
+                       "cluster");
+            goto fail;
+        }
+
+        switch (fh.magic) {
+        case PARALLELS_END_OF_FEATURES_MAGIC:
+            return 0;
+
+        case PARALLELS_DIRTY_BITMAP_FEATURE_MAGIC:
+            if (bitmap) {
+                error_setg(errp, "Multiple bitmaps in Format Extension");
+                goto fail;
+            }
+            bitmap = parallels_load_bitmap(bs, pos, fh.data_size, errp);
+            if (!bitmap) {
+                goto fail;
+            }
+            break;
+
+        default:
+            error_setg(errp, "Unknown feature: 0x%" PRIu64, fh.magic);
+            goto fail;
+        }
+
+        pos = ext_cluster + QEMU_ALIGN_UP(pos + fh.data_size - ext_cluster, 8);
+    }
+
+fail:
+    if (bitmap) {
+        bdrv_release_dirty_bitmap(bitmap);
+    }
+
+    return -EINVAL;
+}
+
+int parallels_read_format_extension(BlockDriverState *bs,
+                                    int64_t ext_off, Error **errp)
+{
+    BDRVParallelsState *s = bs->opaque;
+    int ret;
+    int cluster_size = s->tracks << BDRV_SECTOR_BITS;
+    uint8_t *ext_cluster = qemu_blockalign(bs, cluster_size);
+
+    assert(ext_off > 0);
+
+    ret = bdrv_pread(bs->file, ext_off, ext_cluster, cluster_size);
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to read Format Extension cluster");
+        goto out;
+    }
+
+    ret = parallels_parse_format_extension(bs, ext_cluster, errp);
+
+out:
+    qemu_vfree(ext_cluster);
+
+    return ret;
+}
diff --git a/block/parallels.c b/block/parallels.c
index 3c22dfdc9d..d33b1fa7b8 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -29,6 +29,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/error-report.h"
 #include "qapi/error.h"
 #include "block/block_int.h"
 #include "block/qdict.h"
@@ -843,6 +844,23 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail_options;
     }
 
+    if (ph.ext_off) {
+        if (flags & BDRV_O_RDWR) {
+            /*
+             * It's unsafe to open image RW if there is an extension (as we
+             * don't support it). But parallels driver in QEMU historically
+             * ignores the extension, so print warning and don't care.
+             */
+            warn_report("Format Extension ignored in RW mode");
+        } else {
+            ret = parallels_read_format_extension(
+                    bs, le64_to_cpu(ph.ext_off) << BDRV_SECTOR_BITS, errp);
+            if (ret < 0) {
+                goto fail;
+            }
+        }
+    }
+
     if ((flags & BDRV_O_RDWR) && !(flags & BDRV_O_INACTIVE)) {
         s->header->inuse = cpu_to_le32(HEADER_INUSE_MAGIC);
         ret = parallels_update_header(bs);
diff --git a/block/meson.build b/block/meson.build
index eeaefe5809..d21990ec95 100644
--- a/block/meson.build
+++ b/block/meson.build
@@ -57,7 +57,8 @@ block_ss.add(when: 'CONFIG_QED', if_true: files(
   'qed-table.c',
   'qed.c',
 ))
-block_ss.add(when: [libxml2, 'CONFIG_PARALLELS'], if_true: files('parallels.c'))
+block_ss.add(when: [libxml2, 'CONFIG_PARALLELS'],
+             if_true: files('parallels.c', 'parallels-ext.c'))
 block_ss.add(when: 'CONFIG_WIN32', if_true: files('file-win32.c', 'win32-aio.c'))
 block_ss.add(when: 'CONFIG_POSIX', if_true: [files('file-posix.c'), coref, iokit])
 block_ss.add(when: libiscsi, if_true: files('iscsi-opts.c'))
-- 
2.29.2

Re: [PATCH 3/5] parallels: support bitmap extension for read-only mode

[Denis V. Lunev]

On 2/16/21 7:45 PM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/parallels.h     |   6 +-
>  block/parallels-ext.c | 286 ++++++++++++++++++++++++++++++++++++++++++
>  block/parallels.c     |  18 +++
>  block/meson.build     |   3 +-
>  4 files changed, 311 insertions(+), 2 deletions(-)
>  create mode 100644 block/parallels-ext.c
>
> diff --git a/block/parallels.h b/block/parallels.h
> index 5aa101cfc8..2dbb7668a3 100644
> --- a/block/parallels.h
> +++ b/block/parallels.h
> @@ -48,7 +48,8 @@ typedef struct ParallelsHeader {
>      uint64_t nb_sectors;
>      uint32_t inuse;
>      uint32_t data_off;
> -    char padding[12];
> +    uint32_t flags;
> +    uint64_t ext_off;
>  } QEMU_PACKED ParallelsHeader;
>  
>  typedef enum ParallelsPreallocMode {
> @@ -84,4 +85,7 @@ typedef struct BDRVParallelsState {
>      Error *migration_blocker;
>  } BDRVParallelsState;
>  
> +int parallels_read_format_extension(BlockDriverState *bs,
> +                                    int64_t ext_off, Error **errp);
> +
>  #endif
> diff --git a/block/parallels-ext.c b/block/parallels-ext.c
> new file mode 100644
> index 0000000000..b825b55124
> --- /dev/null
> +++ b/block/parallels-ext.c
> @@ -0,0 +1,286 @@
> +/*
> + * Support for Parallels Format Extansion. It's a part of parallels format
> + * driver.
s/Extansion/Extension/
s/Support for Parallels/Support of Parallels/
s/It's a part of parallels format/It's a part of Parallels format/
> + *
> + * Copyright (c) 2021 Virtuozzo International GmbH
> + *
> + * 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.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "block/block_int.h"
> +#include "parallels.h"
> +#include "crypto/hash.h"
> +#include "qemu/uuid.h"
> +
> +#define PARALLELS_FORMAT_EXTENSION_MAGIC 0xAB234CEF23DCEA87ULL
> +
> +#define PARALLELS_END_OF_FEATURES_MAGIC 0x0ULL
> +#define PARALLELS_DIRTY_BITMAP_FEATURE_MAGIC 0x20385FAE252CB34AULL
> +
> +typedef struct ParallelsFormatExtensionHeader {
> +    uint64_t magic; /* PARALLELS_FORMAT_EXTENSION_MAGIC */
> +    uint8_t check_sum[16];
> +} QEMU_PACKED ParallelsFormatExtensionHeader;
> +
> +typedef struct ParallelsFeatureHeader {
> +    uint64_t magic;
> +    uint64_t flags;
> +    uint32_t data_size;
> +    uint32_t _unused;
> +} QEMU_PACKED ParallelsFeatureHeader;
> +
> +typedef struct ParallelsDirtyBitmapFeature {
> +    uint64_t size;
> +    uint8_t id[16];
> +    uint32_t granularity;
> +    uint32_t l1_size;
> +    /* L1 table follows */
> +} QEMU_PACKED ParallelsDirtyBitmapFeature;
> +
> +/* Given L1 table read bitmap data from the image and populate @bitmap */
> +static int parallels_load_bitmap_data(BlockDriverState *bs,
> +                                      const uint64_t *l1_table,
> +                                      uint32_t l1_size,
> +                                      BdrvDirtyBitmap *bitmap,
> +                                      Error **errp)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    int ret = 0;
> +    uint64_t offset, limit;
> +    int cluster_size = s->tracks << BDRV_SECTOR_BITS;

should we save cluster size to BDS or create helper for the purpose?
Such operation through the code is looking terrible. Originally it was
available in one place only and that was acceptable. Now it spreads
over and over.

> +    uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
> +    uint8_t *buf = NULL;
> +    uint64_t i, tab_size =
> +        DIV_ROUND_UP(bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size),
> +                     cluster_size);
> +
> +    if (tab_size != l1_size) {
> +        error_setg(errp, "Bitmap table size %" PRIu32 " does not correspond "
> +                   "to bitmap size and cluster size. Expected %" PRIu64,
> +                   l1_size, tab_size);
> +        return -EINVAL;
> +    }
> +
> +    buf = qemu_blockalign(bs, cluster_size);
> +    limit = bdrv_dirty_bitmap_serialization_coverage(cluster_size, bitmap);
> +    for (i = 0, offset = 0; i < tab_size; ++i, offset += limit) {
> +        uint64_t count = MIN(bm_size - offset, limit);
> +        uint64_t entry = l1_table[i];
> +
> +        if (entry == 0) {
> +            /* No need to deserialize zeros because @bitmap is cleared. */
> +            continue;
> +        }
> +
> +        if (entry == 1) {
> +            bdrv_dirty_bitmap_deserialize_ones(bitmap, offset, count, false);
> +        } else {
> +            ret = bdrv_pread(bs->file, entry << BDRV_SECTOR_BITS, buf,
> +                             cluster_size);
> +            if (ret < 0) {
> +                error_setg_errno(errp, -ret,
> +                                 "Failed to read bitmap data cluster");
> +                goto finish;
> +            }
> +            bdrv_dirty_bitmap_deserialize_part(bitmap, buf, offset, count,
> +                                               false);
> +        }
> +    }
> +    ret = 0;
> +
> +    bdrv_dirty_bitmap_deserialize_finish(bitmap);
> +
> +finish:
> +    qemu_vfree(buf);
> +
> +    return ret;
> +}
> +
> +/*
> + * @data buffer (of @data_size size) is the Dirty bitmaps feature which
> + * consists of ParallelsDirtyBitmapFeature followed by L1 table.
> + */
> +static BdrvDirtyBitmap *parallels_load_bitmap(BlockDriverState *bs,
> +                                              uint8_t *data,
> +                                              size_t data_size,
> +                                              Error **errp)
> +{
> +    int ret;
> +    ParallelsDirtyBitmapFeature bf;
> +    g_autofree uint64_t *l1_table = NULL;
> +    BdrvDirtyBitmap *bitmap;
> +    QemuUUID uuid;
> +    char uuidstr[UUID_FMT_LEN + 1];
> +
> +    memcpy(&bf, data, sizeof(bf));
> +    bf.size = le64_to_cpu(bf.size);
> +    bf.granularity = le32_to_cpu(bf.granularity) << BDRV_SECTOR_BITS;
> +    bf.l1_size = le32_to_cpu(bf.l1_size);
> +    data += sizeof(bf);
> +    data_size -= sizeof(bf);
could this be underflowed? size_t is unsigned

> +
> +    if (bf.size != bs->total_sectors) {
> +        error_setg(errp, "Bitmap size (in sectors) %" PRId64 " differs from "
> +                   "disk size in sectors %" PRId64, bf.size, bs->total_sectors);
> +        return NULL;
> +    }
> +
> +    if (bf.l1_size * sizeof(uint64_t) > data_size) {
> +        error_setg(errp, "Bitmaps feature corrupted: l1 table exceeds "
> +                   "extension data_size");
> +        return NULL;
> +    }
> +
> +    memcpy(&uuid, bf.id, sizeof(uuid));
> +    qemu_uuid_unparse(&uuid, uuidstr);
> +    bitmap = bdrv_create_dirty_bitmap(bs, bf.granularity, uuidstr, errp);
> +    if (!bitmap) {
> +        return NULL;
> +    }
> +
> +    l1_table = g_memdup(data, bf.l1_size * sizeof(uint64_t));
le64 to CPU conversion seems missed

> +
> +    ret = parallels_load_bitmap_data(bs, l1_table, bf.l1_size, bitmap, errp);
> +    if (ret < 0) {
> +        bdrv_release_dirty_bitmap(bitmap);
l1_table is leaked

> +        return NULL;
> +    }
> +
> +    /* We support format extension only for RO parallels images. */
> +    assert(!(bs->open_flags & BDRV_O_RDWR));
This is what I am not fully agree with. We support bitmap RO, i.e. we
will not
be able to continue tracking and write this again, but for the purpose
of the reverse delta we could have the image RW.

and even for the case of consistency, do you feel that assert could be
tooooooo strong. Our colleagues will come to us here with the
blames that QEMU has been crashed even they are doing something
completely wrong :)

> +    bdrv_dirty_bitmap_set_readonly(bitmap, true);
l1_table is leaked here too


> +
> +    return bitmap;
> +}
> +
> +static int parallels_parse_format_extension(BlockDriverState *bs,
> +                                            uint8_t *ext_cluster, Error **errp)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    int ret;
> +    int remaining = s->tracks << BDRV_SECTOR_BITS; /* one cluster */
> +    uint8_t *pos = ext_cluster;
> +    ParallelsFormatExtensionHeader eh;
> +    g_autofree uint8_t *hash = NULL;
> +    size_t hash_len = 0;
> +    BdrvDirtyBitmap *bitmap = NULL;
> +
> +    memcpy(&eh, pos, sizeof(eh));
> +    eh.magic = le64_to_cpu(eh.magic);
> +    pos += sizeof(eh);
> +    remaining -= sizeof(eh);
> +
> +    if (eh.magic != PARALLELS_FORMAT_EXTENSION_MAGIC) {
> +        error_setg(errp, "Wrong parallels Format Extension magic: 0x%" PRIx64
> +                   ", expected: 0x%llx", eh.magic,
> +                   PARALLELS_FORMAT_EXTENSION_MAGIC);
> +        goto fail;
> +    }
> +
> +    ret = qcrypto_hash_bytes(QCRYPTO_HASH_ALG_MD5, (char *)pos, remaining,
> +                             &hash, &hash_len, errp);
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +
> +    if (hash_len != sizeof(eh.check_sum) ||
> +        memcmp(hash, eh.check_sum, sizeof(eh.check_sum)) != 0) {
> +        error_setg(errp, "Wrong checksum in Format Extension header. Format "
> +                   "extension is corrupted.");
> +        goto fail;
> +    }
> +
> +    while (true) {
> +        ParallelsFeatureHeader fh;
> +
> +        memcpy(&fh, pos, sizeof(fh));
> +        pos += sizeof(fh);
> +        remaining -= sizeof(fh);
remaining is to be checked before memcpy to avoid reading beyond end of
the cluster

> +
> +        fh.magic = le64_to_cpu(fh.magic);
> +        fh.flags = le64_to_cpu(fh.flags);
> +        fh.data_size = le32_to_cpu(fh.data_size);
> +
> +        if (fh.flags) {
> +            error_setg(errp, "Flags for extension feature are unsupported");
> +            goto fail;
> +        }
> +
> +        if (fh.data_size > remaining) {
> +            error_setg(errp, "Feature data_size exceedes Format Extension "
> +                       "cluster");
> +            goto fail;
> +        }
> +
> +        switch (fh.magic) {
> +        case PARALLELS_END_OF_FEATURES_MAGIC:
> +            return 0;
> +
> +        case PARALLELS_DIRTY_BITMAP_FEATURE_MAGIC:
> +            if (bitmap) {
> +                error_setg(errp, "Multiple bitmaps in Format Extension");
> +                goto fail;
> +            }
unsure at the moment. May be this is too restrictive.

> +            bitmap = parallels_load_bitmap(bs, pos, fh.data_size, errp);
> +            if (!bitmap) {
> +                goto fail;
> +            }
> +            break;
> +
> +        default:
> +            error_setg(errp, "Unknown feature: 0x%" PRIu64, fh.magic);
> +            goto fail;
> +        }
> +
> +        pos = ext_cluster + QEMU_ALIGN_UP(pos + fh.data_size - ext_cluster, 8);
> +    }
> +
> +fail:
> +    if (bitmap) {
> +        bdrv_release_dirty_bitmap(bitmap);
> +    }
> +
> +    return -EINVAL;
> +}
> +
> +int parallels_read_format_extension(BlockDriverState *bs,
> +                                    int64_t ext_off, Error **errp)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    int ret;
> +    int cluster_size = s->tracks << BDRV_SECTOR_BITS;
> +    uint8_t *ext_cluster = qemu_blockalign(bs, cluster_size);
> +
> +    assert(ext_off > 0);
> +
> +    ret = bdrv_pread(bs->file, ext_off, ext_cluster, cluster_size);
> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret, "Failed to read Format Extension cluster");
> +        goto out;
> +    }
> +
> +    ret = parallels_parse_format_extension(bs, ext_cluster, errp);
> +
> +out:
> +    qemu_vfree(ext_cluster);
> +
> +    return ret;
> +}
> diff --git a/block/parallels.c b/block/parallels.c
> index 3c22dfdc9d..d33b1fa7b8 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -29,6 +29,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
>  #include "qapi/error.h"
>  #include "block/block_int.h"
>  #include "block/qdict.h"
> @@ -843,6 +844,23 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>          goto fail_options;
>      }
>  
> +    if (ph.ext_off) {
> +        if (flags & BDRV_O_RDWR) {
> +            /*
> +             * It's unsafe to open image RW if there is an extension (as we
> +             * don't support it). But parallels driver in QEMU historically
> +             * ignores the extension, so print warning and don't care.
> +             */
> +            warn_report("Format Extension ignored in RW mode");
> +        } else {
> +            ret = parallels_read_format_extension(
> +                    bs, le64_to_cpu(ph.ext_off) << BDRV_SECTOR_BITS, errp);
> +            if (ret < 0) {
> +                goto fail;
> +            }
> +        }
> +    }
> +
>      if ((flags & BDRV_O_RDWR) && !(flags & BDRV_O_INACTIVE)) {
>          s->header->inuse = cpu_to_le32(HEADER_INUSE_MAGIC);
>          ret = parallels_update_header(bs);
> diff --git a/block/meson.build b/block/meson.build
> index eeaefe5809..d21990ec95 100644
> --- a/block/meson.build
> +++ b/block/meson.build
> @@ -57,7 +57,8 @@ block_ss.add(when: 'CONFIG_QED', if_true: files(
>    'qed-table.c',
>    'qed.c',
>  ))
> -block_ss.add(when: [libxml2, 'CONFIG_PARALLELS'], if_true: files('parallels.c'))
> +block_ss.add(when: [libxml2, 'CONFIG_PARALLELS'],
> +             if_true: files('parallels.c', 'parallels-ext.c'))
>  block_ss.add(when: 'CONFIG_WIN32', if_true: files('file-win32.c', 'win32-aio.c'))
>  block_ss.add(when: 'CONFIG_POSIX', if_true: [files('file-posix.c'), coref, iokit])
>  block_ss.add(when: libiscsi, if_true: files('iscsi-opts.c'))

Re: [PATCH 3/5] parallels: support bitmap extension for read-only mode

[Vladimir Sementsov-Ogievskiy]

19.02.2021 14:47, Denis V. Lunev wrote:
> On 2/16/21 7:45 PM, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/parallels.h     |   6 +-
>>   block/parallels-ext.c | 286 ++++++++++++++++++++++++++++++++++++++++++
>>   block/parallels.c     |  18 +++
>>   block/meson.build     |   3 +-
>>   4 files changed, 311 insertions(+), 2 deletions(-)
>>   create mode 100644 block/parallels-ext.c
>>
>> diff --git a/block/parallels.h b/block/parallels.h
>> index 5aa101cfc8..2dbb7668a3 100644
>> --- a/block/parallels.h
>> +++ b/block/parallels.h
>> @@ -48,7 +48,8 @@ typedef struct ParallelsHeader {
>>       uint64_t nb_sectors;
>>       uint32_t inuse;
>>       uint32_t data_off;
>> -    char padding[12];
>> +    uint32_t flags;
>> +    uint64_t ext_off;
>>   } QEMU_PACKED ParallelsHeader;
>>   
>>   typedef enum ParallelsPreallocMode {
>> @@ -84,4 +85,7 @@ typedef struct BDRVParallelsState {
>>       Error *migration_blocker;
>>   } BDRVParallelsState;
>>   
>> +int parallels_read_format_extension(BlockDriverState *bs,
>> +                                    int64_t ext_off, Error **errp);
>> +
>>   #endif
>> diff --git a/block/parallels-ext.c b/block/parallels-ext.c
>> new file mode 100644
>> index 0000000000..b825b55124
>> --- /dev/null
>> +++ b/block/parallels-ext.c
>> @@ -0,0 +1,286 @@
>> +/*
>> + * Support for Parallels Format Extansion. It's a part of parallels format
>> + * driver.
> s/Extansion/Extension/
> s/Support for Parallels/Support of Parallels/
> s/It's a part of parallels format/It's a part of Parallels format/
>> + *
>> + * Copyright (c) 2021 Virtuozzo International GmbH
>> + *
>> + * 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.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "block/block_int.h"
>> +#include "parallels.h"
>> +#include "crypto/hash.h"
>> +#include "qemu/uuid.h"
>> +
>> +#define PARALLELS_FORMAT_EXTENSION_MAGIC 0xAB234CEF23DCEA87ULL
>> +
>> +#define PARALLELS_END_OF_FEATURES_MAGIC 0x0ULL
>> +#define PARALLELS_DIRTY_BITMAP_FEATURE_MAGIC 0x20385FAE252CB34AULL
>> +
>> +typedef struct ParallelsFormatExtensionHeader {
>> +    uint64_t magic; /* PARALLELS_FORMAT_EXTENSION_MAGIC */
>> +    uint8_t check_sum[16];
>> +} QEMU_PACKED ParallelsFormatExtensionHeader;
>> +
>> +typedef struct ParallelsFeatureHeader {
>> +    uint64_t magic;
>> +    uint64_t flags;
>> +    uint32_t data_size;
>> +    uint32_t _unused;
>> +} QEMU_PACKED ParallelsFeatureHeader;
>> +
>> +typedef struct ParallelsDirtyBitmapFeature {
>> +    uint64_t size;
>> +    uint8_t id[16];
>> +    uint32_t granularity;
>> +    uint32_t l1_size;
>> +    /* L1 table follows */
>> +} QEMU_PACKED ParallelsDirtyBitmapFeature;
>> +
>> +/* Given L1 table read bitmap data from the image and populate @bitmap */
>> +static int parallels_load_bitmap_data(BlockDriverState *bs,
>> +                                      const uint64_t *l1_table,
>> +                                      uint32_t l1_size,
>> +                                      BdrvDirtyBitmap *bitmap,
>> +                                      Error **errp)
>> +{
>> +    BDRVParallelsState *s = bs->opaque;
>> +    int ret = 0;
>> +    uint64_t offset, limit;
>> +    int cluster_size = s->tracks << BDRV_SECTOR_BITS;
> 
> should we save cluster size to BDS or create helper for the purpose?
> Such operation through the code is looking terrible. Originally it was
> available in one place only and that was acceptable. Now it spreads
> over and over.

Agree, will do.

> 
>> +    uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
>> +    uint8_t *buf = NULL;
>> +    uint64_t i, tab_size =
>> +        DIV_ROUND_UP(bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size),
>> +                     cluster_size);
>> +
>> +    if (tab_size != l1_size) {
>> +        error_setg(errp, "Bitmap table size %" PRIu32 " does not correspond "
>> +                   "to bitmap size and cluster size. Expected %" PRIu64,
>> +                   l1_size, tab_size);
>> +        return -EINVAL;
>> +    }
>> +
>> +    buf = qemu_blockalign(bs, cluster_size);
>> +    limit = bdrv_dirty_bitmap_serialization_coverage(cluster_size, bitmap);
>> +    for (i = 0, offset = 0; i < tab_size; ++i, offset += limit) {
>> +        uint64_t count = MIN(bm_size - offset, limit);
>> +        uint64_t entry = l1_table[i];
>> +
>> +        if (entry == 0) {
>> +            /* No need to deserialize zeros because @bitmap is cleared. */
>> +            continue;
>> +        }
>> +
>> +        if (entry == 1) {
>> +            bdrv_dirty_bitmap_deserialize_ones(bitmap, offset, count, false);
>> +        } else {
>> +            ret = bdrv_pread(bs->file, entry << BDRV_SECTOR_BITS, buf,
>> +                             cluster_size);
>> +            if (ret < 0) {
>> +                error_setg_errno(errp, -ret,
>> +                                 "Failed to read bitmap data cluster");
>> +                goto finish;
>> +            }
>> +            bdrv_dirty_bitmap_deserialize_part(bitmap, buf, offset, count,
>> +                                               false);
>> +        }
>> +    }
>> +    ret = 0;
>> +
>> +    bdrv_dirty_bitmap_deserialize_finish(bitmap);
>> +
>> +finish:
>> +    qemu_vfree(buf);
>> +
>> +    return ret;
>> +}
>> +
>> +/*
>> + * @data buffer (of @data_size size) is the Dirty bitmaps feature which
>> + * consists of ParallelsDirtyBitmapFeature followed by L1 table.
>> + */
>> +static BdrvDirtyBitmap *parallels_load_bitmap(BlockDriverState *bs,
>> +                                              uint8_t *data,
>> +                                              size_t data_size,
>> +                                              Error **errp)
>> +{
>> +    int ret;
>> +    ParallelsDirtyBitmapFeature bf;
>> +    g_autofree uint64_t *l1_table = NULL;
>> +    BdrvDirtyBitmap *bitmap;
>> +    QemuUUID uuid;
>> +    char uuidstr[UUID_FMT_LEN + 1];
>> +
>> +    memcpy(&bf, data, sizeof(bf));
>> +    bf.size = le64_to_cpu(bf.size);
>> +    bf.granularity = le32_to_cpu(bf.granularity) << BDRV_SECTOR_BITS;
>> +    bf.l1_size = le32_to_cpu(bf.l1_size);
>> +    data += sizeof(bf);
>> +    data_size -= sizeof(bf);
> could this be underflowed? size_t is unsigned

I should check the size at function start

> 
>> +
>> +    if (bf.size != bs->total_sectors) {
>> +        error_setg(errp, "Bitmap size (in sectors) %" PRId64 " differs from "
>> +                   "disk size in sectors %" PRId64, bf.size, bs->total_sectors);
>> +        return NULL;
>> +    }
>> +
>> +    if (bf.l1_size * sizeof(uint64_t) > data_size) {
>> +        error_setg(errp, "Bitmaps feature corrupted: l1 table exceeds "
>> +                   "extension data_size");
>> +        return NULL;
>> +    }
>> +
>> +    memcpy(&uuid, bf.id, sizeof(uuid));
>> +    qemu_uuid_unparse(&uuid, uuidstr);
>> +    bitmap = bdrv_create_dirty_bitmap(bs, bf.granularity, uuidstr, errp);
>> +    if (!bitmap) {
>> +        return NULL;
>> +    }
>> +
>> +    l1_table = g_memdup(data, bf.l1_size * sizeof(uint64_t));
> le64 to CPU conversion seems missed

for l1_table entries.. yes. will fix

> 
>> +
>> +    ret = parallels_load_bitmap_data(bs, l1_table, bf.l1_size, bitmap, errp);
>> +    if (ret < 0) {
>> +        bdrv_release_dirty_bitmap(bitmap);
> l1_table is leaked

no, it's defined with g_autofree, so it's automatically freed.

> 
>> +        return NULL;
>> +    }
>> +
>> +    /* We support format extension only for RO parallels images. */
>> +    assert(!(bs->open_flags & BDRV_O_RDWR));
> This is what I am not fully agree with. We support bitmap RO, i.e. we
> will not
> be able to continue tracking and write this again, but for the purpose
> of the reverse delta we could have the image RW.
> 
> and even for the case of consistency, do you feel that assert could be
> tooooooo strong. Our colleagues will come to us here with the
> blames that QEMU has been crashed even they are doing something
> completely wrong :)

Assert here is OK, as we will not come here on RW image, as we don't load bitmaps for RW image, so it's OK...

About reverse delta, I think we'd better use qcow2 as delta. We can load bitmap from RO parallels and copy it to qcow2 delta.

What you want is loading bitmap in "disabled" mode for RW parallels image. In qcow2 bitmaps format we have corresponding flag,
so we can store active and disabled bitmaps, and load them as active and disabled appropriately. In parallels format we
don't have this flag. So, I think, by default the bitmap in parallels format is assumed to be active, and we must update it
when disk is written to. If we want to change this behavior we'll need an open flag for parallels format like
load-bitmaps-as-disabled=true..

> 
>> +    bdrv_dirty_bitmap_set_readonly(bitmap, true);
> l1_table is leaked here too
> 
> 
>> +
>> +    return bitmap;
>> +}
>> +
>> +static int parallels_parse_format_extension(BlockDriverState *bs,
>> +                                            uint8_t *ext_cluster, Error **errp)
>> +{
>> +    BDRVParallelsState *s = bs->opaque;
>> +    int ret;
>> +    int remaining = s->tracks << BDRV_SECTOR_BITS; /* one cluster */
>> +    uint8_t *pos = ext_cluster;
>> +    ParallelsFormatExtensionHeader eh;
>> +    g_autofree uint8_t *hash = NULL;
>> +    size_t hash_len = 0;
>> +    BdrvDirtyBitmap *bitmap = NULL;
>> +
>> +    memcpy(&eh, pos, sizeof(eh));
>> +    eh.magic = le64_to_cpu(eh.magic);
>> +    pos += sizeof(eh);
>> +    remaining -= sizeof(eh);
>> +
>> +    if (eh.magic != PARALLELS_FORMAT_EXTENSION_MAGIC) {
>> +        error_setg(errp, "Wrong parallels Format Extension magic: 0x%" PRIx64
>> +                   ", expected: 0x%llx", eh.magic,
>> +                   PARALLELS_FORMAT_EXTENSION_MAGIC);
>> +        goto fail;
>> +    }
>> +
>> +    ret = qcrypto_hash_bytes(QCRYPTO_HASH_ALG_MD5, (char *)pos, remaining,
>> +                             &hash, &hash_len, errp);
>> +    if (ret < 0) {
>> +        goto fail;
>> +    }
>> +
>> +    if (hash_len != sizeof(eh.check_sum) ||
>> +        memcmp(hash, eh.check_sum, sizeof(eh.check_sum)) != 0) {
>> +        error_setg(errp, "Wrong checksum in Format Extension header. Format "
>> +                   "extension is corrupted.");
>> +        goto fail;
>> +    }
>> +
>> +    while (true) {
>> +        ParallelsFeatureHeader fh;
>> +
>> +        memcpy(&fh, pos, sizeof(fh));
>> +        pos += sizeof(fh);
>> +        remaining -= sizeof(fh);
> remaining is to be checked before memcpy to avoid reading beyond end of
> the cluster

agree

> 
>> +
>> +        fh.magic = le64_to_cpu(fh.magic);
>> +        fh.flags = le64_to_cpu(fh.flags);
>> +        fh.data_size = le32_to_cpu(fh.data_size);
>> +
>> +        if (fh.flags) {
>> +            error_setg(errp, "Flags for extension feature are unsupported");
>> +            goto fail;
>> +        }
>> +
>> +        if (fh.data_size > remaining) {
>> +            error_setg(errp, "Feature data_size exceedes Format Extension "
>> +                       "cluster");
>> +            goto fail;
>> +        }
>> +
>> +        switch (fh.magic) {
>> +        case PARALLELS_END_OF_FEATURES_MAGIC:
>> +            return 0;
>> +
>> +        case PARALLELS_DIRTY_BITMAP_FEATURE_MAGIC:
>> +            if (bitmap) {
>> +                error_setg(errp, "Multiple bitmaps in Format Extension");
>> +                goto fail;
>> +            }
> unsure at the moment. May be this is too restrictive.

Hmm. Somehow I thought that it is mentioned in spec.. But now I see that it doesn't. Will improve to load several bitmaps it's not difficult.

> 
>> +            bitmap = parallels_load_bitmap(bs, pos, fh.data_size, errp);
>> +            if (!bitmap) {
>> +                goto fail;
>> +            }
>> +            break;
>> +
>> +        default:
>> +            error_setg(errp, "Unknown feature: 0x%" PRIu64, fh.magic);
>> +            goto fail;
>> +        }
>> +
>> +        pos = ext_cluster + QEMU_ALIGN_UP(pos + fh.data_size - ext_cluster, 8);
>> +    }
>> +
>> +fail:
>> +    if (bitmap) {
>> +        bdrv_release_dirty_bitmap(bitmap);
>> +    }
>> +
>> +    return -EINVAL;
>> +}
>> +
>> +int parallels_read_format_extension(BlockDriverState *bs,
>> +                                    int64_t ext_off, Error **errp)
>> +{
>> +    BDRVParallelsState *s = bs->opaque;
>> +    int ret;
>> +    int cluster_size = s->tracks << BDRV_SECTOR_BITS;
>> +    uint8_t *ext_cluster = qemu_blockalign(bs, cluster_size);
>> +
>> +    assert(ext_off > 0);
>> +
>> +    ret = bdrv_pread(bs->file, ext_off, ext_cluster, cluster_size);
>> +    if (ret < 0) {
>> +        error_setg_errno(errp, -ret, "Failed to read Format Extension cluster");
>> +        goto out;
>> +    }
>> +
>> +    ret = parallels_parse_format_extension(bs, ext_cluster, errp);
>> +
>> +out:
>> +    qemu_vfree(ext_cluster);
>> +
>> +    return ret;
>> +}
>> diff --git a/block/parallels.c b/block/parallels.c
>> index 3c22dfdc9d..d33b1fa7b8 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -29,6 +29,7 @@
>>    */
>>   
>>   #include "qemu/osdep.h"
>> +#include "qemu/error-report.h"
>>   #include "qapi/error.h"
>>   #include "block/block_int.h"
>>   #include "block/qdict.h"
>> @@ -843,6 +844,23 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>           goto fail_options;
>>       }
>>   
>> +    if (ph.ext_off) {
>> +        if (flags & BDRV_O_RDWR) {
>> +            /*
>> +             * It's unsafe to open image RW if there is an extension (as we
>> +             * don't support it). But parallels driver in QEMU historically
>> +             * ignores the extension, so print warning and don't care.
>> +             */
>> +            warn_report("Format Extension ignored in RW mode");
>> +        } else {
>> +            ret = parallels_read_format_extension(
>> +                    bs, le64_to_cpu(ph.ext_off) << BDRV_SECTOR_BITS, errp);
>> +            if (ret < 0) {
>> +                goto fail;
>> +            }
>> +        }
>> +    }
>> +
>>       if ((flags & BDRV_O_RDWR) && !(flags & BDRV_O_INACTIVE)) {
>>           s->header->inuse = cpu_to_le32(HEADER_INUSE_MAGIC);
>>           ret = parallels_update_header(bs);
>> diff --git a/block/meson.build b/block/meson.build
>> index eeaefe5809..d21990ec95 100644
>> --- a/block/meson.build
>> +++ b/block/meson.build
>> @@ -57,7 +57,8 @@ block_ss.add(when: 'CONFIG_QED', if_true: files(
>>     'qed-table.c',
>>     'qed.c',
>>   ))
>> -block_ss.add(when: [libxml2, 'CONFIG_PARALLELS'], if_true: files('parallels.c'))
>> +block_ss.add(when: [libxml2, 'CONFIG_PARALLELS'],
>> +             if_true: files('parallels.c', 'parallels-ext.c'))
>>   block_ss.add(when: 'CONFIG_WIN32', if_true: files('file-win32.c', 'win32-aio.c'))
>>   block_ss.add(when: 'CONFIG_POSIX', if_true: [files('file-posix.c'), coref, iokit])
>>   block_ss.add(when: libiscsi, if_true: files('iscsi-opts.c'))
> 


-- 
Best regards,
Vladimir

[PATCH 4/5] iotests.py: add unarchive_sample_image() helper

[Vladimir Sementsov-Ogievskiy via]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/iotests.py | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 00be68eca3..ea6238f242 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -17,6 +17,7 @@
 #
 
 import atexit
+import bz2
 from collections import OrderedDict
 import faulthandler
 import io
@@ -24,6 +25,7 @@ import json
 import logging
 import os
 import re
+import shutil
 import signal
 import struct
 import subprocess
@@ -96,6 +98,14 @@ luks_default_secret_object = 'secret,id=keysec0,data=' + \
                              os.environ.get('IMGKEYSECRET', '')
 luks_default_key_secret_opt = 'key-secret=keysec0'
 
+sample_img_dir = os.environ['SAMPLE_IMG_DIR']
+
+
+def unarchive_sample_image(sample, fname):
+    sample_fname = os.path.join(sample_img_dir, sample + '.bz2')
+    with bz2.open(sample_fname) as f_in, open(fname, 'wb') as f_out:
+        shutil.copyfileobj(f_in, f_out)
+
 
 def qemu_tool_pipe_and_status(tool: str, args: Sequence[str],
                               connect_stderr: bool = True) -> Tuple[str, int]:
-- 
2.29.2

[PATCH 5/5] iotests: add parallels-read-bitmap test

[Vladimir Sementsov-Ogievskiy]

Test support for reading bitmap from parallels image format.
parallels-with-bitmap.bz2 is generated on Virtuozzo by
parallels-with-bitmap.sh

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 .../sample_images/parallels-with-bitmap.bz2   | Bin 0 -> 203 bytes
 .../sample_images/parallels-with-bitmap.sh    |  33 ++++++++++
 .../qemu-iotests/tests/parallels-read-bitmap  |  57 ++++++++++++++++++
 .../tests/parallels-read-bitmap.out           |   6 ++
 4 files changed, 96 insertions(+)
 create mode 100644 tests/qemu-iotests/sample_images/parallels-with-bitmap.bz2
 create mode 100755 tests/qemu-iotests/sample_images/parallels-with-bitmap.sh
 create mode 100755 tests/qemu-iotests/tests/parallels-read-bitmap
 create mode 100644 tests/qemu-iotests/tests/parallels-read-bitmap.out

diff --git a/tests/qemu-iotests/sample_images/parallels-with-bitmap.bz2 b/tests/qemu-iotests/sample_images/parallels-with-bitmap.bz2
new file mode 100644
index 0000000000000000000000000000000000000000..54892fd4d01bf743d395bd4f3d896494146ab5a9
GIT binary patch
literal 203
zcmV;+05tzXT4*^jL0KkKS@=;0bpT+Hf7|^?Km<xfFyKQJ7=Y^F-%vt;00~Ysa6|-=
zk&7Szk`SoS002EkfMftPG<ipnsiCK}K_sNmm}me3FiZr%Oaf_u5F8kD;mB_~cxD-r
z5P$(X{&Tq5C`<xK02D?NNdN+t$~z$m00O|zFh^ynq*yaCtkn+NZzWom<#OEoF`?zb
zv(i3x^K~wt!aLPcRBP+PckUsIh6*LgjYSh0`}#7hMC9NR5D)+W0d&8Mxgwk>NPH-R
Fx`3oHQ9u9y

literal 0
HcmV?d00001

diff --git a/tests/qemu-iotests/sample_images/parallels-with-bitmap.sh b/tests/qemu-iotests/sample_images/parallels-with-bitmap.sh
new file mode 100755
index 0000000000..e4a1d71277
--- /dev/null
+++ b/tests/qemu-iotests/sample_images/parallels-with-bitmap.sh
@@ -0,0 +1,33 @@
+#!/bin/bash
+
+CT=parallels-with-bitmap-ct
+DIR=$PWD/parallels-with-bitmap-dir
+IMG=$DIR/root.hds
+XML=$DIR/DiskDescriptor.xml
+TARGET=parallels-with-bitmap.bz2
+
+rm -rf $DIR
+
+prlctl create $CT --vmtype ct
+prlctl set $CT --device-add hdd --image $DIR --recreate --size 2G
+
+# cleanup the image
+qemu-img create -f parallels $IMG 64G
+
+# create bitmap
+prlctl backup $CT
+
+prlctl set $CT --device-del hdd1
+prlctl destroy $CT
+
+dev=$(ploop mount $XML | sed -n 's/^Adding delta dev=\(\/dev\/ploop[0-9]\+\).*/\1/p')
+dd if=/dev/zero of=$dev bs=64K seek=5 count=2 oflag=direct
+dd if=/dev/zero of=$dev bs=64K seek=30 count=1 oflag=direct
+dd if=/dev/zero of=$dev bs=64K seek=10 count=3 oflag=direct
+ploop umount $XML  # bitmap name will be in the output
+
+bzip2 -z $IMG
+
+mv $IMG.bz2 $TARGET
+
+rm -rf $DIR
diff --git a/tests/qemu-iotests/tests/parallels-read-bitmap b/tests/qemu-iotests/tests/parallels-read-bitmap
new file mode 100755
index 0000000000..b50b79f509
--- /dev/null
+++ b/tests/qemu-iotests/tests/parallels-read-bitmap
@@ -0,0 +1,57 @@
+#!/usr/bin/env python3
+#
+# Test parallels load bitmap
+#
+# Copyright (c) 2021 Virtuozzo International GmbH.
+#
+# 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 json
+import iotests
+from iotests import qemu_nbd_popen, qemu_img_pipe, log, file_path
+
+iotests.script_initialize(supported_fmts=['parallels'])
+
+nbd_sock = file_path('nbd-sock', base_dir=iotests.sock_dir)
+disk = iotests.file_path('disk')
+bitmap = 'e4f2eed0-37fe-4539-b50b-85d2e7fd235f'
+nbd_opts = f'driver=nbd,server.type=unix,server.path={nbd_sock}' \
+        f',x-dirty-bitmap=qemu:dirty-bitmap:{bitmap}'
+
+
+iotests.unarchive_sample_image('parallels-with-bitmap', disk)
+
+iotests.unarchive_sample_image('parallels-with-bitmap', '/work/mega')
+
+
+with qemu_nbd_popen('--read-only', f'--socket={nbd_sock}',
+                    f'--bitmap={bitmap}', '-f', iotests.imgfmt, disk):
+    out = qemu_img_pipe('map', '--output=json', '--image-opts', nbd_opts)
+    chunks = json.loads(out)
+    cluster = 64 * 1024
+
+    log('dirty clusters (cluster size is 64K):')
+    for c in chunks:
+        assert c['start'] % cluster == 0
+        assert c['length'] % cluster == 0
+        if c['data']:
+            continue
+
+        a = c['start'] // cluster
+        b = (c['start'] + c['length']) // cluster
+        if b - a > 1:
+            log(f'{a}-{b-1}')
+        else:
+            log(a)
diff --git a/tests/qemu-iotests/tests/parallels-read-bitmap.out b/tests/qemu-iotests/tests/parallels-read-bitmap.out
new file mode 100644
index 0000000000..e8f6bc9e96
--- /dev/null
+++ b/tests/qemu-iotests/tests/parallels-read-bitmap.out
@@ -0,0 +1,6 @@
+Start NBD server
+dirty clusters (cluster size is 64K):
+5-6
+10-12
+30
+Kill NBD server
-- 
2.29.2

Re: [PATCH 5/5] iotests: add parallels-read-bitmap test

[Denis V. Lunev]

On 2/16/21 7:45 PM, Vladimir Sementsov-Ogievskiy wrote:
> Test support for reading bitmap from parallels image format.
> parallels-with-bitmap.bz2 is generated on Virtuozzo by
> parallels-with-bitmap.sh
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  .../sample_images/parallels-with-bitmap.bz2   | Bin 0 -> 203 bytes
>  .../sample_images/parallels-with-bitmap.sh    |  33 ++++++++++
>  .../qemu-iotests/tests/parallels-read-bitmap  |  57 ++++++++++++++++++
>  .../tests/parallels-read-bitmap.out           |   6 ++
>  4 files changed, 96 insertions(+)
>  create mode 100644 tests/qemu-iotests/sample_images/parallels-with-bitmap.bz2
>  create mode 100755 tests/qemu-iotests/sample_images/parallels-with-bitmap.sh
>  create mode 100755 tests/qemu-iotests/tests/parallels-read-bitmap
>  create mode 100644 tests/qemu-iotests/tests/parallels-read-bitmap.out
>
> diff --git a/tests/qemu-iotests/sample_images/parallels-with-bitmap.bz2 b/tests/qemu-iotests/sample_images/parallels-with-bitmap.bz2
> new file mode 100644
> index 0000000000000000000000000000000000000000..54892fd4d01bf743d395bd4f3d896494146ab5a9
> GIT binary patch
> literal 203
> zcmV;+05tzXT4*^jL0KkKS@=;0bpT+Hf7|^?Km<xfFyKQJ7=Y^F-%vt;00~Ysa6|-=
> zk&7Szk`SoS002EkfMftPG<ipnsiCK}K_sNmm}me3FiZr%Oaf_u5F8kD;mB_~cxD-r
> z5P$(X{&Tq5C`<xK02D?NNdN+t$~z$m00O|zFh^ynq*yaCtkn+NZzWom<#OEoF`?zb
> zv(i3x^K~wt!aLPcRBP+PckUsIh6*LgjYSh0`}#7hMC9NR5D)+W0d&8Mxgwk>NPH-R
> Fx`3oHQ9u9y
>
> literal 0
> HcmV?d00001
>
> diff --git a/tests/qemu-iotests/sample_images/parallels-with-bitmap.sh b/tests/qemu-iotests/sample_images/parallels-with-bitmap.sh
> new file mode 100755
> index 0000000000..e4a1d71277
> --- /dev/null
> +++ b/tests/qemu-iotests/sample_images/parallels-with-bitmap.sh
> @@ -0,0 +1,33 @@
do we need Copyright notice here? I am unsure that this script is to be
acceptable in QEMU repo. Anyway, it looks fine :)


> +#!/bin/bash
> +
> +CT=parallels-with-bitmap-ct
> +DIR=$PWD/parallels-with-bitmap-dir
> +IMG=$DIR/root.hds
> +XML=$DIR/DiskDescriptor.xml
> +TARGET=parallels-with-bitmap.bz2
> +
> +rm -rf $DIR
> +
> +prlctl create $CT --vmtype ct
> +prlctl set $CT --device-add hdd --image $DIR --recreate --size 2G
> +
> +# cleanup the image
> +qemu-img create -f parallels $IMG 64G
> +
> +# create bitmap
> +prlctl backup $CT
> +
> +prlctl set $CT --device-del hdd1
> +prlctl destroy $CT
> +
> +dev=$(ploop mount $XML | sed -n 's/^Adding delta dev=\(\/dev\/ploop[0-9]\+\).*/\1/p')
> +dd if=/dev/zero of=$dev bs=64K seek=5 count=2 oflag=direct
> +dd if=/dev/zero of=$dev bs=64K seek=30 count=1 oflag=direct
> +dd if=/dev/zero of=$dev bs=64K seek=10 count=3 oflag=direct
> +ploop umount $XML  # bitmap name will be in the output
> +
> +bzip2 -z $IMG
> +
> +mv $IMG.bz2 $TARGET
> +
> +rm -rf $DIR
> diff --git a/tests/qemu-iotests/tests/parallels-read-bitmap b/tests/qemu-iotests/tests/parallels-read-bitmap
> new file mode 100755
> index 0000000000..b50b79f509
> --- /dev/null
> +++ b/tests/qemu-iotests/tests/parallels-read-bitmap
> @@ -0,0 +1,57 @@
> +#!/usr/bin/env python3
> +#
> +# Test parallels load bitmap
> +#
> +# Copyright (c) 2021 Virtuozzo International GmbH.
> +#
> +# 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 json
> +import iotests
> +from iotests import qemu_nbd_popen, qemu_img_pipe, log, file_path
> +
> +iotests.script_initialize(supported_fmts=['parallels'])
> +
> +nbd_sock = file_path('nbd-sock', base_dir=iotests.sock_dir)
> +disk = iotests.file_path('disk')
> +bitmap = 'e4f2eed0-37fe-4539-b50b-85d2e7fd235f'
> +nbd_opts = f'driver=nbd,server.type=unix,server.path={nbd_sock}' \
> +        f',x-dirty-bitmap=qemu:dirty-bitmap:{bitmap}'
> +
> +
> +iotests.unarchive_sample_image('parallels-with-bitmap', disk)
> +
> +iotests.unarchive_sample_image('parallels-with-bitmap', '/work/mega')
no-no-no, '/work/mega' is absolutely no way


> +
> +
> +with qemu_nbd_popen('--read-only', f'--socket={nbd_sock}',
> +                    f'--bitmap={bitmap}', '-f', iotests.imgfmt, disk):
> +    out = qemu_img_pipe('map', '--output=json', '--image-opts', nbd_opts)
> +    chunks = json.loads(out)
> +    cluster = 64 * 1024
> +
> +    log('dirty clusters (cluster size is 64K):')
> +    for c in chunks:
> +        assert c['start'] % cluster == 0
> +        assert c['length'] % cluster == 0
> +        if c['data']:
> +            continue
> +
> +        a = c['start'] // cluster
> +        b = (c['start'] + c['length']) // cluster
> +        if b - a > 1:
> +            log(f'{a}-{b-1}')
> +        else:
> +            log(a)
> diff --git a/tests/qemu-iotests/tests/parallels-read-bitmap.out b/tests/qemu-iotests/tests/parallels-read-bitmap.out
> new file mode 100644
> index 0000000000..e8f6bc9e96
> --- /dev/null
> +++ b/tests/qemu-iotests/tests/parallels-read-bitmap.out
> @@ -0,0 +1,6 @@
> +Start NBD server
> +dirty clusters (cluster size is 64K):
> +5-6
> +10-12
> +30
> +Kill NBD server

Re: [PATCH 5/5] iotests: add parallels-read-bitmap test

[Vladimir Sementsov-Ogievskiy]

19.02.2021 14:56, Denis V. Lunev wrote:
> On 2/16/21 7:45 PM, Vladimir Sementsov-Ogievskiy wrote:
>> Test support for reading bitmap from parallels image format.
>> parallels-with-bitmap.bz2 is generated on Virtuozzo by
>> parallels-with-bitmap.sh
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   .../sample_images/parallels-with-bitmap.bz2   | Bin 0 -> 203 bytes
>>   .../sample_images/parallels-with-bitmap.sh    |  33 ++++++++++
>>   .../qemu-iotests/tests/parallels-read-bitmap  |  57 ++++++++++++++++++
>>   .../tests/parallels-read-bitmap.out           |   6 ++
>>   4 files changed, 96 insertions(+)
>>   create mode 100644 tests/qemu-iotests/sample_images/parallels-with-bitmap.bz2
>>   create mode 100755 tests/qemu-iotests/sample_images/parallels-with-bitmap.sh
>>   create mode 100755 tests/qemu-iotests/tests/parallels-read-bitmap
>>   create mode 100644 tests/qemu-iotests/tests/parallels-read-bitmap.out
>>
>> diff --git a/tests/qemu-iotests/sample_images/parallels-with-bitmap.bz2 b/tests/qemu-iotests/sample_images/parallels-with-bitmap.bz2
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..54892fd4d01bf743d395bd4f3d896494146ab5a9
>> GIT binary patch
>> literal 203
>> zcmV;+05tzXT4*^jL0KkKS@=;0bpT+Hf7|^?Km<xfFyKQJ7=Y^F-%vt;00~Ysa6|-=
>> zk&7Szk`SoS002EkfMftPG<ipnsiCK}K_sNmm}me3FiZr%Oaf_u5F8kD;mB_~cxD-r
>> z5P$(X{&Tq5C`<xK02D?NNdN+t$~z$m00O|zFh^ynq*yaCtkn+NZzWom<#OEoF`?zb
>> zv(i3x^K~wt!aLPcRBP+PckUsIh6*LgjYSh0`}#7hMC9NR5D)+W0d&8Mxgwk>NPH-R
>> Fx`3oHQ9u9y
>>
>> literal 0
>> HcmV?d00001
>>
>> diff --git a/tests/qemu-iotests/sample_images/parallels-with-bitmap.sh b/tests/qemu-iotests/sample_images/parallels-with-bitmap.sh
>> new file mode 100755
>> index 0000000000..e4a1d71277
>> --- /dev/null
>> +++ b/tests/qemu-iotests/sample_images/parallels-with-bitmap.sh
>> @@ -0,0 +1,33 @@
> do we need Copyright notice here?

Will add

> I am unsure that this script is to be
> acceptable in QEMU repo. Anyway, it looks fine :)

Yes, it requires some proprietary tools, not available for most of developers..
On the other hand, I should document somehow, how did I create the image
(at least for future me). I can do it in a commit message, but I think better
is just add a script, it will not hurt.

> 
> 
>> +#!/bin/bash
>> +
>> +CT=parallels-with-bitmap-ct
>> +DIR=$PWD/parallels-with-bitmap-dir
>> +IMG=$DIR/root.hds
>> +XML=$DIR/DiskDescriptor.xml
>> +TARGET=parallels-with-bitmap.bz2
>> +
>> +rm -rf $DIR
>> +
>> +prlctl create $CT --vmtype ct
>> +prlctl set $CT --device-add hdd --image $DIR --recreate --size 2G
>> +
>> +# cleanup the image
>> +qemu-img create -f parallels $IMG 64G
>> +
>> +# create bitmap
>> +prlctl backup $CT
>> +
>> +prlctl set $CT --device-del hdd1
>> +prlctl destroy $CT
>> +
>> +dev=$(ploop mount $XML | sed -n 's/^Adding delta dev=\(\/dev\/ploop[0-9]\+\).*/\1/p')
>> +dd if=/dev/zero of=$dev bs=64K seek=5 count=2 oflag=direct
>> +dd if=/dev/zero of=$dev bs=64K seek=30 count=1 oflag=direct
>> +dd if=/dev/zero of=$dev bs=64K seek=10 count=3 oflag=direct
>> +ploop umount $XML  # bitmap name will be in the output
>> +
>> +bzip2 -z $IMG
>> +
>> +mv $IMG.bz2 $TARGET
>> +
>> +rm -rf $DIR
>> diff --git a/tests/qemu-iotests/tests/parallels-read-bitmap b/tests/qemu-iotests/tests/parallels-read-bitmap
>> new file mode 100755
>> index 0000000000..b50b79f509
>> --- /dev/null
>> +++ b/tests/qemu-iotests/tests/parallels-read-bitmap
>> @@ -0,0 +1,57 @@
>> +#!/usr/bin/env python3
>> +#
>> +# Test parallels load bitmap
>> +#
>> +# Copyright (c) 2021 Virtuozzo International GmbH.
>> +#
>> +# 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 json
>> +import iotests
>> +from iotests import qemu_nbd_popen, qemu_img_pipe, log, file_path
>> +
>> +iotests.script_initialize(supported_fmts=['parallels'])
>> +
>> +nbd_sock = file_path('nbd-sock', base_dir=iotests.sock_dir)
>> +disk = iotests.file_path('disk')
>> +bitmap = 'e4f2eed0-37fe-4539-b50b-85d2e7fd235f'
>> +nbd_opts = f'driver=nbd,server.type=unix,server.path={nbd_sock}' \
>> +        f',x-dirty-bitmap=qemu:dirty-bitmap:{bitmap}'
>> +
>> +
>> +iotests.unarchive_sample_image('parallels-with-bitmap', disk)
>> +
>> +iotests.unarchive_sample_image('parallels-with-bitmap', '/work/mega')
> no-no-no, '/work/mega' is absolutely no way
> 

Oops)

> 
>> +
>> +
>> +with qemu_nbd_popen('--read-only', f'--socket={nbd_sock}',
>> +                    f'--bitmap={bitmap}', '-f', iotests.imgfmt, disk):
>> +    out = qemu_img_pipe('map', '--output=json', '--image-opts', nbd_opts)
>> +    chunks = json.loads(out)
>> +    cluster = 64 * 1024
>> +
>> +    log('dirty clusters (cluster size is 64K):')
>> +    for c in chunks:
>> +        assert c['start'] % cluster == 0
>> +        assert c['length'] % cluster == 0
>> +        if c['data']:
>> +            continue
>> +
>> +        a = c['start'] // cluster
>> +        b = (c['start'] + c['length']) // cluster
>> +        if b - a > 1:
>> +            log(f'{a}-{b-1}')
>> +        else:
>> +            log(a)
>> diff --git a/tests/qemu-iotests/tests/parallels-read-bitmap.out b/tests/qemu-iotests/tests/parallels-read-bitmap.out
>> new file mode 100644
>> index 0000000000..e8f6bc9e96
>> --- /dev/null
>> +++ b/tests/qemu-iotests/tests/parallels-read-bitmap.out
>> @@ -0,0 +1,6 @@
>> +Start NBD server
>> +dirty clusters (cluster size is 64K):
>> +5-6
>> +10-12
>> +30
>> +Kill NBD server
> 


-- 
Best regards,
Vladimir