Re: [Qemu-devel] [PATCH 06/22] qcow2: add dirty bitmaps extension

[Max Reitz <mreitz@redhat.com>]

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

On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
> Add dirty bitmap extension as specified in docs/specs/qcow2.txt.
> For now, just mirror extension header into Qcow2 state and check
> constraints.
> 
> For now, disable image resize if it has bitmaps. It will be fixed later.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/qcow2.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  block/qcow2.h |  4 +++
>  2 files changed, 87 insertions(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index c079aa8..08c4ef9 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c

[...]

> @@ -162,6 +164,62 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
>              }
>              break;
>  
> +        case QCOW2_EXT_MAGIC_DIRTY_BITMAPS:
> +            ret = bdrv_pread(bs->file, offset, &bitmaps_ext, ext.len);

Overflows with ext.len > sizeof(bitmaps_ext).

(ext.len < sizeof(bitmaps_ext) is also wrong, but less dramatically so.)

> +            if (ret < 0) {
> +                error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
> +                                 "Could not read ext header");
> +                return ret;
> +            }
> +
> +            if (bitmaps_ext.reserved32 != 0) {
> +                error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
> +                                 "Reserved field is not zero.");

Please drop the full stop at the end.

> +                return -EINVAL;
> +            }
> +
> +            be32_to_cpus(&bitmaps_ext.nb_bitmaps);
> +            be64_to_cpus(&bitmaps_ext.bitmap_directory_size);
> +            be64_to_cpus(&bitmaps_ext.bitmap_directory_offset);
> +
> +            if (bitmaps_ext.nb_bitmaps > QCOW_MAX_DIRTY_BITMAPS) {
> +                error_setg(errp, "ERROR: bitmaps_ext: "
> +                                 "too many dirty bitmaps");
> +                return -EINVAL;
> +            }
> +
> +            if (bitmaps_ext.nb_bitmaps == 0) {
> +                error_setg(errp, "ERROR: bitmaps_ext: "
> +                                 "found bitmaps extension with zero bitmaps");
> +                return -EINVAL;
> +            }
> +
> +            if (bitmaps_ext.bitmap_directory_offset & (s->cluster_size - 1)) {
> +                error_setg(errp, "ERROR: bitmaps_ext: "
> +                                 "wrong dirty bitmap directory offset");

s/wrong/invalid/

> +                return -EINVAL;
> +            }
> +
> +            if (bitmaps_ext.bitmap_directory_size >
> +                QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE) {
> +                error_setg(errp, "ERROR: bitmaps_ext: "
> +                                 "too large dirty bitmap directory");
> +                return -EINVAL;
> +            }
> +
> +            s->nb_bitmaps = bitmaps_ext.nb_bitmaps;
> +            s->bitmap_directory_offset =
> +                    bitmaps_ext.bitmap_directory_offset;
> +            s->bitmap_directory_size =
> +                    bitmaps_ext.bitmap_directory_size;
> +
> +#ifdef DEBUG_EXT
> +            printf("Qcow2: Got dirty bitmaps extension:"
> +                   " offset=%" PRIu64 " nb_bitmaps=%" PRIu32 "\n",
> +                   s->bitmap_directory_offset, s->nb_bitmaps);
> +#endif
> +            break;
> +
>          default:
>              /* unknown magic - save it in case we need to rewrite the header */
>              {

[...]

> @@ -2509,6 +2585,13 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
>          return -ENOTSUP;
>      }
>  
> +    /* cannot proceed if image has bitmaps */
> +    if (s->nb_bitmaps) {
> +        /* FIXME */

I'd call it a TODO, but that's probably a matter of taste.

> +        error_report("Can't resize an image which has bitmaps");
> +        return -ENOTSUP;
> +    }
> +
>      /* shrinking is currently not supported */
>      if (offset < bs->total_sectors * 512) {
>          error_report("qcow2 doesn't support shrinking images yet");

Max


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