Re: [Qemu-devel] [PATCH 1/5] block/qcow2-bitmap: Skip length check in some cases

[Eric Blake <eblake@redhat.com>]

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

On 3/5/19 5:43 PM, John Snow wrote:
> If we were to allow resizes, the length check that happens when we load
> bitmap headers from disk when we read or store bitmaps would begin to
> fail:
> 
> Imagine the circumstance where we've resized bitmaps in memory, but they still
> have the old values on-disk. The lengths will no longer match bdrv_getlength,
> so we must allow this check to be skipped when flushing bitmaps to disk.
> 
> Limit this to when we are about to overwrite the headers: we will verify the
> outgoing headers, but we will skip verifying the known stale headers.

No-op until we actually do allow resizes later in the series, but makes
sense.

> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/qcow2-bitmap.c | 34 +++++++++++++++++++++-------------
>  1 file changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index c3b210ede1..d02730004a 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -435,7 +435,8 @@ static inline Qcow2BitmapDirEntry *next_dir_entry(Qcow2BitmapDirEntry *entry)
>      return (Qcow2BitmapDirEntry *)((uint8_t *)entry + dir_entry_size(entry));
>  }
>  
> -static int check_dir_entry(BlockDriverState *bs, Qcow2BitmapDirEntry *entry)
> +static int check_dir_entry(BlockDriverState *bs, Qcow2BitmapDirEntry *entry,
> +                           bool allow_resize)
>  {
>      BDRVQcow2State *s = bs->opaque;
>      uint64_t phys_bitmap_bytes;
> @@ -462,8 +463,14 @@ static int check_dir_entry(BlockDriverState *bs, Qcow2BitmapDirEntry *entry)
>          return len;

Someday, it would be nice to plumb Error* through this function, so that
you can give distinct reasons for failure, rather than lumping all
failures into the nebulous "doesn't meet the constraints" because we
lost context when slamming multiple errors into a single -EINVAL. But
that's a separate patch series.

>      }
>  
> -    fail = (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) ||
> -           (len > ((phys_bitmap_bytes * 8) << entry->granularity_bits));
> +    if (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) {
> +        return -EINVAL;
> +    }
> +
> +    if (!allow_resize &&
> +        (len > ((phys_bitmap_bytes * 8) << entry->granularity_bits))) {
> +        return -EINVAL;
> +    }
>  
>      return fail ? -EINVAL : 0;

Dead conditional - with your refactoring, this line is only reached when
fail == false.

With it changed to 'return 0',
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


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