Re: [Qemu-block] [PATCH 09/21] qcow2: add .bdrv_load_autoloading_dirty_bitmaps

[Max Reitz]

On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
> Auto loading bitmaps are bitmaps in Qcow2, with the AUTO flag set. They
> are loaded when the image is opened and become BdrvDirtyBitmaps for the
> corresponding drive.
> 
> Extra data in bitmaps is not supported for now.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  block/Makefile.objs  |   2 +-
>  block/qcow2-bitmap.c | 663 
> +++++++++++++++++++++++++++++++++++++++++++++++++++
>  block/qcow2.c        |   2 +
>  block/qcow2.h        |   3 +
>  4 files changed, 669 insertions(+), 1 deletion(-)
>  create mode 100644 block/qcow2-bitmap.c
> 

[...]

> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> new file mode 100644
> index 0000000..0f797e6
> --- /dev/null
> +++ b/block/qcow2-bitmap.c
> @@ -0,0 +1,663 @@

[...]

> +/* Check table entry specification constraints. If cluster_size is 0, offset
> + * alignment is not checked. */
> +static int check_table_entry(uint64_t entry, int cluster_size)
> +{
> +    uint64_t offset;
> +
> +    if (entry & BME_TABLE_ENTRY_RESERVED_MASK) {
> +        return -EINVAL;
> +    }
> +
> +    offset = entry & BME_TABLE_ENTRY_OFFSET_MASK;
> +    if (offset != 0) {
> +        /* if offset specified, bit 0 must is reserved */

-must

> +        if (entry & 1) {
> +            return -EINVAL;
> +        }
> +
> +        if ((cluster_size != 0) && (entry % cluster_size != 0)) {

Why would cluster_size be 0? Also, shouldn't it be offset instead of entry?

> +            return -EINVAL;
> +        }
> +    }
> +
> +    return 0;
> +}

[...]

> +/* bitmap table entries must satisfy specification constraints */
> +static int load_bitmap_data(BlockDriverState *bs,
> +                            const uint64_t *bitmap_table,
> +                            uint32_t bitmap_table_size,
> +                            BdrvDirtyBitmap *bitmap)
> +{
> +    int ret = 0;
> +    BDRVQcow2State *s = bs->opaque;
> +    uint64_t sector, dsc;
> +    uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
> +    uint8_t *buf = NULL;
> +    uint64_t i, tab_size =
> +            size_to_clusters(s,
> +                bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));
> +
> +    if (tab_size != bitmap_table_size ||
> +            tab_size > BME_MAX_TABLE_SIZE) {

This line should be aligned to the opening parenthesis.

[...]

> +static int check_dir_entry(BlockDriverState *bs, Qcow2BitmapDirEntry *entry)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +    uint64_t phys_bitmap_bytes =
> +            (uint64_t)entry->bitmap_table_size * s->cluster_size;
> +    uint64_t max_virtual_bits =
> +            (phys_bitmap_bytes * 8) << entry->granularity_bits;

I think this shift can overflow, even if bitmap_table_size, cluster_size
and granularity_bits are all within their respective limits.

> +    int64_t nb_sectors = bdrv_nb_sectors(bs);

Just using bdrv_getlength() would probably be simpler.

> +
> +    if (nb_sectors < 0) {
> +        return nb_sectors;
> +    }
> +
> +    int fail =

I'd prefer a boolean.

> +            (entry->bitmap_table_size == 0) ||
> +            (entry->bitmap_table_offset == 0) ||
> +            (entry->bitmap_table_offset % s->cluster_size) ||
> +            (entry->bitmap_table_size > BME_MAX_TABLE_SIZE) ||
> +            (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) ||
> +            (entry->bitmap_table_offset != 0 &&
> +                (nb_sectors << BDRV_SECTOR_BITS) > max_virtual_bits) ||
> +            (entry->granularity_bits > BME_MAX_GRANULARITY_BITS) ||
> +            (entry->granularity_bits < BME_MIN_GRANULARITY_BITS) ||
> +            (entry->flags & BME_RESERVED_FLAGS) ||
> +            (entry->name_size > BME_MAX_NAME_SIZE) ||
> +            (entry->type != BT_DIRTY_TRACKING_BITMAP);
> +
> +    return fail ? -EINVAL : 0;
> +}

[...]

> +/* bitmap_list_load
> + * Get bitmap list from qcow2 image. Actually reads bitmap directory,
> + * checks it and convert to bitmap list.
> + */
> +static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, uint64_t 
> offset,
> +                                         uint64_t size, Error **errp)
> +{

[...]

> +    bm_list = bitmap_list_new();
> +    for (e = (Qcow2BitmapDirEntry *)dir;
> +             e < (Qcow2BitmapDirEntry *)dir_end; e = next_dir_entry(e)) {

This line should be aligned to the opening parenthesis.

[...]

> +
> +/* bitmap_list_store
> + * Store bitmap list to qcow2 image as a bitmap directory.
> + * Everything is checked.
> + */
> +static int bitmap_list_store(BlockDriverState *bs, Qcow2BitmapList *bm_list,
> +                             uint64_t *offset, uint64_t *size, bool in_place)
> +{

[...]

> +fail:
> +    g_free(dir);
> +
> +    if (dir_offset > 0) {
> +        qcow2_free_clusters(bs, dir_offset, dir_size, QCOW2_DISCARD_OTHER);

I think this should only be freed if in_place is false.

Max

> +    }
> +
> +    return ret;
> +}

signature.asc
Description: OpenPGP digital signature