[Qemu-trivial] [PATCH] Teach block/vdi about "discarded" (no longer allocated) blocks

[Qemu-trivial] [PATCH] Teach block/vdi about "discarded" (no longer allocated) blocks

[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)
+
+/* 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;
-- 
1.7.7.1

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Teach block/vdi about "discarded" (no longer allocated) blocks

[Stefan Weil]

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

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Teach block/vdi about "discarded" (no longer allocated) blocks

[Eric Sunshine]

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

Re: [Qemu-trivial] [PATCH] Teach block/vdi about "discarded" (no longer allocated) blocks

[Stefan Hajnoczi]

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

Re: [Qemu-trivial] [PATCH] Teach block/vdi about "discarded" (no longer allocated) blocks

[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

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Teach block/vdi about "discarded" (no longer allocated) blocks

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

Cheers,
Stefan

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Teach block/vdi about "discarded" (no longer allocated) blocks

[Eric Sunshine]

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

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Teach block/vdi about "discarded" (no longer allocated) blocks

[Kevin Wolf]

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

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Teach block/vdi about "discarded" (no longer allocated) blocks

[Eric Sunshine]

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

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Teach block/vdi about "discarded" (no longer allocated) blocks

[Kevin Wolf]

Am 28.10.2011 10:15, schrieb Eric Sunshine:
> 
> 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.

I'll amend it with this change:

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

 /* A never-allocated block; semantically arbitrary content. */
-#define VDI_UNALLOCATED ((uint32_t)~0)
+#define VDI_UNALLOCATED 0xffffffffU

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

 #define VDI_IS_ALLOCATED(X) ((X) < VDI_DISCARDED)