[Qemu-devel] Re: [PATCH] vdi: Fix image opening and creation for odd disk sizes

[Kevin Wolf <kwolf@redhat.com>]

Am 10.05.2010 22:12, schrieb Stefan Weil:
> The fix is based on a patch from Kevin Wolf. Here his comment:
> 
> "The number of blocks needs to be rounded up to cover all of the virtual hard
> disk. Without this fix, we can't even open our own images if their size is not
> a multiple of the block size."
> 
> While Kevin's patch addressed vdi_create, my modification also fixes
> vdi_open which now accepts images with odd disk sizes as well as
> images created with old versions of qemu-img.
> 
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: François Revol <revol@free.fr>
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> ---
>  block/vdi.c |   29 +++++++++++++++++++++--------
>  1 files changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/block/vdi.c b/block/vdi.c
> index 1ce18d5..362c898 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -405,19 +405,12 @@ static int vdi_open(BlockDriverState *bs, int flags)
>          /* We only support data blocks which start on a sector boundary. */
>          logout("unsupported data offset 0x%x B\n", header.offset_data);
>          goto fail;
> -    } else if (header.disk_size % SECTOR_SIZE != 0) {
> -        logout("unsupported disk size %" PRIu64 " B\n", header.disk_size);
> -        goto fail;
>      } else if (header.sector_size != SECTOR_SIZE) {
>          logout("unsupported sector size %u B\n", header.sector_size);
>          goto fail;
>      } else if (header.block_size != 1 * MiB) {
>          logout("unsupported block size %u B\n", header.block_size);
>          goto fail;
> -    } else if ((header.disk_size + header.block_size - 1) / header.block_size !=
> -               (uint64_t)header.blocks_in_image) {
> -        logout("unexpected block number %u B\n", header.blocks_in_image);
> -        goto fail;
>      } else if (!uuid_is_null(header.uuid_link)) {
>          logout("link uuid != 0, unsupported\n");
>          goto fail;
> @@ -426,6 +419,23 @@ static int vdi_open(BlockDriverState *bs, int flags)
>          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
> +           SECTOR_SIZE. */
> +        logout("odd disk size %" PRIu64 " B, round up\n", header.disk_size);
> +        header.disk_size += SECTOR_SIZE - 1;
> +        header.disk_size &= ~(SECTOR_SIZE - 1);
> +    }
> +
> +    if (header.disk_size >
> +        (uint64_t)header.blocks_in_image * header.block_size) {
> +        /* Old versions of qemu-img could create images with too large
> +           disk sizes. We accept them but truncate the disk size. */
> +        logout("large disk size %" PRIu64 " B, truncated\n", header.disk_size);
> +        header.disk_size = (uint64_t)header.blocks_in_image * header.block_size;
> +    }

I don't think this is useful behaviour. Such images are broken and
should not be opened. While it's true that qemu-img could create such
images, qemu could never open them afterwards, so nobody can have used
them anyway. So I think a goto fail; is the right thing to do.

Otherwise the patch looks good to me now.

Kevin