Re: [Qemu-devel] [PATCH 05/29] qcow2-bitmap: structs and consts

[Kevin Wolf]

Am 08.08.2016 um 17:04 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Create block/qcow2-bitmap.c
> Add data structures and constraints accordingly to docs/specs/qcow2.txt
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  block/Makefile.objs  |  2 +-
>  block/qcow2-bitmap.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  block/qcow2.h        | 29 +++++++++++++++++++++++++++++
>  3 files changed, 77 insertions(+), 1 deletion(-)
>  create mode 100644 block/qcow2-bitmap.c
> 
> diff --git a/block/Makefile.objs b/block/Makefile.objs
> index 2593a2f..545d618 100644
> --- a/block/Makefile.objs
> +++ b/block/Makefile.objs
> @@ -1,5 +1,5 @@
>  block-obj-y += raw_bsd.o qcow.o vdi.o vmdk.o cloop.o bochs.o vpc.o vvfat.o
> -block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o 
> qcow2-cache.o
> +block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o 
> qcow2-cache.o qcow2-bitmap.o
>  block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
>  block-obj-y += qed-check.o
>  block-obj-$(CONFIG_VHDX) += vhdx.o vhdx-endian.o vhdx-log.o
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> new file mode 100644
> index 0000000..cd18b07
> --- /dev/null
> +++ b/block/qcow2-bitmap.c
> @@ -0,0 +1,47 @@
> +/*
> + * Bitmaps for the QCOW version 2 format
> + *
> + * Copyright (c) 2014-2016 Vladimir Sementsov-Ogievskiy
> + *
> + * This file is derived from qcow2-snapshot.c, original copyright:
> + * Copyright (c) 2004-2006 Fabrice Bellard
> + *
> + * 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.
> + */
> +
> +/* NOTICE: BME here means Bitmaps Extension and used as a namespace for
> + * _internal_ constants. Please do not use this _internal_ abbreviation for
> + * other needs and/or outside of this file. */
> +
> +/* Bitmap directory entry constraints */
> +#define BME_MAX_TABLE_SIZE 0x8000000
> +#define BME_MAX_PHYS_SIZE 0x20000000 /* 512 mb */
> +#define BME_MAX_GRANULARITY_BITS 31
> +#define BME_MIN_GRANULARITY_BITS 9

The spec accepts 0. Should we change the spec or at least add a note
that qemu requires a minimum of 512 bytes?

> +#define BME_MAX_NAME_SIZE 1023
> +
> +/* Bitmap directory entry flags */
> +#define BME_RESERVED_FLAGS 0xffffffff
> +
> +/* bits [1, 8] U [56, 63] are reserved */
> +#define BME_TABLE_ENTRY_RESERVED_MASK 0xff000000000001fe
> +
> +typedef enum BitmapType {
> +    BT_DIRTY_TRACKING_BITMAP = 1
> +} BitmapType;
> diff --git a/block/qcow2.h b/block/qcow2.h
> index b36a7bf..b12cecc 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -52,6 +52,10 @@
>   * space for snapshot names and IDs */
>  #define QCOW_MAX_SNAPSHOTS_SIZE (1024 * QCOW_MAX_SNAPSHOTS)
>  
> +/* Bitmap header extension constraints */
> +#define QCOW_MAX_DIRTY_BITMAPS 65535
> +#define QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE (1024 * QCOW_MAX_DIRTY_BITMAPS)
> +
>  /* indicate that the refcount of the referenced cluster is exactly one. */
>  #define QCOW_OFLAG_COPIED     (1ULL << 63)
>  /* indicate that the cluster is compressed (they never have the copied flag) 
> */
> @@ -142,6 +146,22 @@ typedef struct QEMU_PACKED QCowSnapshotHeader {
>      /* name follows  */
>  } QCowSnapshotHeader;
>  
> +/* QCow2BitmapHeader is actually a bitmap directory entry */
> +typedef struct QEMU_PACKED QCow2BitmapHeader {

Why not call it Qcow2BitmapDirEntry then?

> +    /* header is 8 byte aligned */
> +    uint64_t bitmap_table_offset;
> +
> +    uint32_t bitmap_table_size;
> +    uint32_t flags;
> +
> +    uint8_t type;
> +    uint8_t granularity_bits;
> +    uint16_t name_size;
> +    uint32_t extra_data_size;
> +    /* extra data follows  */
> +    /* name follows  */
> +} QCow2BitmapHeader;
> +
>  typedef struct QEMU_PACKED QCowSnapshotExtraData {
>      uint64_t vm_state_size_large;
>      uint64_t disk_size;
> @@ -222,6 +242,15 @@ typedef uint64_t Qcow2GetRefcountFunc(const void 
> *refcount_array,
>  typedef void Qcow2SetRefcountFunc(void *refcount_array,
>                                    uint64_t index, uint64_t value);
>  
> +/* Be careful, Qcow2BitmapHeaderExt is not an extension of 
> QCow2BitmapHeader, it
> + * is Qcow2 header extension */

Misunderstanding already avoided with Qcow2BitmapDirEntry. :-)

> +typedef struct Qcow2BitmapHeaderExt {
> +    uint32_t nb_bitmaps;
> +    uint32_t reserved32;
> +    uint64_t bitmap_directory_size;
> +    uint64_t bitmap_directory_offset;
> +} QEMU_PACKED Qcow2BitmapHeaderExt;

Looks good otherwise.

Kevin