Re: [Qemu-devel] [PATCH v16 13/22] qcow2: add persistent dirty bitmaps support

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3684 bytes --]

On 25.02.2017 18:07, Vladimir Sementsov-Ogievskiy wrote:
> Store persistent dirty bitmaps in qcow2 image.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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;
> +}


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]