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 <vsement...@virtuozzo.com> > --- > 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
