On 25.02.2017 18:07, Vladimir Sementsov-Ogievskiy wrote:
> Store persistent dirty bitmaps in qcow2 image.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
> ---
> block.c | 6 +-
> block/qcow2-bitmap.c | 473
> +++++++++++++++++++++++++++++++++++++++++++++++++++
> block/qcow2.c | 9 +
> block/qcow2.h | 1 +
> 4 files changed, 486 insertions(+), 3 deletions(-)
>
> diff --git a/block.c b/block.c
> index a0346c80c6..16cf522219 100644
> --- a/block.c
> +++ b/block.c
> @@ -2322,9 +2322,6 @@ static void bdrv_close(BlockDriverState *bs)
> bdrv_flush(bs);
> bdrv_drain(bs); /* in case flush left pending I/O */
>
> - bdrv_release_named_dirty_bitmaps(bs);
> - assert(QLIST_EMPTY(&bs->dirty_bitmaps));
> -
> if (bs->drv) {
> BdrvChild *child, *next;
>
> @@ -2363,6 +2360,9 @@ static void bdrv_close(BlockDriverState *bs)
> bs->full_open_options = NULL;
> }
>
> + bdrv_release_named_dirty_bitmaps(bs);
> + assert(QLIST_EMPTY(&bs->dirty_bitmaps));
> +
> QLIST_FOREACH_SAFE(ban, &bs->aio_notifiers, list, ban_next) {
> g_free(ban);
> }
Might deserve an own patch, but I don't mind.
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index ba72b7d2ac..e377215d5c 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
[...]
> @@ -127,6 +145,70 @@ static int check_table_entry(uint64_t entry, int
> cluster_size)
> return 0;
> }
>
> +static int check_constraints_on_bitmap(BlockDriverState *bs,
> + const char *name,
> + uint32_t granularity,
> + Error **errp)
> +{
> + BDRVQcow2State *s = bs->opaque;
> + int granularity_bits = ctz32(granularity);
> + int64_t len = bdrv_getlength(bs);
> +
> + assert(granularity > 0);
> + assert((granularity & (granularity - 1)) == 0);
> +
> + if (len < 0) {
> + error_setg_errno(errp, -len, "Failed to get size of '%s'",
> + bdrv_get_device_or_node_name(bs));
> + return len;
> + }
> +
> + if (granularity_bits > BME_MAX_GRANULARITY_BITS) {
> + error_setg(errp, "Granularity exceeds maximum (%u bytes)",
> + 1 << BME_MAX_GRANULARITY_BITS);
This will overflow because 1 << 31 is not representable in int (and 1 is
an int). The %u saves it by converting it back, but it's still
implementation-defined behavior at most.
I'd prefer a plain 1ull and %ull. That way, this would be save no matter
what value BME_MAX_GRANULARITY_BITS is.
> + return -EINVAL;
> + }
> + if (granularity_bits < BME_MIN_GRANULARITY_BITS) {
> + error_setg(errp, "Granularity is under minimum (%u bytes)",
> + 1 << BME_MIN_GRANULARITY_BITS);
The same applies here, although this does not have the overflow issue.
Rest looks good (to me O:-)).
Max
> + return -EINVAL;
> + }
> +
> + if ((len > (uint64_t)BME_MAX_PHYS_SIZE << granularity_bits) ||
> + (len > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size <<
> + granularity_bits))
> + {
> + error_setg(errp, "Too much space will be occupied by the bitmap. "
> + "Use larger granularity");
> + return -EINVAL;
> + }
> +
> + if (strlen(name) > BME_MAX_NAME_SIZE) {
> + error_setg(errp, "Name length exceeds maximum (%u characters)",
> + BME_MAX_NAME_SIZE);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
signature.asc
Description: OpenPGP digital signature
