Patchwork vdi: Fix image opening and creation for odd disk sizes

login
register
mail settings
Submitter Stefan Weil
Date May 10, 2010, 8:12 p.m.
Message ID <1273522353-12851-1-git-send-email-weil@mail.berlios.de>
Download mbox | patch
Permalink /patch/52138/
State New
Headers show

Comments

Stefan Weil - May 10, 2010, 8:12 p.m.
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(-)
François Revol - May 10, 2010, 8:44 p.m.
Le Mon, 10 May 2010 22:12:33 +0200, Stefan Weil a écrit :
> 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>

Looks good to me on first read.

François.
Kevin Wolf - May 11, 2010, 7:52 a.m.
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

Patch

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;
+    }
+
     bs->total_sectors = header.disk_size / SECTOR_SIZE;
 
     s->block_size = header.block_size;
@@ -829,7 +839,10 @@  static int vdi_create(const char *filename, QEMUOptionParameter *options)
         return -errno;
     }
 
-    blocks = bytes / block_size;
+    /* We need enough blocks to store the given disk size,
+       so always round up. */
+    blocks = (bytes + block_size - 1) / block_size;
+
     bmap_size = blocks * sizeof(uint32_t);
     bmap_size = ((bmap_size + SECTOR_SIZE - 1) & ~(SECTOR_SIZE -1));