From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:54454) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RJA2r-0005Xo-5i for qemu-devel@nongnu.org; Wed, 26 Oct 2011 16:25:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RJA2p-0001Kb-A0 for qemu-devel@nongnu.org; Wed, 26 Oct 2011 16:25:53 -0400 Message-ID: <4EA86C98.8040303@weilnetz.de> Date: Wed, 26 Oct 2011 22:24:56 +0200 From: Stefan Weil MIME-Version: 1.0 References: <1319658678-18355-1-git-send-email-sunshine@sunshineco.com> In-Reply-To: <1319658678-18355-1-git-send-email-sunshine@sunshineco.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] Teach block/vdi about "discarded" (no longer allocated) blocks List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Sunshine Cc: qemu-trivial@nongnu.org, Kevin Wolf , qemu-devel@nongnu.org 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 > --- > > 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 > Cc: Kevin Wolf > > 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