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

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

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

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