[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Teach block/vdi about "discarded

From: Eric Sunshine
Subject: Re: [Qemu-trivial] [Qemu-devel] [PATCH] Teach block/vdi about "discarded" (no longer allocated) blocks
Date: Wed, 26 Oct 2011 16:54:40 -0400

On Oct 26, 2011, at 4:24 PM, Stefan Weil wrote:
Thank you for this extension. I have several remarks - see below.

Am 26.10.2011 21:51, schrieb Eric Sunshine:
An entry in the VDI block map will hold an offset to the actual block if
the block is allocated, or one of two specially-interpreted values if
not allocated. Using VirtualBox terminology, value VDI_IMAGE_BLOCK_FREE (0xffffffff) represents a never-allocated block (semantically arbitrary
content). VDI_IMAGE_BLOCK_ZERO (0xfffffffe) represents a "discarded"
block (semantically zero-filled). block/vdi knows only about

Signed-off-by: Eric Sunshine <address@hidden>

Without this patch, "qemu-image check" on a VDI image containing
discarded blocks reports errors such as:

ERROR: block index 3434 too large, is 4294967294

Decimal 4294967294 is 0xfffffffe. Worse, "qemu-image convert" or direct access of the VDI image from qemu involves reads and writes of blocks at
the bogus block offset 4294967294 within the image file.

Cc: Stefan Weil <address@hidden>
Cc: Kevin Wolf <address@hidden>

block/vdi.c | 23 ++++++++++++++---------
1 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index 883046d..25790c4 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -114,8 +114,13 @@ void uuid_unparse(const uuid_t uu, char *out);
#define VDI_TEXT "<<< QEMU VM Virtual Disk Image >>>\n"

-/* Unallocated blocks use this index (no need to convert endianness). */
+/* A never-allocated block; semantically arbitrary content. */
+#define VDI_UNALLOCATED ((uint32_t)~0)

Why did you change the definition of VDI_UNALLOCATED?
Or do you get a difference with the old definition?

My hope was that future readers of the code might find it easier to assimilate if it used the same notation "(uint32_t)~0" as the VirtualBox source code (which also is the most accurate documentation of the VDI format). I don't have particularly strong feelings about it and can re-roll using UINT32_MAX if you prefer.

It's ok to change the comment, but you missed an important point (endianness).

The removal of the comment was intentional because it was ambiguous and confusing rather than illuminating. Specifically, it does not explain if this is a case of programmer laziness (0xffffffff being the same on big- and little-endian) or if code employing VDI_UNALLOCATED applies proper endian conversions. Had the comment indicated that VDI_UNALLOCATED is only ever employed with host-endian values (which is the case), then that would have been worth retaining. I can re-roll with a clearer comment but would be sorry to see the confusing comment retained.

+/* A discarded (no longer allocated) block; semantically zero- filled. */
+#define VDI_DISCARDED ((uint32_t)~1)

The type cast is not needed. Please use



#if !defined(CONFIG_UUID)
void uuid_generate(uuid_t out)
@@ -307,10 +312,10 @@ static int vdi_check(BlockDriverState *bs, BdrvCheckResult *res)
/* Check block map and value of blocks_allocated. */
for (block = 0; block < s->header.blocks_in_image; block++) {
uint32_t bmap_entry = le32_to_cpu(s->bmap[block]);
- if (bmap_entry != VDI_UNALLOCATED) {
+ if (VDI_IS_ALLOCATED(bmap_entry)) {
if (bmap_entry < s->header.blocks_in_image) {
- if (bmap[bmap_entry] == VDI_UNALLOCATED) {
+ if (!VDI_IS_ALLOCATED(bmap[bmap_entry])) {
bmap[bmap_entry] = bmap_entry;
} else {
fprintf(stderr, "ERROR: block index %" PRIu32
@@ -472,7 +477,7 @@ static int vdi_is_allocated(BlockDriverState *bs, int64_t sector_num,
n_sectors = nb_sectors;
*pnum = n_sectors;
- return bmap_entry != VDI_UNALLOCATED;
+ return VDI_IS_ALLOCATED(bmap_entry);

static void vdi_aio_cancel(BlockDriverAIOCB *blockacb)
@@ -603,7 +608,7 @@ static void vdi_aio_read_cb(void *opaque, int ret)
/* prepare next AIO request */
acb->n_sectors = n_sectors;
bmap_entry = le32_to_cpu(s->bmap[block_index]);
- if (bmap_entry == VDI_UNALLOCATED) {
+ if (!VDI_IS_ALLOCATED(bmap_entry)) {
/* Block not allocated, return zeros, no need to wait. */
memset(acb->buf, 0, n_sectors * SECTOR_SIZE);
ret = vdi_schedule_bh(vdi_aio_rw_bh, acb);
@@ -685,7 +690,7 @@ static void vdi_aio_write_cb(void *opaque, int ret)
if (acb->header_modified) {
VdiHeader *header = acb->block_buffer;
logout("now writing modified header\n");
- assert(acb->bmap_first != VDI_UNALLOCATED);
+ assert(VDI_IS_ALLOCATED(acb->bmap_first));
*header = s->header;
acb->header_modified = 0;
@@ -699,7 +704,7 @@ static void vdi_aio_write_cb(void *opaque, int ret)
goto done;
- } else if (acb->bmap_first != VDI_UNALLOCATED) {
+ } else if (VDI_IS_ALLOCATED(acb->bmap_first)) {
/* One or more new blocks were allocated. */
uint64_t offset;
uint32_t bmap_first;
@@ -749,7 +754,7 @@ static void vdi_aio_write_cb(void *opaque, int ret)
/* prepare next AIO request */
acb->n_sectors = n_sectors;
bmap_entry = le32_to_cpu(s->bmap[block_index]);
- if (bmap_entry == VDI_UNALLOCATED) {
+ if (!VDI_IS_ALLOCATED(bmap_entry)) {
/* Allocate new block and write to it. */
uint64_t offset;
uint8_t *block;

Did you test your code for big endian hosts?
While 0xffffffff does not change with the endianness, 0xfffffffe does.

Yes, my work has been done on a big-endian PowerPC iMac G5. I also audited the code to ensure that all functionality dealing with VDI_UNALLOCATED and VDI_DISCARDED involves only host-endian values, hence host-endian ((uint32_t)~0) and ((uint32_t)~1) or UINT32_MAX and UINT32_MAX -1 work correctly.

-- ES

reply via email to

[Prev in Thread] Current Thread [Next in Thread]