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 <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). */
> -#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