[PATCH v2 0/6] parallels: load bitmap extension

[PATCH v2 0/6] parallels: load bitmap extension

[Vladimir Sementsov-Ogievskiy]

Hi all!

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

v2:
01: tweak commit message, add r-bs by Eric and Denis
02: tweak wording by Denis's suggestions
03: new, suggested by Denis
04: rebase on 03, add several error checking,
    convert l1 table to cpu from small-endian,
    allow loading several bitmaps
06: add copyright, drop "/work/mega"

Vladimir Sementsov-Ogievskiy (6):
  qcow2-bitmap: make bytes_covered_by_bitmap_cluster() public
  parallels.txt: fix bitmap L1 table description
  block/parallels: BDRVParallelsState: add cluster_size field
  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                    |  27 +-
 block/parallels.h                             |   7 +-
 include/block/dirty-bitmap.h                  |   2 +
 block/dirty-bitmap.c                          |  13 +
 block/parallels-ext.c                         | 300 ++++++++++++++++++
 block/parallels.c                             |  26 +-
 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    |  51 +++
 .../qemu-iotests/tests/parallels-read-bitmap  |  55 ++++
 .../tests/parallels-read-bitmap.out           |   6 +
 13 files changed, 484 insertions(+), 32 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 v2 1/6] 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 share it with bitmap loading in
parallels format.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
---
 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

[PATCH v2 2/6] 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 | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/docs/interop/parallels.txt b/docs/interop/parallels.txt
index f15bf35bd1..73af9a2c4b 100644
--- a/docs/interop/parallels.txt
+++ b/docs/interop/parallels.txt
@@ -208,21 +208,24 @@ of its data area are:
   28 - 31:    l1_size
               The number of entries in the L1 table of the bitmap.
 
-  variable:   l1_table (8 * l1_size bytes)
-              L1 offset table (in bytes)
+  variable:   L1 offset table (l1_table), size: 8 * l1_size bytes
 
-A dirty bitmap is stored using a one-level structure for the mapping to host
-clusters - an L1 table.
+Dirty bitmap is stored in the array of clusters inside Parallels Image file.
+Offsets of these clusters are saved in L1 offset table here. 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.
+If L1 table entry is 0, all bits in the corresponding cluster of the bitmap
+are assumed to be 0.
 
-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)
-- 
2.29.2

[PATCH v2 3/6] block/parallels: BDRVParallelsState: add cluster_size field

[Vladimir Sementsov-Ogievskiy]

We are going to use it in more places, calculating
"s->tracks << BDRV_SECTOR_BITS" doesn't look good.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/parallels.h | 1 +
 block/parallels.c | 8 ++++----
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/parallels.h b/block/parallels.h
index 5aa101cfc8..9a9209e320 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -79,6 +79,7 @@ typedef struct BDRVParallelsState {
     ParallelsPreallocMode prealloc_mode;
 
     unsigned int tracks;
+    unsigned int cluster_size;
 
     unsigned int off_multiplier;
     Error *migration_blocker;
diff --git a/block/parallels.c b/block/parallels.c
index 3c22dfdc9d..9594d84978 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -421,7 +421,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
     int ret;
     uint32_t i;
     bool flush_bat = false;
-    int cluster_size = s->tracks << BDRV_SECTOR_BITS;
 
     size = bdrv_getlength(bs->file->bs);
     if (size < 0) {
@@ -472,7 +471,7 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
             high_off = off;
         }
 
-        if (prev_off != 0 && (prev_off + cluster_size) != off) {
+        if (prev_off != 0 && (prev_off + s->cluster_size) != off) {
             res->bfi.fragmented_clusters++;
         }
         prev_off = off;
@@ -487,10 +486,10 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
         }
     }
 
-    res->image_end_offset = high_off + cluster_size;
+    res->image_end_offset = high_off + s->cluster_size;
     if (size > res->image_end_offset) {
         int64_t count;
-        count = DIV_ROUND_UP(size - res->image_end_offset, cluster_size);
+        count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
         fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n",
                 fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR",
                 size - res->image_end_offset);
@@ -771,6 +770,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         ret = -EFBIG;
         goto fail;
     }
+    s->cluster_size = s->tracks << BDRV_SECTOR_BITS;
 
     s->bat_size = le32_to_cpu(ph.bat_entries);
     if (s->bat_size > INT_MAX / sizeof(uint32_t)) {
-- 
2.29.2

[PATCH v2 4/6] 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 | 300 ++++++++++++++++++++++++++++++++++++++++++
 block/parallels.c     |  18 +++
 block/meson.build     |   3 +-
 4 files changed, 325 insertions(+), 2 deletions(-)
 create mode 100644 block/parallels-ext.c

diff --git a/block/parallels.h b/block/parallels.h
index 9a9209e320..f22f43f988 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 {
@@ -85,4 +86,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..e0dd0975c6
--- /dev/null
+++ b/block/parallels-ext.c
@@ -0,0 +1,300 @@
+/*
+ * Support of Parallels Format Extension. 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;
+    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),
+                     s->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, s->cluster_size);
+    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 = 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,
+                             s->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];
+    int i;
+
+    if (data_size < sizeof(bf)) {
+        error_setg(errp, "Too small Bitmap Feature area in Parallels Format "
+                   "Extension: %zu bytes, expected at least %zu bytes",
+                   data_size, sizeof(bf));
+        return NULL;
+    }
+    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_new(uint64_t, bf.l1_size);
+    for (i = 0; i < bf.l1_size; i++, data += sizeof(uint64_t)) {
+        l1_table[i] = ldq_le_p(data);
+    }
+
+    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->cluster_size;
+    uint8_t *pos = ext_cluster;
+    ParallelsFormatExtensionHeader eh;
+    g_autofree uint8_t *hash = NULL;
+    size_t hash_len = 0;
+    GSList *bitmaps = NULL, *el;
+
+    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;
+        BdrvDirtyBitmap *bitmap;
+
+        if (remaining < sizeof(fh)) {
+            error_setg(errp, "Can not read feature header, as remaining bytes "
+                       "(%d) in Format Extension is less than Feature header "
+                       "size (%zu)", remaining, sizeof(fh));
+            goto fail;
+        }
+
+        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:
+            bitmap = parallels_load_bitmap(bs, pos, fh.data_size, errp);
+            if (!bitmap) {
+                goto fail;
+            }
+            bitmaps = g_slist_append(bitmaps, bitmap);
+            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:
+    for (el = bitmaps; el; el = el->next) {
+        bdrv_release_dirty_bitmap(el->data);
+    }
+    g_slist_free(bitmaps);
+
+    return -EINVAL;
+}
+
+int parallels_read_format_extension(BlockDriverState *bs,
+                                    int64_t ext_off, Error **errp)
+{
+    BDRVParallelsState *s = bs->opaque;
+    int ret;
+    uint8_t *ext_cluster = qemu_blockalign(bs, s->cluster_size);
+
+    assert(ext_off > 0);
+
+    ret = bdrv_pread(bs->file, ext_off, ext_cluster, s->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 9594d84978..6ebad2a2bb 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

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

[Vladimir Sementsov-Ogievskiy]

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 4e758308f2..90d0b62523 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 logging
 import os
 import re
+import shutil
 import signal
 import struct
 import subprocess
@@ -96,6 +98,14 @@
                              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 v2 6/6] 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    |  51 ++++++++++++++++
 .../qemu-iotests/tests/parallels-read-bitmap  |  55 ++++++++++++++++++
 .../tests/parallels-read-bitmap.out           |   6 ++
 4 files changed, 112 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..30615aa6bd
--- /dev/null
+++ b/tests/qemu-iotests/sample_images/parallels-with-bitmap.sh
@@ -0,0 +1,51 @@
+#!/bin/bash
+#
+# 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/>.
+#
+
+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..af6b9c5db3
--- /dev/null
+++ b/tests/qemu-iotests/tests/parallels-read-bitmap
@@ -0,0 +1,55 @@
+#!/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)
+
+
+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 v2 5/6] iotests.py: add unarchive_sample_image() helper

[Denis V. Lunev]

On 2/24/21 1:47 PM, Vladimir Sementsov-Ogievskiy wrote:
> 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 4e758308f2..90d0b62523 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 logging
>  import os
>  import re
> +import shutil
>  import signal
>  import struct
>  import subprocess
> @@ -96,6 +98,14 @@
>                               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]:
Reviewed-by: Denis V. Lunev <den@openvz.org>

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

[Denis V. Lunev]

On 2/24/21 1:47 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 | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/docs/interop/parallels.txt b/docs/interop/parallels.txt
> index f15bf35bd1..73af9a2c4b 100644
> --- a/docs/interop/parallels.txt
> +++ b/docs/interop/parallels.txt
> @@ -208,21 +208,24 @@ of its data area are:
>    28 - 31:    l1_size
>                The number of entries in the L1 table of the bitmap.
>  
> -  variable:   l1_table (8 * l1_size bytes)
> -              L1 offset table (in bytes)
> +  variable:   L1 offset table (l1_table), size: 8 * l1_size bytes
>  
> -A dirty bitmap is stored using a one-level structure for the mapping to host
> -clusters - an L1 table.
> +Dirty bitmap is stored in the array of clusters inside Parallels Image file.
> +Offsets of these clusters are saved in L1 offset table here. 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.
> +If L1 table entry is 0, all bits in the corresponding cluster of the bitmap
> +are assumed to be 0.
>  
> -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)
Reviewed-by: Denis V. Lunev <den@openvz.org>

Re: [PATCH v2 3/6] block/parallels: BDRVParallelsState: add cluster_size field

[Denis V. Lunev]

On 2/24/21 1:47 PM, Vladimir Sementsov-Ogievskiy wrote:
> We are going to use it in more places, calculating
> "s->tracks << BDRV_SECTOR_BITS" doesn't look good.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/parallels.h | 1 +
>  block/parallels.c | 8 ++++----
>  2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/block/parallels.h b/block/parallels.h
> index 5aa101cfc8..9a9209e320 100644
> --- a/block/parallels.h
> +++ b/block/parallels.h
> @@ -79,6 +79,7 @@ typedef struct BDRVParallelsState {
>      ParallelsPreallocMode prealloc_mode;
>  
>      unsigned int tracks;
> +    unsigned int cluster_size;
>  
>      unsigned int off_multiplier;
>      Error *migration_blocker;
> diff --git a/block/parallels.c b/block/parallels.c
> index 3c22dfdc9d..9594d84978 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -421,7 +421,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>      int ret;
>      uint32_t i;
>      bool flush_bat = false;
> -    int cluster_size = s->tracks << BDRV_SECTOR_BITS;
>  
>      size = bdrv_getlength(bs->file->bs);
>      if (size < 0) {
> @@ -472,7 +471,7 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>              high_off = off;
>          }
>  
> -        if (prev_off != 0 && (prev_off + cluster_size) != off) {
> +        if (prev_off != 0 && (prev_off + s->cluster_size) != off) {
>              res->bfi.fragmented_clusters++;
>          }
>          prev_off = off;
> @@ -487,10 +486,10 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>          }
>      }
>  
> -    res->image_end_offset = high_off + cluster_size;
> +    res->image_end_offset = high_off + s->cluster_size;
>      if (size > res->image_end_offset) {
>          int64_t count;
> -        count = DIV_ROUND_UP(size - res->image_end_offset, cluster_size);
> +        count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
>          fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n",
>                  fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR",
>                  size - res->image_end_offset);
> @@ -771,6 +770,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>          ret = -EFBIG;
>          goto fail;
>      }
> +    s->cluster_size = s->tracks << BDRV_SECTOR_BITS;
>  
>      s->bat_size = le32_to_cpu(ph.bat_entries);
>      if (s->bat_size > INT_MAX / sizeof(uint32_t)) {
Reviewed-by: Denis V. Lunev <den@openvz.org>

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

[Denis V. Lunev]

On 2/24/21 1:47 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    |  51 ++++++++++++++++
>  .../qemu-iotests/tests/parallels-read-bitmap  |  55 ++++++++++++++++++
>  .../tests/parallels-read-bitmap.out           |   6 ++
>  4 files changed, 112 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..30615aa6bd
> --- /dev/null
> +++ b/tests/qemu-iotests/sample_images/parallels-with-bitmap.sh
> @@ -0,0 +1,51 @@
> +#!/bin/bash
> +#
> +# 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/>.
> +#
> +
> +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..af6b9c5db3
> --- /dev/null
> +++ b/tests/qemu-iotests/tests/parallels-read-bitmap
> @@ -0,0 +1,55 @@
> +#!/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)
> +
> +
> +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
Reviewed-by: Denis V. Lunev <den@openvz.org>

Re: [PATCH v2 4/6] parallels: support bitmap extension for read-only mode

[Denis V. Lunev]

On 2/24/21 1:47 PM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/parallels.h     |   6 +-
>  block/parallels-ext.c | 300 ++++++++++++++++++++++++++++++++++++++++++
>  block/parallels.c     |  18 +++
>  block/meson.build     |   3 +-
>  4 files changed, 325 insertions(+), 2 deletions(-)
>  create mode 100644 block/parallels-ext.c
>
> diff --git a/block/parallels.h b/block/parallels.h
> index 9a9209e320..f22f43f988 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 {
> @@ -85,4 +86,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..e0dd0975c6
> --- /dev/null
> +++ b/block/parallels-ext.c
> @@ -0,0 +1,300 @@
> +/*
> + * Support of Parallels Format Extension. 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;
> +    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),
> +                     s->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, s->cluster_size);
> +    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 = 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,
> +                             s->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];
> +    int i;
> +
> +    if (data_size < sizeof(bf)) {
> +        error_setg(errp, "Too small Bitmap Feature area in Parallels Format "
> +                   "Extension: %zu bytes, expected at least %zu bytes",
> +                   data_size, sizeof(bf));
> +        return NULL;
> +    }
> +    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_new(uint64_t, bf.l1_size);
> +    for (i = 0; i < bf.l1_size; i++, data += sizeof(uint64_t)) {
> +        l1_table[i] = ldq_le_p(data);
> +    }
> +
> +    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->cluster_size;
> +    uint8_t *pos = ext_cluster;
> +    ParallelsFormatExtensionHeader eh;
> +    g_autofree uint8_t *hash = NULL;
> +    size_t hash_len = 0;
> +    GSList *bitmaps = NULL, *el;
> +
> +    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;
> +        BdrvDirtyBitmap *bitmap;
> +
> +        if (remaining < sizeof(fh)) {
> +            error_setg(errp, "Can not read feature header, as remaining bytes "
> +                       "(%d) in Format Extension is less than Feature header "
> +                       "size (%zu)", remaining, sizeof(fh));
> +            goto fail;
> +        }
> +
> +        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:
> +            bitmap = parallels_load_bitmap(bs, pos, fh.data_size, errp);
> +            if (!bitmap) {
> +                goto fail;
> +            }
> +            bitmaps = g_slist_append(bitmaps, bitmap);
> +            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:
> +    for (el = bitmaps; el; el = el->next) {
> +        bdrv_release_dirty_bitmap(el->data);
> +    }
> +    g_slist_free(bitmaps);
> +
> +    return -EINVAL;
> +}
> +
> +int parallels_read_format_extension(BlockDriverState *bs,
> +                                    int64_t ext_off, Error **errp)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    int ret;
> +    uint8_t *ext_cluster = qemu_blockalign(bs, s->cluster_size);
> +
> +    assert(ext_off > 0);
> +
> +    ret = bdrv_pread(bs->file, ext_off, ext_cluster, s->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 9594d84978..6ebad2a2bb 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'))
Reviewed-by: Denis V. Lunev <den@openvz.org>

[PATCH v2 7/6] MAINTAINERS: update parallels block driver

[Vladimir Sementsov-Ogievskiy]

Add new parallels-ext.c and myself as co-maintainer.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 9b2aa18e1f..92ba1fce5e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3117,9 +3117,11 @@ F: block/dmg.c
 parallels
 M: Stefan Hajnoczi <stefanha@redhat.com>
 M: Denis V. Lunev <den@openvz.org>
+M: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
 L: qemu-block@nongnu.org
 S: Supported
 F: block/parallels.c
+F: block/parallels-ext.c
 F: docs/interop/parallels.txt
 
 qed
-- 
2.29.2

Re: [PATCH v2 7/6] MAINTAINERS: update parallels block driver

[Denis V. Lunev]

On 3/4/21 12:51 PM, Vladimir Sementsov-Ogievskiy wrote:
> Add new parallels-ext.c and myself as co-maintainer.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  MAINTAINERS | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9b2aa18e1f..92ba1fce5e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3117,9 +3117,11 @@ F: block/dmg.c
>  parallels
>  M: Stefan Hajnoczi <stefanha@redhat.com>
>  M: Denis V. Lunev <den@openvz.org>
> +M: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>  L: qemu-block@nongnu.org
>  S: Supported
>  F: block/parallels.c
> +F: block/parallels-ext.c
>  F: docs/interop/parallels.txt
>  
>  qed
Reviewed-by: Denis V. Lunev <den@openvz.org>

Re: [PATCH v2 7/6] MAINTAINERS: update parallels block driver

[Vladimir Sementsov-Ogievskiy]

04.03.2021 12:51, Vladimir Sementsov-Ogievskiy wrote:
> Add new parallels-ext.c and myself as co-maintainer.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   MAINTAINERS | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9b2aa18e1f..92ba1fce5e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3117,9 +3117,11 @@ F: block/dmg.c
>   parallels
>   M: Stefan Hajnoczi <stefanha@redhat.com>
>   M: Denis V. Lunev <den@openvz.org>
> +M: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>   L: qemu-block@nongnu.org
>   S: Supported
>   F: block/parallels.c
> +F: block/parallels-ext.c
>   F: docs/interop/parallels.txt
>   
>   qed
> 


squash-in:

diff --git a/MAINTAINERS b/MAINTAINERS
index 92ba1fce5e..6d6480e1b0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3123,6 +3123,7 @@ S: Supported
  F: block/parallels.c
  F: block/parallels-ext.c
  F: docs/interop/parallels.txt
+T: git https://src.openvz.org/scm/~vsementsov/qemu.git parallels
  
  qed
  M: Stefan Hajnoczi <stefanha@redhat.com>



-- 
Best regards,
Vladimir

Re: [PATCH v2 7/6] MAINTAINERS: update parallels block driver

[Denis V. Lunev]

On 3/4/21 12:58 PM, Vladimir Sementsov-Ogievskiy wrote:
> 04.03.2021 12:51, Vladimir Sementsov-Ogievskiy wrote:
>> Add new parallels-ext.c and myself as co-maintainer.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   MAINTAINERS | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 9b2aa18e1f..92ba1fce5e 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -3117,9 +3117,11 @@ F: block/dmg.c
>>   parallels
>>   M: Stefan Hajnoczi <stefanha@redhat.com>
>>   M: Denis V. Lunev <den@openvz.org>
>> +M: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>   L: qemu-block@nongnu.org
>>   S: Supported
>>   F: block/parallels.c
>> +F: block/parallels-ext.c
>>   F: docs/interop/parallels.txt
>>     qed
>>
>
>
> squash-in:
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 92ba1fce5e..6d6480e1b0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3123,6 +3123,7 @@ S: Supported
>  F: block/parallels.c
>  F: block/parallels-ext.c
>  F: docs/interop/parallels.txt
> +T: git https://src.openvz.org/scm/~vsementsov/qemu.git parallels
>  
>  qed
>  M: Stefan Hajnoczi <stefanha@redhat.com>
>
>
>
Reviewed-by: Denis V. Lunev <den@openvz.org>

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

[Kevin Wolf]

Am 24.02.2021 um 11:47 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 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 | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/docs/interop/parallels.txt b/docs/interop/parallels.txt
> index f15bf35bd1..73af9a2c4b 100644
> --- a/docs/interop/parallels.txt
> +++ b/docs/interop/parallels.txt
> @@ -208,21 +208,24 @@ of its data area are:
>    28 - 31:    l1_size
>                The number of entries in the L1 table of the bitmap.
>  
> -  variable:   l1_table (8 * l1_size bytes)
> -              L1 offset table (in bytes)
> +  variable:   L1 offset table (l1_table), size: 8 * l1_size bytes
>  
> -A dirty bitmap is stored using a one-level structure for the mapping to host
> -clusters - an L1 table.
> +Dirty bitmap is stored in the array of clusters inside Parallels Image file.
> +Offsets of these clusters are saved in L1 offset table here. Each L1 table
> +entry is a 64bit integer described below:

I think the English grammar needs some fixes here (missing articles).

If I understand correctly, it's also not really an array, which I would
understand as consecutive clusters.

Maybe something like this:

  The dirty bitmap described by this feature extension is stored in a set
  of clusters inside the Parallels image file. The offsets of these
  clusters are saved in the L1 offset table specified by the feature
  extension. Each L1 table entry is a 64 bit integer as 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.
> +If L1 table entry is 0, all bits in the corresponding cluster of the bitmap
> +are assumed to be 0.

"an L1 table", like before.

> -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.

Same here.

> -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

"the 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)

These changes can be made while applying the patch.

Kevin

Re: [PATCH v2 3/6] block/parallels: BDRVParallelsState: add cluster_size field

[Kevin Wolf]

Am 24.02.2021 um 11:47 hat Vladimir Sementsov-Ogievskiy geschrieben:
> We are going to use it in more places, calculating
> "s->tracks << BDRV_SECTOR_BITS" doesn't look good.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> @@ -771,6 +770,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>          ret = -EFBIG;
>          goto fail;
>      }
> +    s->cluster_size = s->tracks << BDRV_SECTOR_BITS;
>  
>      s->bat_size = le32_to_cpu(ph.bat_entries);
>      if (s->bat_size > INT_MAX / sizeof(uint32_t)) {

Checking the context, I saw this a few lines above:

    if (s->tracks > INT32_MAX/513) {

Is the 513 intentional?

Kevin

Re: [PATCH v2 0/6] parallels: load bitmap extension

[Kevin Wolf]

Am 24.02.2021 um 11:47 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Hi all!
> 
> We need to load bitmaps from parallels image in our product.
> So here is a feature.

Thanks, applied to the block branch.

I changed some sentences in patch 2 as suggested in my reply to it.
Please let me know if you're happy with these changes or if you would
like to tweak them a bit more.

Kevin

Re: [PATCH v2 3/6] block/parallels: BDRVParallelsState: add cluster_size field

[Denis V. Lunev]

On 3/4/21 5:24 PM, Kevin Wolf wrote:
> Am 24.02.2021 um 11:47 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> We are going to use it in more places, calculating
>> "s->tracks << BDRV_SECTOR_BITS" doesn't look good.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> @@ -771,6 +770,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>          ret = -EFBIG;
>>          goto fail;
>>      }
>> +    s->cluster_size = s->tracks << BDRV_SECTOR_BITS;
>>  
>>      s->bat_size = le32_to_cpu(ph.bat_entries);
>>      if (s->bat_size > INT_MAX / sizeof(uint32_t)) {
> Checking the context, I saw this a few lines above:
>
>     if (s->tracks > INT32_MAX/513) {
>
> Is the 513 intentional?
>
> Kevin
>
I can not remember why I have written this at that time,
but original comment for the commit was

commit d25d59802021a747812472780d80a0e792078f40
Author: Denis V. Lunev <den@openvz.org>
Date:   Mon Jul 28 20:23:55 2014 +0400

    parallels: 2TB+ parallels images support
   
    Parallels has released in the recent updates of Parallels Server 5/6
    new addition to his image format. Images with signature WithouFreSpacExt
    have offsets in the catalog coded not as offsets in sectors (multiple
    of 512 bytes) but offsets coded in blocks (i.e. header->tracks * 512)
   
    In this case all 64 bits of header->nb_sectors are used for image size.
   
    This patch implements support of this for qemu-img and also adds
specific
    check for an incorrect image. Images with block size greater than
    INT_MAX/513 are not supported. The biggest available Parallels image
    cluster size in the field is 1 Mb. Thus this limit will not hurt
    anyone.
   
    Signed-off-by: Denis V. Lunev <den@openvz.org>
    CC: Jeff Cody <jcody@redhat.com>
    CC: Kevin Wolf <kwolf@redhat.com>
    CC: Stefan Hajnoczi <stefanha@redhat.com>
    Reviewed-by: Jeff Cody <jcody@redhat.com>
    Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Thus I believe that this is intentional.

Den

Re: [PATCH v2 3/6] block/parallels: BDRVParallelsState: add cluster_size field

[Kevin Wolf]

Am 04.03.2021 um 15:57 hat Denis V. Lunev geschrieben:
> On 3/4/21 5:24 PM, Kevin Wolf wrote:
> > Am 24.02.2021 um 11:47 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >> We are going to use it in more places, calculating
> >> "s->tracks << BDRV_SECTOR_BITS" doesn't look good.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >> @@ -771,6 +770,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
> >>          ret = -EFBIG;
> >>          goto fail;
> >>      }
> >> +    s->cluster_size = s->tracks << BDRV_SECTOR_BITS;
> >>  
> >>      s->bat_size = le32_to_cpu(ph.bat_entries);
> >>      if (s->bat_size > INT_MAX / sizeof(uint32_t)) {
> > Checking the context, I saw this a few lines above:
> >
> >     if (s->tracks > INT32_MAX/513) {
> >
> > Is the 513 intentional?
> >
> > Kevin
> >
> I can not remember why I have written this at that time,
> but original comment for the commit was
> 
> commit d25d59802021a747812472780d80a0e792078f40
> Author: Denis V. Lunev <den@openvz.org>
> Date:   Mon Jul 28 20:23:55 2014 +0400
> 
>     parallels: 2TB+ parallels images support
>    
>     Parallels has released in the recent updates of Parallels Server 5/6
>     new addition to his image format. Images with signature WithouFreSpacExt
>     have offsets in the catalog coded not as offsets in sectors (multiple
>     of 512 bytes) but offsets coded in blocks (i.e. header->tracks * 512)
>    
>     In this case all 64 bits of header->nb_sectors are used for image size.
>    
>     This patch implements support of this for qemu-img and also adds
> specific
>     check for an incorrect image. Images with block size greater than
>     INT_MAX/513 are not supported. The biggest available Parallels image
>     cluster size in the field is 1 Mb. Thus this limit will not hurt
>     anyone.
>    
>     Signed-off-by: Denis V. Lunev <den@openvz.org>
>     CC: Jeff Cody <jcody@redhat.com>
>     CC: Kevin Wolf <kwolf@redhat.com>
>     CC: Stefan Hajnoczi <stefanha@redhat.com>
>     Reviewed-by: Jeff Cody <jcody@redhat.com>
>     Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Thus I believe that this is intentional.

Hm, fair. It's a weird number. I would have guessed a typo, but if it's
in the commit message as well, it might be intentional. Or just a typo
combined with copy & paste.

If we ever remember or find a new reason why it has to be 513 rather
than 512, adding a comment would be nice.

Kevin