| Message ID | 1319658678-18355-1-git-send-email-sunshine@sunshineco.com |
|---|---|
| State | New |
| Headers | show |
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 > VDI_IMAGE_BLOCK_FREE. Teach it about VDI_IMAGE_BLOCK_ZERO. > > Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> > --- > > 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 <weil@mail.berlios.de> > Cc: Kevin Wolf <kwolf@redhat.com> > > 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). */ > -#define VDI_UNALLOCATED UINT32_MAX > +/* 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? It's ok to change the comment, but you missed an important point (endianness). > + > +/* A discarded (no longer allocated) block; semantically zero-filled. */ > +#define VDI_DISCARDED ((uint32_t)~1) The type cast is not needed. Please use #define VDI_DISCARD (VDI_UNALLOCATED - 1) > + > +#define VDI_IS_ALLOCATED(X) ((X) < VDI_DISCARDED) > > #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) { > blocks_allocated++; > - 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; > vdi_header_to_le(header); > acb->header_modified = 0; > @@ -699,7 +704,7 @@ static void vdi_aio_write_cb(void *opaque, int ret) > goto done; > } > return; > - } 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. Kind regards, Stefan Weil
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 >> VDI_IMAGE_BLOCK_FREE. Teach it about VDI_IMAGE_BLOCK_ZERO. >> >> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> >> --- >> >> 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 <weil@mail.berlios.de> >> Cc: Kevin Wolf <kwolf@redhat.com> >> >> 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). */ >> -#define VDI_UNALLOCATED UINT32_MAX >> +/* 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 > > #define VDI_DISCARD (VDI_UNALLOCATED - 1) > >> + >> +#define VDI_IS_ALLOCATED(X) ((X) < VDI_DISCARDED) >> >> #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) { >> blocks_allocated++; >> - 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; >> vdi_header_to_le(header); >> acb->header_modified = 0; >> @@ -699,7 +704,7 @@ static void vdi_aio_write_cb(void *opaque, int >> ret) >> goto done; >> } >> return; >> - } 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
On Wed, Oct 26, 2011 at 03:51:18PM -0400, Eric Sunshine wrote: > 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 > VDI_IMAGE_BLOCK_FREE. Teach it about VDI_IMAGE_BLOCK_ZERO. > > Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> > --- > > 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 <weil@mail.berlios.de> > Cc: Kevin Wolf <kwolf@redhat.com> > > block/vdi.c | 23 ++++++++++++++--------- > 1 files changed, 14 insertions(+), 9 deletions(-) Good to see this improvement. I recently talked to a CernVM developer who had issues with vdi images. This may fix the issue they were seeing. I think Kevin should take this through the block tree. I won't apply it to trivial-patches. Stefan
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 > VDI_IMAGE_BLOCK_FREE. Teach it about VDI_IMAGE_BLOCK_ZERO. > > Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> Thanks, applied to the block branch. Kevin
Am 27.10.2011 10:53, schrieb Kevin Wolf: > 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 >> VDI_IMAGE_BLOCK_FREE. Teach it about VDI_IMAGE_BLOCK_ZERO. >> >> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> > > Thanks, applied to the block branch. > > Kevin Kevin, I don't want to block improvements. Nevertheless I'd like to see a small modification in this patch: both #defines should be implemented without a type cast. Please change them or wait until Eric sends an update. My favorite is this: #define VDI_UNALLOCATED UINT32_MAX #define VDI_DISCARD (VDI_UNALLOCATED - 1) This would also be ok: #define VDI_UNALLOCATED 0xffffffffU #define VDI_DISCARD 0xfffffffeU Using the macro names and the definitions (with type cast) from the original VirtualBox code would also be ok. Cheers, Stefan
On Oct 27, 2011, at 12:12 PM, Stefan Weil wrote: > Am 27.10.2011 10:53, schrieb Kevin Wolf: >> 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 >>> VDI_IMAGE_BLOCK_FREE. Teach it about VDI_IMAGE_BLOCK_ZERO. >>> >>> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> >> >> Thanks, applied to the block branch. >> >> Kevin > > > Kevin, I don't want to block improvements. Nevertheless > I'd like to see a small modification in this patch: > both #defines should be implemented without a type cast. > Please change them or wait until Eric sends an update. > > My favorite is this: > > #define VDI_UNALLOCATED UINT32_MAX > #define VDI_DISCARD (VDI_UNALLOCATED - 1) > > This would also be ok: > > #define VDI_UNALLOCATED 0xffffffffU > #define VDI_DISCARD 0xfffffffeU > > Using the macro names and the definitions (with type cast) > from the original VirtualBox code would also be ok. I originally implemented the change using the macro names, comments, and definitions from the VirtualBox code, but found that it made the diff so noisy that it obscured the simpler underlying change of teaching block/vdi about "discarded" blocks. Sticking with the original VDI_UNALLOCATED macro name kept the diff noise level down. At any rate, if Kevin can amend the commit with one of your above suggestions, that would be simplest. Otherwise, I can re-roll. Let me know which is preferred. -- ES
Am 27.10.2011 18:12, schrieb Stefan Weil: > Am 27.10.2011 10:53, schrieb Kevin Wolf: >> 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 >>> VDI_IMAGE_BLOCK_FREE. Teach it about VDI_IMAGE_BLOCK_ZERO. >>> >>> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> >> >> Thanks, applied to the block branch. >> >> Kevin > > > Kevin, I don't want to block improvements. Nevertheless > I'd like to see a small modification in this patch: > both #defines should be implemented without a type cast. > Please change them or wait until Eric sends an update. > > My favorite is this: > > #define VDI_UNALLOCATED UINT32_MAX > #define VDI_DISCARD (VDI_UNALLOCATED - 1) > > This would also be ok: > > #define VDI_UNALLOCATED 0xffffffffU > #define VDI_DISCARD 0xfffffffeU > > Using the macro names and the definitions (with type cast) > from the original VirtualBox code would also be ok. I did see your comments, and I waited for the endianness thing to be answered. However, how the definition of these constants is written is really not a functional defect, but simply a matter of taste. It's an old rule that whoever does the work also decides on the details. I really think it's wasting our time if we need to discuss if a type cast in the constant definition is only allowed after typedefing uint32_t to something else like in VBox. So my preferred way is to leave the patch as it is. The code is simple and clear and objectively seen it won't get any better with your taste applied. If Eric prefers, I can update it to use 0xffffffffU, though. Kevin
On Oct 28, 2011, at 4:00 AM, Kevin Wolf wrote: > Am 27.10.2011 18:12, schrieb Stefan Weil: >> Am 27.10.2011 10:53, schrieb Kevin Wolf: >>> 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 >>>> VDI_IMAGE_BLOCK_FREE. Teach it about VDI_IMAGE_BLOCK_ZERO. >>>> >>>> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> >>> >>> Thanks, applied to the block branch. >>> >>> Kevin >> >> >> Kevin, I don't want to block improvements. Nevertheless >> I'd like to see a small modification in this patch: >> both #defines should be implemented without a type cast. >> Please change them or wait until Eric sends an update. >> >> My favorite is this: >> >> #define VDI_UNALLOCATED UINT32_MAX >> #define VDI_DISCARD (VDI_UNALLOCATED - 1) >> >> This would also be ok: >> >> #define VDI_UNALLOCATED 0xffffffffU >> #define VDI_DISCARD 0xfffffffeU >> >> Using the macro names and the definitions (with type cast) >> from the original VirtualBox code would also be ok. > > I did see your comments, and I waited for the endianness thing to be > answered. However, how the definition of these constants is written is > really not a functional defect, but simply a matter of taste. It's an > old rule that whoever does the work also decides on the details. > > I really think it's wasting our time if we need to discuss if a type > cast in the constant definition is only allowed after typedefing > uint32_t to something else like in VBox. > > So my preferred way is to leave the patch as it is. The code is simple > and clear and objectively seen it won't get any better with your taste > applied. If Eric prefers, I can update it to use 0xffffffffU, though. The 0xffffffffU notation has the benefit of being explicit, whereas the ((uint32_t)~0) notation, taken from the VirtualBox source, is somewhat magical for a reader who does not perform an automatic ((uint32_t)~0) == 0xffffffffU conversion in his head. Consequently, the 0xffffffffU notation might a better choice, if it's not too much bother for you to amend the patch. -- ES
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). */ -#define VDI_UNALLOCATED UINT32_MAX +/* A never-allocated block; semantically arbitrary content. */ +#define VDI_UNALLOCATED ((uint32_t)~0) + +/* A discarded (no longer allocated) block; semantically zero-filled. */ +#define VDI_DISCARDED ((uint32_t)~1) + +#define VDI_IS_ALLOCATED(X) ((X) < VDI_DISCARDED) #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) { blocks_allocated++; - 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; vdi_header_to_le(header); acb->header_modified = 0; @@ -699,7 +704,7 @@ static void vdi_aio_write_cb(void *opaque, int ret) goto done; } return; - } 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;
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 VDI_IMAGE_BLOCK_FREE. Teach it about VDI_IMAGE_BLOCK_ZERO. Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> --- 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 <weil@mail.berlios.de> Cc: Kevin Wolf <kwolf@redhat.com> block/vdi.c | 23 ++++++++++++++--------- 1 files changed, 14 insertions(+), 9 deletions(-)