[Qemu-devel] [PATCH] vdi: Fix image creation

[Qemu-devel] [PATCH] vdi: Fix image creation

[Kevin Wolf]

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.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vdi.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index 2b4d2c2..b990bbc 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -827,7 +827,7 @@ static int vdi_create(const char *filename, QEMUOptionParameter *options)
         return -errno;
     }
 
-    blocks = bytes / block_size;
+    blocks = (bytes + block_size - 1) / block_size;
     bmap_size = blocks * sizeof(uint32_t);
     bmap_size = ((bmap_size + SECTOR_SIZE - 1) & ~(SECTOR_SIZE -1));
 
-- 
1.6.6.1

[Qemu-devel] Re: [PATCH] vdi: Fix image creation

[Stefan Weil]

Am 06.05.2010 14:55, schrieb Kevin Wolf:
> 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.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> block/vdi.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/block/vdi.c b/block/vdi.c
> index 2b4d2c2..b990bbc 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -827,7 +827,7 @@ static int vdi_create(const char *filename, 
> QEMUOptionParameter *options)
> return -errno;
> }
>
> - blocks = bytes / block_size;
> + blocks = (bytes + block_size - 1) / block_size;
> bmap_size = blocks * sizeof(uint32_t);
> bmap_size = ((bmap_size + SECTOR_SIZE - 1) & ~(SECTOR_SIZE -1));
>


'bytes' (for header.disk_size) must be fixed, too, and so does vdi_open.
I'll send a patch which hopefully addresses all these points.

Thanks + kind regards,
Stefan

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

[Stefan Weil]

This patch fixes a regression introduced by commit
95a2f9bc588c3f83375d87b0a9394f89a1bcfada.

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 any image which is large enough to hold
the blocks.

I also decided to keep the original code in vdi_create which rounds down.
Rounding works in both directions, and there are good arguments for both,
so I just left the original simple code.

It is very important to use the rounded value for the new disk size, too -
otherwise VirtualBox cannot open our disk image.

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 |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index 1d257b4..2213819 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -414,8 +414,8 @@ static int vdi_open(BlockDriverState *bs, int flags)
     } 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) {
+    } else if (header.disk_size < 
+               (uint64_t)header.blocks_in_image * header.block_size) {
         logout("unexpected block number %u B\n", header.blocks_in_image);
         goto fail;
     } else if (!uuid_is_null(header.uuid_link)) {
@@ -827,7 +827,10 @@ static int vdi_create(const char *filename, QEMUOptionParameter *options)
         return -errno;
     }
 
+    /* VirtualBox wants a disk size which is a multiple of the block size. */
     blocks = bytes / block_size;
+    bytes = blocks * block_size;
+
     bmap_size = blocks * sizeof(uint32_t);
     bmap_size = ((bmap_size + SECTOR_SIZE - 1) & ~(SECTOR_SIZE -1));
 
-- 
1.7.0

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

[Kevin Wolf]

Am 06.05.2010 20:29, schrieb Stefan Weil:
> This patch fixes a regression introduced by commit
> 95a2f9bc588c3f83375d87b0a9394f89a1bcfada.
> 
> 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 any image which is large enough to hold
> the blocks.

Shouldn't it be the other way round? That is, an image which has some
unused blocks at its end makes sense, whereas an image with a virtual
disk size that can't be represented with the number of blocks doesn't?

> I also decided to keep the original code in vdi_create which rounds down.
> Rounding works in both directions, and there are good arguments for both,
> so I just left the original simple code.
> 
> It is very important to use the rounded value for the new disk size, too -
> otherwise VirtualBox cannot open our disk image.

So you're saying that in VDI you can't represent disks with an odd size?
The one thing common across image formats seems to be that they are
broken...

Kevin

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

[François Revol]

Le Fri, 07 May 2010 09:55:23 +0200, Kevin Wolf a écrit :
> Am 06.05.2010 20:29, schrieb Stefan Weil:
> > This patch fixes a regression introduced by commit
> > 95a2f9bc588c3f83375d87b0a9394f89a1bcfada.
> >
> > 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 any image which is large enough to hold
> > the blocks.
>
> Shouldn't it be the other way round? That is, an image which has some
> unused blocks at its end makes sense, whereas an image with a virtual
> disk size that can't be represented with the number of blocks
> doesn't?

Exactly, else you don't create what you are asked for.

> > I also decided to keep the original code in vdi_create which rounds
> > down.
> > Rounding works in both directions, and there are good arguments for
> > both,
> > so I just left the original simple code.
> >
> > It is very important to use the rounded value for the new disk
> > size, too -
> > otherwise VirtualBox cannot open our disk image.
>
> So you're saying that in VDI you can't represent disks with an odd
> size?
> The one thing common across image formats seems to be that they are
> broken...

VB works quite well with my converted laptop image which indeed doesn't
end on block boundary.

Was it because you were just setting size larger than the covered by
the blocks ?

François.

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

[Stefan Weil]

Am 07.05.2010 13:55, schrieb François Revol:
> Le Fri, 07 May 2010 09:55:23 +0200, Kevin Wolf a écrit :
>> Am 06.05.2010 20:29, schrieb Stefan Weil:
>>> This patch fixes a regression introduced by commit
>>> 95a2f9bc588c3f83375d87b0a9394f89a1bcfada.
>>>
>>> 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 any image which is large enough to hold
>>> the blocks.
>>
>> Shouldn't it be the other way round? That is, an image which has some
>> unused blocks at its end makes sense, whereas an image with a virtual
>> disk size that can't be represented with the number of blocks
>> doesn't?
>
> Exactly, else you don't create what you are asked for.
>
>>> I also decided to keep the original code in vdi_create which rounds
>>> down.
>>> Rounding works in both directions, and there are good arguments for
>>> both,
>>> so I just left the original simple code.
>>>
>>> It is very important to use the rounded value for the new disk
>>> size, too -
>>> otherwise VirtualBox cannot open our disk image.
>>
>> So you're saying that in VDI you can't represent disks with an odd
>> size?
>> The one thing common across image formats seems to be that they are
>> broken...
>
> VB works quite well with my converted laptop image which indeed doesn't
> end on block boundary.
>
> Was it because you were just setting size larger than the covered by
> the blocks ?
>
> François.

Kevin and you are right, and my interpretation of disk_size was wrong.

disk_size is not the size used for the blocks (then it would have to be
large enough to keep all blocks).

disk_size is the number of bytes which are really used for data
(so it is less or equal blocks_in_image * 1 MiB). VBoxManage
allows creation of disk images which use the last block only partially,
something I did not know up to now.

Kevin's patch is correct but still incomplete. VBoxManage can
create images with really odd disk sizes (even sizes which are not
a multiple of the sector size), so the checks in vdi_open
need modifications. The current code also fails for read or write
access beyond the last block.

So I'll send a new patch...

Regards
Stefan

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

[Kevin Wolf]

Am 09.05.2010 12:17, schrieb Stefan Weil:
> Kevin and you are right, and my interpretation of disk_size was wrong.
> 
> disk_size is not the size used for the blocks (then it would have to be
> large enough to keep all blocks).
> 
> disk_size is the number of bytes which are really used for data
> (so it is less or equal blocks_in_image * 1 MiB). VBoxManage
> allows creation of disk images which use the last block only partially,
> something I did not know up to now.

Ok. Makes a lot more sense this way.

> Kevin's patch is correct but still incomplete. VBoxManage can
> create images with really odd disk sizes (even sizes which are not
> a multiple of the sector size), so the checks in vdi_open
> need modifications. The current code also fails for read or write
> access beyond the last block.

Not sure what the semantics of such images is. Disk emulation should
only access complete sectors, and the qemu block layer works with
sectors, too. How does VBox implement this? Is the size rounded for the
virtual disk size? And in what direction?

Kevin

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

[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;
+    }
+
     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));
 
-- 
1.7.1

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

[François Revol]

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.

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

[Kevin Wolf]

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

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

[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.

v3:
Don't allow reading of disk images with too large disk sizes.
Neither VBoxManage nor old versions of qemu-img read such images.
This change requires rounding of odd disk sizes before we do the checks.

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 |   23 ++++++++++++++++-------
 1 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index 1ce18d5..b53a3c1 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -393,6 +393,15 @@ static int vdi_open(BlockDriverState *bs, int flags)
     vdi_header_print(&header);
 #endif
 
+    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.version != VDI_VERSION_1_1) {
         logout("unsupported version %u.%u\n",
                header.version >> 16, header.version & 0xffff);
@@ -405,18 +414,15 @@ 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);
+    } else if (header.disk_size >
+               (uint64_t)header.blocks_in_image * header.block_size) {
+        logout("unsupported disk size %" PRIu64 " B\n", header.disk_size);
         goto fail;
     } else if (!uuid_is_null(header.uuid_link)) {
         logout("link uuid != 0, unsupported\n");
@@ -829,7 +835,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));
 
-- 
1.7.1

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

[Kevin Wolf]

Am 12.05.2010 20:25, 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.
> 
> v3:
> Don't allow reading of disk images with too large disk sizes.
> Neither VBoxManage nor old versions of qemu-img read such images.
> This change requires rounding of odd disk sizes before we do the checks.
> 
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: François Revol <revol@free.fr>
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>

Thanks, applied to the block branch.

Kevin