[Qemu-devel] [PATCH] block/vpc: fix get_sector_offset for vpc_read

[Qemu-devel] [PATCH] block/vpc: fix get_sector_offset for vpc_read

[Peter Lieven]

VHD images contain a bitmap at the beginning of each data block
to indicate the allocation status of each sector in the data block.

vpc_read currently checks the allocation status of the first sector
in a data block it is going to read and then assumes the same allocation
status for the whole remainder of the data block. This might end up
in incorrect zero sectors if the first sector is unallocated.

To fix this indiciate a sector is allocated as soon as its
data block has a valid page table index.

CC: qemu-stable@nongnu.org
Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/vpc.c |   32 --------------------------------
 1 file changed, 32 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index 46803b1..9a75334 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -376,38 +376,6 @@ static inline int64_t get_sector_offset(BlockDriverState *bs,
         bdrv_pwrite_sync(bs->file, bitmap_offset, bitmap, s->bitmap_size);
     }
 
-//    printf("sector: %" PRIx64 ", index: %x, offset: %x, bioff: %" PRIx64 ", bloff: %" PRIx64 "\n",
-//	sector_num, pagetable_index, pageentry_index,
-//	bitmap_offset, block_offset);
-
-// disabled by reason
-#if 0
-#ifdef CACHE
-    if (bitmap_offset != s->last_bitmap)
-    {
-	lseek(s->fd, bitmap_offset, SEEK_SET);
-
-	s->last_bitmap = bitmap_offset;
-
-	// Scary! Bitmap is stored as big endian 32bit entries,
-	// while we used to look it up byte by byte
-	read(s->fd, s->pageentry_u8, 512);
-	for (i = 0; i < 128; i++)
-	    be32_to_cpus(&s->pageentry_u32[i]);
-    }
-
-    if ((s->pageentry_u8[pageentry_index / 8] >> (pageentry_index % 8)) & 1)
-	return -1;
-#else
-    lseek(s->fd, bitmap_offset + (pageentry_index / 8), SEEK_SET);
-
-    read(s->fd, &bitmap_entry, 1);
-
-    if ((bitmap_entry >> (pageentry_index % 8)) & 1)
-	return -1; // not allocated
-#endif
-#endif
-
     return block_offset;
 }
 
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH] block/vpc: fix get_sector_offset for vpc_read

[Max Reitz]

On 2015-02-23 at 10:01, Peter Lieven wrote:
> VHD images contain a bitmap at the beginning of each data block
> to indicate the allocation status of each sector in the data block.
>
> vpc_read currently checks the allocation status of the first sector
> in a data block it is going to read and then assumes the same allocation
> status for the whole remainder of the data block. This might end up
> in incorrect zero sectors if the first sector is unallocated.
>
> To fix this indiciate a sector is allocated as soon as its
> data block has a valid page table index.

Hm, as far as I can see, this patch just removes unused code. How does 
this correspond to the commit message?

Max

> CC: qemu-stable@nongnu.org
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>   block/vpc.c |   32 --------------------------------
>   1 file changed, 32 deletions(-)
>
> diff --git a/block/vpc.c b/block/vpc.c
> index 46803b1..9a75334 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -376,38 +376,6 @@ static inline int64_t get_sector_offset(BlockDriverState *bs,
>           bdrv_pwrite_sync(bs->file, bitmap_offset, bitmap, s->bitmap_size);
>       }
>   
> -//    printf("sector: %" PRIx64 ", index: %x, offset: %x, bioff: %" PRIx64 ", bloff: %" PRIx64 "\n",
> -//	sector_num, pagetable_index, pageentry_index,
> -//	bitmap_offset, block_offset);
> -
> -// disabled by reason
> -#if 0
> -#ifdef CACHE
> -    if (bitmap_offset != s->last_bitmap)
> -    {
> -	lseek(s->fd, bitmap_offset, SEEK_SET);
> -
> -	s->last_bitmap = bitmap_offset;
> -
> -	// Scary! Bitmap is stored as big endian 32bit entries,
> -	// while we used to look it up byte by byte
> -	read(s->fd, s->pageentry_u8, 512);
> -	for (i = 0; i < 128; i++)
> -	    be32_to_cpus(&s->pageentry_u32[i]);
> -    }
> -
> -    if ((s->pageentry_u8[pageentry_index / 8] >> (pageentry_index % 8)) & 1)
> -	return -1;
> -#else
> -    lseek(s->fd, bitmap_offset + (pageentry_index / 8), SEEK_SET);
> -
> -    read(s->fd, &bitmap_entry, 1);
> -
> -    if ((bitmap_entry >> (pageentry_index % 8)) & 1)
> -	return -1; // not allocated
> -#endif
> -#endif
> -
>       return block_offset;
>   }
>   

Re: [Qemu-devel] [Qemu-stable] [PATCH] block/vpc: fix get_sector_offset for vpc_read

[Peter Lieven]

Am 23.02.2015 um 18:10 schrieb Max Reitz:
> On 2015-02-23 at 10:01, Peter Lieven wrote:
>> VHD images contain a bitmap at the beginning of each data block
>> to indicate the allocation status of each sector in the data block.
>>
>> vpc_read currently checks the allocation status of the first sector
>> in a data block it is going to read and then assumes the same allocation
>> status for the whole remainder of the data block. This might end up
>> in incorrect zero sectors if the first sector is unallocated.
>>
>> To fix this indiciate a sector is allocated as soon as its
>> data block has a valid page table index.
>
> Hm, as far as I can see, this patch just removes unused code. How does this correspond to the commit message?

Oh, you are right. I was somehow believing that the #if 0 was just disabling the cache.

Please ignore this one.

Peter

>
> Max
>
>> CC: qemu-stable@nongnu.org
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   block/vpc.c |   32 --------------------------------
>>   1 file changed, 32 deletions(-)
>>
>> diff --git a/block/vpc.c b/block/vpc.c
>> index 46803b1..9a75334 100644
>> --- a/block/vpc.c
>> +++ b/block/vpc.c
>> @@ -376,38 +376,6 @@ static inline int64_t get_sector_offset(BlockDriverState *bs,
>>           bdrv_pwrite_sync(bs->file, bitmap_offset, bitmap, s->bitmap_size);
>>       }
>>   -//    printf("sector: %" PRIx64 ", index: %x, offset: %x, bioff: %" PRIx64 ", bloff: %" PRIx64 "\n",
>> -//    sector_num, pagetable_index, pageentry_index,
>> -//    bitmap_offset, block_offset);
>> -
>> -// disabled by reason
>> -#if 0
>> -#ifdef CACHE
>> -    if (bitmap_offset != s->last_bitmap)
>> -    {
>> -    lseek(s->fd, bitmap_offset, SEEK_SET);
>> -
>> -    s->last_bitmap = bitmap_offset;
>> -
>> -    // Scary! Bitmap is stored as big endian 32bit entries,
>> -    // while we used to look it up byte by byte
>> -    read(s->fd, s->pageentry_u8, 512);
>> -    for (i = 0; i < 128; i++)
>> -        be32_to_cpus(&s->pageentry_u32[i]);
>> -    }
>> -
>> -    if ((s->pageentry_u8[pageentry_index / 8] >> (pageentry_index % 8)) & 1)
>> -    return -1;
>> -#else
>> -    lseek(s->fd, bitmap_offset + (pageentry_index / 8), SEEK_SET);
>> -
>> -    read(s->fd, &bitmap_entry, 1);
>> -
>> -    if ((bitmap_entry >> (pageentry_index % 8)) & 1)
>> -    return -1; // not allocated
>> -#endif
>> -#endif
>> -
>>       return block_offset;
>>   }
>
>


-- 

Mit freundlichen Grüßen

Peter Lieven

...........................................................

   KAMP Netzwerkdienste GmbH
   Vestische Str. 89-91 | 46117 Oberhausen
   Tel: +49 (0) 208.89 402-50 | Fax: +49 (0) 208.89 402-40
   pl@kamp.de | http://www.kamp.de

   Geschäftsführer: Heiner Lante | Michael Lante
   Amtsgericht Duisburg | HRB Nr. 12154
   USt-Id-Nr.: DE 120607556

...........................................................