Hi Stefan, hi Jeff,
please cc me for future block/vdi.c related patches. See more comments
below.
Am 26.03.2014 13:05, schrieb Stefan Hajnoczi:
> From: Jeff Cody <jc...@redhat.com>
>
> The maximum blocks_in_image is 0xffffffff / 4, which also limits the
> maximum disk_size for a VDI image.
>
> Signed-off-by: Jeff Cody <jc...@redhat.com>
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> ---
> block/vdi.c | 28 +++++++++++++++++++++++++---
> 1 file changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/block/vdi.c b/block/vdi.c
> index ac9a025..718884d 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -120,6 +120,12 @@ typedef unsigned char uuid_t[16];
>
> #define VDI_IS_ALLOCATED(X) ((X) < VDI_DISCARDED)
>
> +#define VDI_BLOCK_SIZE (1 * MiB)
There is already DEFAULT_CLUSTER_SIZE which should be used instead of
VDI_BLOCK_SIZE.
> +/* max blocks in image is (0xffffffff / 4) */
> +#define VDI_BLOCKS_IN_IMAGE_MAX 0x3fffffff
This looks wrong. I think it should be (SIZE_MAX / sizeof(uint32_t)). 64
bit platforms with a 64 bit size_t allow more blocks.
> +#define VDI_DISK_SIZE_MAX ((uint64_t)VDI_BLOCKS_IN_IMAGE_MAX * \
> + (uint64_t)VDI_BLOCK_SIZE)
> +
This macro cannot be used because the block size might have a non
default value.
> #if !defined(CONFIG_UUID)
> static inline void uuid_generate(uuid_t out)
> {
> @@ -385,6 +391,11 @@ static int vdi_open(BlockDriverState *bs, QDict
> *options, int flags,
> vdi_header_print(&header);
> #endif
>
> + if (header.disk_size > VDI_DISK_SIZE_MAX) {
Here error_setg is missing. The style does not match the other checks.
More changes are needed because VDI_DISK_SIZE_MAX cannot be used.
> + ret = -EINVAL;
> + goto fail;
> + }
> +
> if (header.disk_size % SECTOR_SIZE != 0) {
> /* 'VBoxManage convertfromraw' can create images with odd disk sizes.
> We accept them but round the disk size to the next multiple of
> @@ -420,9 +431,9 @@ static int vdi_open(BlockDriverState *bs, QDict *options,
> int flags,
> header.sector_size, SECTOR_SIZE);
> ret = -ENOTSUP;
> goto fail;
> - } else if (header.block_size != 1 * MiB) {
> + } else if (header.block_size != VDI_BLOCK_SIZE) {
> error_setg(errp, "unsupported VDI image (sector size %u is not %u)",
Here is a copy+paste bug which was recently introduced.
> - header.block_size, 1 * MiB);
> + header.block_size, VDI_BLOCK_SIZE);
Replace VDI_BLOCK_SIZE by the existing macro here.
> ret = -ENOTSUP;
> goto fail;
> } else if (header.disk_size >
> @@ -441,6 +452,10 @@ static int vdi_open(BlockDriverState *bs, QDict
> *options, int flags,
> error_setg(errp, "unsupported VDI image (non-NULL parent UUID)");
> ret = -ENOTSUP;
> goto fail;
> + } else if (header.blocks_in_image > VDI_BLOCKS_IN_IMAGE_MAX) {
> + error_setg(errp, "unsupported VDI image (too many blocks)");
Fix test and improve error message (show limit) here.
> + ret = -ENOTSUP;
> + goto fail;
> }
>
> bs->total_sectors = header.disk_size / SECTOR_SIZE;
> @@ -689,11 +704,17 @@ static int vdi_create(const char *filename,
> QEMUOptionParameter *options,
> options++;
> }
>
> + if (bytes > VDI_DISK_SIZE_MAX) {
Dto.
> + result = -EINVAL;
> + goto exit;
> + }
> +
> fd = qemu_open(filename,
> O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
> 0644);
> if (fd < 0) {
> - return -errno;
> + result = -errno;
> + goto exit;
> }
>
> /* We need enough blocks to store the given disk size,
> @@ -754,6 +775,7 @@ static int vdi_create(const char *filename,
> QEMUOptionParameter *options,
> result = -errno;
> }
>
> +exit:
Is goto+label better than a simple return statement?
> return result;
> }
>
>
Do we need this patch for QEMU 2.0? For 32 bit systems, the image size
limit is 1000 TB, and that image would need 4 GB for the block cache in
memory. Are such image sizes used anywhere? For 64 bit systems, the
limit will be much higher.
Regards
Stefan
