[Qemu-devel] [PATCH v2 0/4] block/parallels: 2TB+ parallels images support

[Qemu-devel] [PATCH v2 0/4] block/parallels: 2TB+ parallels images support

[Denis V. Lunev]

Parallels has released in the recent updates of Parallels Server 5/6
new addition to his image format. Images with signature WithouFreSpacExt
have offsets in the catalog coded not as offsets in sectors (multiple
of 512 bytes) but offsets coded in blocks (i.e. header->tracks * 512)

In this case to code the virtual disk size for such images nb_sectors
field is extended to 64 bits. The reader of older images with signature
WithoutFreeSpace must manually zero most valuable bits of nb_sectors
on open.

Changes from v1:
- fixed message in patch 1
- added braces to conform qemu coding style in patches 3 & 4
- added check for ph.tracks in patch 4 to avoid offset overflow as suggested
  by Jeff

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Jeff Cody <jcody@redhat.com>

[Qemu-devel] [PATCH v2 1/4] parallels: extend parallels format header with actual data values

[Denis V. Lunev]

Parallels image format has several additional fields inside:
- nb_sectors is actually 64 bit wide. Upper 32bits are not used for
  images with signature "WithoutFreeSpace" and must be explicitly
  zeroed according to Parallels. They will be used for images with
  signature "WithouFreSpacExt"
- inuse is magic which means that the image is currently opened for
  read/write or was not closed correctly, the magic is 0x746f6e59
- data_off is the location of the first data block. It can be zero
  and in this case data starts just beyond the header aligned to
  512 bytes. Though this field does not matter for read-only driver

This patch adds these values to struct parallels_header and adds
proper handling of nb_sectors for currently supported WithoutFreeSpace
images.

WithouFreSpacExt will be covered in next patches.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Jeff Cody <jcody@redhat.com>
---
 block/parallels.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 1a5bd35..ffffe39 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -41,8 +41,10 @@ struct parallels_header {
     uint32_t cylinders;
     uint32_t tracks;
     uint32_t catalog_entries;
-    uint32_t nb_sectors;
-    char padding[24];
+    uint64_t nb_sectors;
+    uint32_t inuse;
+    uint32_t data_off;
+    char padding[12];
 } QEMU_PACKED;
 
 typedef struct BDRVParallelsState {
@@ -90,7 +92,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
-    bs->total_sectors = le32_to_cpu(ph.nb_sectors);
+    bs->total_sectors = 0xffffffff & le64_to_cpu(ph.nb_sectors);
 
     s->tracks = le32_to_cpu(ph.tracks);
     if (s->tracks == 0) {
-- 
1.9.1

[Qemu-devel] [PATCH v2 2/4] parallels: replace tabs with spaces in block/parallels.c

[Denis V. Lunev]

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Jeff Cody <jcody@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index ffffe39..16d14ad 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -61,11 +61,11 @@ static int parallels_probe(const uint8_t *buf, int buf_size, const char *filenam
     const struct parallels_header *ph = (const void *)buf;
 
     if (buf_size < HEADER_SIZE)
-	return 0;
+        return 0;
 
     if (!memcmp(ph->magic, HEADER_MAGIC, 16) &&
-	(le32_to_cpu(ph->version) == HEADER_VERSION))
-	return 100;
+        (le32_to_cpu(ph->version) == HEADER_VERSION))
+        return 100;
 
     return 0;
 }
@@ -115,7 +115,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     for (i = 0; i < s->catalog_size; i++)
-	le32_to_cpus(&s->catalog_bitmap[i]);
+        le32_to_cpus(&s->catalog_bitmap[i]);
 
     qemu_co_mutex_init(&s->lock);
     return 0;
@@ -135,7 +135,7 @@ static int64_t seek_to_sector(BlockDriverState *bs, int64_t sector_num)
 
     /* not allocated */
     if ((index > s->catalog_size) || (s->catalog_bitmap[index] == 0))
-	return -1;
+        return -1;
     return (uint64_t)(s->catalog_bitmap[index] + offset) * 512;
 }
 
-- 
1.9.1

[Qemu-devel] [PATCH v2 3/4] parallels: split check for parallels format in parallels_open

[Denis V. Lunev]

and rework error path a bit. There is no difference at the moment, but
the code will be definitely shorter when additional processing will
be required for WithouFreSpacExt

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Jeff Cody <jcody@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 16d14ad..466705e 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -85,11 +85,11 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
-    if (memcmp(ph.magic, HEADER_MAGIC, 16) ||
-        (le32_to_cpu(ph.version) != HEADER_VERSION)) {
-        error_setg(errp, "Image not in Parallels format");
-        ret = -EINVAL;
-        goto fail;
+    if (le32_to_cpu(ph.version) != HEADER_VERSION) {
+        goto fail_format;
+    }
+    if (memcmp(ph.magic, HEADER_MAGIC, 16)) {
+        goto fail_format;
     }
 
     bs->total_sectors = 0xffffffff & le64_to_cpu(ph.nb_sectors);
@@ -120,6 +120,9 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     qemu_co_mutex_init(&s->lock);
     return 0;
 
+fail_format:
+    error_setg(errp, "Image not in Parallels format");
+    ret = -EINVAL;
 fail:
     g_free(s->catalog_bitmap);
     return ret;
-- 
1.9.1

[Qemu-devel] [PATCH v2 4/4] parallels: 2TB+ parallels images support

[Denis V. Lunev]

Parallels has released in the recent updates of Parallels Server 5/6
new addition to his image format. Images with signature WithouFreSpacExt
have offsets in the catalog coded not as offsets in sectors (multiple
of 512 bytes) but offsets coded in blocks (i.e. header->tracks * 512)

In this case all 64 bits of header->nb_sectors are used for image size.

This patch implements support of this for qemu-img and also adds specific
check for an incorrect image. Images with block size greater than
INT_MAX/513 are not supported. The biggest available Parallels image
cluster size in the field is 1 Mb. Thus this limit will not hurt
anyone.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Jeff Cody <jcody@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 466705e..4414a9d 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -30,6 +30,7 @@
 /**************************************************************/
 
 #define HEADER_MAGIC "WithoutFreeSpace"
+#define HEADER_MAGIC2 "WithouFreSpacExt"
 #define HEADER_VERSION 2
 #define HEADER_SIZE 64
 
@@ -54,6 +55,8 @@ typedef struct BDRVParallelsState {
     unsigned int catalog_size;
 
     unsigned int tracks;
+
+    unsigned int off_multiplier;
 } BDRVParallelsState;
 
 static int parallels_probe(const uint8_t *buf, int buf_size, const char *filename)
@@ -63,7 +66,8 @@ static int parallels_probe(const uint8_t *buf, int buf_size, const char *filenam
     if (buf_size < HEADER_SIZE)
         return 0;
 
-    if (!memcmp(ph->magic, HEADER_MAGIC, 16) &&
+    if ((!memcmp(ph->magic, HEADER_MAGIC, 16) ||
+        !memcmp(ph->magic, HEADER_MAGIC2, 16)) &&
         (le32_to_cpu(ph->version) == HEADER_VERSION))
         return 100;
 
@@ -85,21 +89,31 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
+    bs->total_sectors = le64_to_cpu(ph.nb_sectors);
+
     if (le32_to_cpu(ph.version) != HEADER_VERSION) {
         goto fail_format;
     }
-    if (memcmp(ph.magic, HEADER_MAGIC, 16)) {
+    if (!memcmp(ph.magic, HEADER_MAGIC, 16)) {
+        s->off_multiplier = 1;
+        bs->total_sectors = 0xffffffff & bs->total_sectors;
+    } else if (!memcmp(ph.magic, HEADER_MAGIC2, 16)) {
+        s->off_multiplier = le32_to_cpu(ph.tracks);
+    } else {
         goto fail_format;
     }
 
-    bs->total_sectors = 0xffffffff & le64_to_cpu(ph.nb_sectors);
-
     s->tracks = le32_to_cpu(ph.tracks);
     if (s->tracks == 0) {
         error_setg(errp, "Invalid image: Zero sectors per track");
         ret = -EINVAL;
         goto fail;
     }
+    if (s->tracks > INT32_MAX/513) {
+        error_setg(errp, "Invalid image: Too big cluster");
+        ret = -EFBIG;
+        goto fail;
+    }
 
     s->catalog_size = le32_to_cpu(ph.catalog_entries);
     if (s->catalog_size > INT_MAX / 4) {
@@ -139,7 +153,8 @@ static int64_t seek_to_sector(BlockDriverState *bs, int64_t sector_num)
     /* not allocated */
     if ((index > s->catalog_size) || (s->catalog_bitmap[index] == 0))
         return -1;
-    return (uint64_t)(s->catalog_bitmap[index] + offset) * 512;
+    return
+        ((uint64_t)s->catalog_bitmap[index] * s->off_multiplier + offset) * 512;
 }
 
 static int parallels_read(BlockDriverState *bs, int64_t sector_num,
-- 
1.9.1

Re: [Qemu-devel] [PATCH v2 0/4] block/parallels: 2TB+ parallels images support

[Denis V. Lunev]

On 28/07/14 20:23, Denis V. Lunev wrote:
> Parallels has released in the recent updates of Parallels Server 5/6
> new addition to his image format. Images with signature WithouFreSpacExt
> have offsets in the catalog coded not as offsets in sectors (multiple
> of 512 bytes) but offsets coded in blocks (i.e. header->tracks * 512)
>
> In this case to code the virtual disk size for such images nb_sectors
> field is extended to 64 bits. The reader of older images with signature
> WithoutFreeSpace must manually zero most valuable bits of nb_sectors
> on open.
>
> Changes from v1:
> - fixed message in patch 1
> - added braces to conform qemu coding style in patches 3 & 4
> - added check for ph.tracks in patch 4 to avoid offset overflow as suggested
>    by Jeff
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Jeff Cody <jcody@redhat.com>
can you pls look/commit. I have some other changes on top of this

Re: [Qemu-devel] [PATCH v2 1/4] parallels: extend parallels format header with actual data values

[Jeff Cody]

On Mon, Jul 28, 2014 at 08:23:52PM +0400, Denis V. Lunev wrote:
> Parallels image format has several additional fields inside:
> - nb_sectors is actually 64 bit wide. Upper 32bits are not used for
>   images with signature "WithoutFreeSpace" and must be explicitly
>   zeroed according to Parallels. They will be used for images with
>   signature "WithouFreSpacExt"
> - inuse is magic which means that the image is currently opened for
>   read/write or was not closed correctly, the magic is 0x746f6e59
> - data_off is the location of the first data block. It can be zero
>   and in this case data starts just beyond the header aligned to
>   512 bytes. Though this field does not matter for read-only driver
> 
> This patch adds these values to struct parallels_header and adds
> proper handling of nb_sectors for currently supported WithoutFreeSpace
> images.
> 
> WithouFreSpacExt will be covered in next patches.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Jeff Cody <jcody@redhat.com>
> ---
>  block/parallels.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/block/parallels.c b/block/parallels.c
> index 1a5bd35..ffffe39 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -41,8 +41,10 @@ struct parallels_header {
>      uint32_t cylinders;
>      uint32_t tracks;
>      uint32_t catalog_entries;
> -    uint32_t nb_sectors;
> -    char padding[24];
> +    uint64_t nb_sectors;
> +    uint32_t inuse;
> +    uint32_t data_off;
> +    char padding[12];
>  } QEMU_PACKED;
>  
>  typedef struct BDRVParallelsState {
> @@ -90,7 +92,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>          goto fail;
>      }
>  
> -    bs->total_sectors = le32_to_cpu(ph.nb_sectors);
> +    bs->total_sectors = 0xffffffff & le64_to_cpu(ph.nb_sectors);
>  
>      s->tracks = le32_to_cpu(ph.tracks);
>      if (s->tracks == 0) {
> -- 
> 1.9.1
> 
>

Reviewed-by: Jeff Cody <jcody@redhat.com>

Re: [Qemu-devel] [PATCH v2 3/4] parallels: split check for parallels format in parallels_open

[Jeff Cody]

On Mon, Jul 28, 2014 at 08:23:54PM +0400, Denis V. Lunev wrote:
> and rework error path a bit. There is no difference at the moment, but
> the code will be definitely shorter when additional processing will
> be required for WithouFreSpacExt
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/block/parallels.c b/block/parallels.c
> index 16d14ad..466705e 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -85,11 +85,11 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>          goto fail;
>      }
>  
> -    if (memcmp(ph.magic, HEADER_MAGIC, 16) ||
> -        (le32_to_cpu(ph.version) != HEADER_VERSION)) {
> -        error_setg(errp, "Image not in Parallels format");
> -        ret = -EINVAL;
> -        goto fail;
> +    if (le32_to_cpu(ph.version) != HEADER_VERSION) {
> +        goto fail_format;
> +    }
> +    if (memcmp(ph.magic, HEADER_MAGIC, 16)) {
> +        goto fail_format;
>      }
>  
>      bs->total_sectors = 0xffffffff & le64_to_cpu(ph.nb_sectors);
> @@ -120,6 +120,9 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>      qemu_co_mutex_init(&s->lock);
>      return 0;
>  
> +fail_format:
> +    error_setg(errp, "Image not in Parallels format");
> +    ret = -EINVAL;
>  fail:
>      g_free(s->catalog_bitmap);
>      return ret;
> -- 
> 1.9.1
> 
>

Reviewed-by: Jeff Cody <jcody@redhat.com>

Re: [Qemu-devel] [PATCH v2 4/4] parallels: 2TB+ parallels images support

[Jeff Cody]

On Mon, Jul 28, 2014 at 08:23:55PM +0400, Denis V. Lunev wrote:
> Parallels has released in the recent updates of Parallels Server 5/6
> new addition to his image format. Images with signature WithouFreSpacExt
> have offsets in the catalog coded not as offsets in sectors (multiple
> of 512 bytes) but offsets coded in blocks (i.e. header->tracks * 512)
> 
> In this case all 64 bits of header->nb_sectors are used for image size.
> 
> This patch implements support of this for qemu-img and also adds specific
> check for an incorrect image. Images with block size greater than
> INT_MAX/513 are not supported. The biggest available Parallels image
> cluster size in the field is 1 Mb. Thus this limit will not hurt
> anyone.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/block/parallels.c b/block/parallels.c
> index 466705e..4414a9d 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -30,6 +30,7 @@
>  /**************************************************************/
>  
>  #define HEADER_MAGIC "WithoutFreeSpace"
> +#define HEADER_MAGIC2 "WithouFreSpacExt"
>  #define HEADER_VERSION 2
>  #define HEADER_SIZE 64
>  
> @@ -54,6 +55,8 @@ typedef struct BDRVParallelsState {
>      unsigned int catalog_size;
>  
>      unsigned int tracks;
> +
> +    unsigned int off_multiplier;
>  } BDRVParallelsState;
>  
>  static int parallels_probe(const uint8_t *buf, int buf_size, const char *filename)
> @@ -63,7 +66,8 @@ static int parallels_probe(const uint8_t *buf, int buf_size, const char *filenam
>      if (buf_size < HEADER_SIZE)
>          return 0;
>  
> -    if (!memcmp(ph->magic, HEADER_MAGIC, 16) &&
> +    if ((!memcmp(ph->magic, HEADER_MAGIC, 16) ||
> +        !memcmp(ph->magic, HEADER_MAGIC2, 16)) &&
>          (le32_to_cpu(ph->version) == HEADER_VERSION))
>          return 100;
>  
> @@ -85,21 +89,31 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>          goto fail;
>      }
>  
> +    bs->total_sectors = le64_to_cpu(ph.nb_sectors);
> +
>      if (le32_to_cpu(ph.version) != HEADER_VERSION) {
>          goto fail_format;
>      }
> -    if (memcmp(ph.magic, HEADER_MAGIC, 16)) {
> +    if (!memcmp(ph.magic, HEADER_MAGIC, 16)) {
> +        s->off_multiplier = 1;
> +        bs->total_sectors = 0xffffffff & bs->total_sectors;
> +    } else if (!memcmp(ph.magic, HEADER_MAGIC2, 16)) {
> +        s->off_multiplier = le32_to_cpu(ph.tracks);
> +    } else {
>          goto fail_format;
>      }
>  
> -    bs->total_sectors = 0xffffffff & le64_to_cpu(ph.nb_sectors);
> -
>      s->tracks = le32_to_cpu(ph.tracks);
>      if (s->tracks == 0) {
>          error_setg(errp, "Invalid image: Zero sectors per track");
>          ret = -EINVAL;
>          goto fail;
>      }
> +    if (s->tracks > INT32_MAX/513) {
> +        error_setg(errp, "Invalid image: Too big cluster");
> +        ret = -EFBIG;
> +        goto fail;
> +    }
>  
>      s->catalog_size = le32_to_cpu(ph.catalog_entries);
>      if (s->catalog_size > INT_MAX / 4) {
> @@ -139,7 +153,8 @@ static int64_t seek_to_sector(BlockDriverState *bs, int64_t sector_num)
>      /* not allocated */
>      if ((index > s->catalog_size) || (s->catalog_bitmap[index] == 0))
>          return -1;
> -    return (uint64_t)(s->catalog_bitmap[index] + offset) * 512;
> +    return
> +        ((uint64_t)s->catalog_bitmap[index] * s->off_multiplier + offset) * 512;

This still does a cast to uint_64_t, instead of int64_t; not sure it
really matters in practice, as we should be safe now from exceeding an
int64_t value in the end result.

>  }
>  
>  static int parallels_read(BlockDriverState *bs, int64_t sector_num,
> -- 
> 1.9.1
> 
> 

Re: [Qemu-devel] [PATCH v2 4/4] parallels: 2TB+ parallels images support

[Denis V. Lunev]

On 07/08/14 18:39, Jeff Cody wrote:
> On Mon, Jul 28, 2014 at 08:23:55PM +0400, Denis V. Lunev wrote:
>> Parallels has released in the recent updates of Parallels Server 5/6
>> new addition to his image format. Images with signature WithouFreSpacExt
>> have offsets in the catalog coded not as offsets in sectors (multiple
>> of 512 bytes) but offsets coded in blocks (i.e. header->tracks * 512)
>>
>> In this case all 64 bits of header->nb_sectors are used for image size.
>>
>> This patch implements support of this for qemu-img and also adds specific
>> check for an incorrect image. Images with block size greater than
>> INT_MAX/513 are not supported. The biggest available Parallels image
>> cluster size in the field is 1 Mb. Thus this limit will not hurt
>> anyone.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Jeff Cody <jcody@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>   block/parallels.c | 25 ++++++++++++++++++++-----
>>   1 file changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index 466705e..4414a9d 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -30,6 +30,7 @@
>>   /**************************************************************/
>>   
>>   #define HEADER_MAGIC "WithoutFreeSpace"
>> +#define HEADER_MAGIC2 "WithouFreSpacExt"
>>   #define HEADER_VERSION 2
>>   #define HEADER_SIZE 64
>>   
>> @@ -54,6 +55,8 @@ typedef struct BDRVParallelsState {
>>       unsigned int catalog_size;
>>   
>>       unsigned int tracks;
>> +
>> +    unsigned int off_multiplier;
>>   } BDRVParallelsState;
>>   
>>   static int parallels_probe(const uint8_t *buf, int buf_size, const char *filename)
>> @@ -63,7 +66,8 @@ static int parallels_probe(const uint8_t *buf, int buf_size, const char *filenam
>>       if (buf_size < HEADER_SIZE)
>>           return 0;
>>   
>> -    if (!memcmp(ph->magic, HEADER_MAGIC, 16) &&
>> +    if ((!memcmp(ph->magic, HEADER_MAGIC, 16) ||
>> +        !memcmp(ph->magic, HEADER_MAGIC2, 16)) &&
>>           (le32_to_cpu(ph->version) == HEADER_VERSION))
>>           return 100;
>>   
>> @@ -85,21 +89,31 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>           goto fail;
>>       }
>>   
>> +    bs->total_sectors = le64_to_cpu(ph.nb_sectors);
>> +
>>       if (le32_to_cpu(ph.version) != HEADER_VERSION) {
>>           goto fail_format;
>>       }
>> -    if (memcmp(ph.magic, HEADER_MAGIC, 16)) {
>> +    if (!memcmp(ph.magic, HEADER_MAGIC, 16)) {
>> +        s->off_multiplier = 1;
>> +        bs->total_sectors = 0xffffffff & bs->total_sectors;
>> +    } else if (!memcmp(ph.magic, HEADER_MAGIC2, 16)) {
>> +        s->off_multiplier = le32_to_cpu(ph.tracks);
>> +    } else {
>>           goto fail_format;
>>       }
>>   
>> -    bs->total_sectors = 0xffffffff & le64_to_cpu(ph.nb_sectors);
>> -
>>       s->tracks = le32_to_cpu(ph.tracks);
>>       if (s->tracks == 0) {
>>           error_setg(errp, "Invalid image: Zero sectors per track");
>>           ret = -EINVAL;
>>           goto fail;
>>       }
>> +    if (s->tracks > INT32_MAX/513) {
>> +        error_setg(errp, "Invalid image: Too big cluster");
>> +        ret = -EFBIG;
>> +        goto fail;
>> +    }
>>   
>>       s->catalog_size = le32_to_cpu(ph.catalog_entries);
>>       if (s->catalog_size > INT_MAX / 4) {
>> @@ -139,7 +153,8 @@ static int64_t seek_to_sector(BlockDriverState *bs, int64_t sector_num)
>>       /* not allocated */
>>       if ((index > s->catalog_size) || (s->catalog_bitmap[index] == 0))
>>           return -1;
>> -    return (uint64_t)(s->catalog_bitmap[index] + offset) * 512;
>> +    return
>> +        ((uint64_t)s->catalog_bitmap[index] * s->off_multiplier + offset) * 512;
> This still does a cast to uint_64_t, instead of int64_t; not sure it
> really matters in practice, as we should be safe now from exceeding an
> int64_t value in the end result.

this is safe due to above check for s->tracks > INT32_MAX/513
Actually, original code has exactly the same cast and the situation
is exactly the same before the patch (uint32_t value * 1) and after
the patch (uint32_t * (something < INT32_MAX/513))

Though I can change the cast to int64_t, I do not see much difference.
Should I do this?

>>   }
>>   
>>   static int parallels_read(BlockDriverState *bs, int64_t sector_num,
>> -- 
>> 1.9.1
>>
>>

Re: [Qemu-devel] [PATCH v2 4/4] parallels: 2TB+ parallels images support

[Jeff Cody]

On Thu, Aug 07, 2014 at 07:03:12PM +0400, Denis V. Lunev wrote:
> On 07/08/14 18:39, Jeff Cody wrote:
> >On Mon, Jul 28, 2014 at 08:23:55PM +0400, Denis V. Lunev wrote:
> >>Parallels has released in the recent updates of Parallels Server 5/6
> >>new addition to his image format. Images with signature WithouFreSpacExt
> >>have offsets in the catalog coded not as offsets in sectors (multiple
> >>of 512 bytes) but offsets coded in blocks (i.e. header->tracks * 512)
> >>
> >>In this case all 64 bits of header->nb_sectors are used for image size.
> >>
> >>This patch implements support of this for qemu-img and also adds specific
> >>check for an incorrect image. Images with block size greater than
> >>INT_MAX/513 are not supported. The biggest available Parallels image
> >>cluster size in the field is 1 Mb. Thus this limit will not hurt
> >>anyone.
> >>
> >>Signed-off-by: Denis V. Lunev <den@openvz.org>
> >>CC: Jeff Cody <jcody@redhat.com>
> >>CC: Kevin Wolf <kwolf@redhat.com>
> >>CC: Stefan Hajnoczi <stefanha@redhat.com>
> >>---
> >>  block/parallels.c | 25 ++++++++++++++++++++-----
> >>  1 file changed, 20 insertions(+), 5 deletions(-)
> >>
> >>diff --git a/block/parallels.c b/block/parallels.c
> >>index 466705e..4414a9d 100644
> >>--- a/block/parallels.c
> >>+++ b/block/parallels.c
> >>@@ -30,6 +30,7 @@
> >>  /**************************************************************/
> >>  #define HEADER_MAGIC "WithoutFreeSpace"
> >>+#define HEADER_MAGIC2 "WithouFreSpacExt"
> >>  #define HEADER_VERSION 2
> >>  #define HEADER_SIZE 64
> >>@@ -54,6 +55,8 @@ typedef struct BDRVParallelsState {
> >>      unsigned int catalog_size;
> >>      unsigned int tracks;
> >>+
> >>+    unsigned int off_multiplier;
> >>  } BDRVParallelsState;
> >>  static int parallels_probe(const uint8_t *buf, int buf_size, const char *filename)
> >>@@ -63,7 +66,8 @@ static int parallels_probe(const uint8_t *buf, int buf_size, const char *filenam
> >>      if (buf_size < HEADER_SIZE)
> >>          return 0;
> >>-    if (!memcmp(ph->magic, HEADER_MAGIC, 16) &&
> >>+    if ((!memcmp(ph->magic, HEADER_MAGIC, 16) ||
> >>+        !memcmp(ph->magic, HEADER_MAGIC2, 16)) &&
> >>          (le32_to_cpu(ph->version) == HEADER_VERSION))
> >>          return 100;
> >>@@ -85,21 +89,31 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
> >>          goto fail;
> >>      }
> >>+    bs->total_sectors = le64_to_cpu(ph.nb_sectors);
> >>+
> >>      if (le32_to_cpu(ph.version) != HEADER_VERSION) {
> >>          goto fail_format;
> >>      }
> >>-    if (memcmp(ph.magic, HEADER_MAGIC, 16)) {
> >>+    if (!memcmp(ph.magic, HEADER_MAGIC, 16)) {
> >>+        s->off_multiplier = 1;
> >>+        bs->total_sectors = 0xffffffff & bs->total_sectors;
> >>+    } else if (!memcmp(ph.magic, HEADER_MAGIC2, 16)) {
> >>+        s->off_multiplier = le32_to_cpu(ph.tracks);
> >>+    } else {
> >>          goto fail_format;
> >>      }
> >>-    bs->total_sectors = 0xffffffff & le64_to_cpu(ph.nb_sectors);
> >>-
> >>      s->tracks = le32_to_cpu(ph.tracks);
> >>      if (s->tracks == 0) {
> >>          error_setg(errp, "Invalid image: Zero sectors per track");
> >>          ret = -EINVAL;
> >>          goto fail;
> >>      }
> >>+    if (s->tracks > INT32_MAX/513) {
> >>+        error_setg(errp, "Invalid image: Too big cluster");
> >>+        ret = -EFBIG;
> >>+        goto fail;
> >>+    }
> >>      s->catalog_size = le32_to_cpu(ph.catalog_entries);
> >>      if (s->catalog_size > INT_MAX / 4) {
> >>@@ -139,7 +153,8 @@ static int64_t seek_to_sector(BlockDriverState *bs, int64_t sector_num)
> >>      /* not allocated */
> >>      if ((index > s->catalog_size) || (s->catalog_bitmap[index] == 0))
> >>          return -1;
> >>-    return (uint64_t)(s->catalog_bitmap[index] + offset) * 512;
> >>+    return
> >>+        ((uint64_t)s->catalog_bitmap[index] * s->off_multiplier + offset) * 512;
> >This still does a cast to uint_64_t, instead of int64_t; not sure it
> >really matters in practice, as we should be safe now from exceeding an
> >int64_t value in the end result.
> 
> this is safe due to above check for s->tracks > INT32_MAX/513
> Actually, original code has exactly the same cast and the situation
> is exactly the same before the patch (uint32_t value * 1) and after
> the patch (uint32_t * (something < INT32_MAX/513))
> 
> Though I can change the cast to int64_t, I do not see much difference.
> Should I do this?
>

Right, as I said in practice it should now be safe from exceeding an
int64_t (due to the bounds check on s->tracks). I think it is worth
changing if someone else requests a respin for another reason, but
probably not to do a respin for this on its own.

Another question - do you have any sample images?  If they compress
well (bzip2 does a good job with most blank images) it would be nice
to have a couple of parallels images (e.g. one "WithouFreSpacExt" and
one "WithoutFreeSpace") in the tests sample_images directory.  If you
can provide both types of images, I'll amend test 076 to include them.

I can go ahead and give my R-b for this patch:

Reviewed-by: Jeff Cody <jcody@redhat.com>


> >>  }
> >>  static int parallels_read(BlockDriverState *bs, int64_t sector_num,
> >>-- 
> >>1.9.1
> >>
> >>
> 
> 

Re: [Qemu-devel] [PATCH v2 4/4] parallels: 2TB+ parallels images support

[Denis V. Lunev]

On 07/08/14 19:14, Jeff Cody wrote:
> On Thu, Aug 07, 2014 at 07:03:12PM +0400, Denis V. Lunev wrote:
>> On 07/08/14 18:39, Jeff Cody wrote:
>>> On Mon, Jul 28, 2014 at 08:23:55PM +0400, Denis V. Lunev wrote:
>>>> Parallels has released in the recent updates of Parallels Server 5/6
>>>> new addition to his image format. Images with signature WithouFreSpacExt
>>>> have offsets in the catalog coded not as offsets in sectors (multiple
>>>> of 512 bytes) but offsets coded in blocks (i.e. header->tracks * 512)
>>>>
>>>> In this case all 64 bits of header->nb_sectors are used for image size.
>>>>
>>>> This patch implements support of this for qemu-img and also adds specific
>>>> check for an incorrect image. Images with block size greater than
>>>> INT_MAX/513 are not supported. The biggest available Parallels image
>>>> cluster size in the field is 1 Mb. Thus this limit will not hurt
>>>> anyone.
>>>>
>>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>>> CC: Jeff Cody <jcody@redhat.com>
>>>> CC: Kevin Wolf <kwolf@redhat.com>
>>>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>>>> ---
>>>>   block/parallels.c | 25 ++++++++++++++++++++-----
>>>>   1 file changed, 20 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/block/parallels.c b/block/parallels.c
>>>> index 466705e..4414a9d 100644
>>>> --- a/block/parallels.c
>>>> +++ b/block/parallels.c
>>>> @@ -30,6 +30,7 @@
>>>>   /**************************************************************/
>>>>   #define HEADER_MAGIC "WithoutFreeSpace"
>>>> +#define HEADER_MAGIC2 "WithouFreSpacExt"
>>>>   #define HEADER_VERSION 2
>>>>   #define HEADER_SIZE 64
>>>> @@ -54,6 +55,8 @@ typedef struct BDRVParallelsState {
>>>>       unsigned int catalog_size;
>>>>       unsigned int tracks;
>>>> +
>>>> +    unsigned int off_multiplier;
>>>>   } BDRVParallelsState;
>>>>   static int parallels_probe(const uint8_t *buf, int buf_size, const char *filename)
>>>> @@ -63,7 +66,8 @@ static int parallels_probe(const uint8_t *buf, int buf_size, const char *filenam
>>>>       if (buf_size < HEADER_SIZE)
>>>>           return 0;
>>>> -    if (!memcmp(ph->magic, HEADER_MAGIC, 16) &&
>>>> +    if ((!memcmp(ph->magic, HEADER_MAGIC, 16) ||
>>>> +        !memcmp(ph->magic, HEADER_MAGIC2, 16)) &&
>>>>           (le32_to_cpu(ph->version) == HEADER_VERSION))
>>>>           return 100;
>>>> @@ -85,21 +89,31 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>>>           goto fail;
>>>>       }
>>>> +    bs->total_sectors = le64_to_cpu(ph.nb_sectors);
>>>> +
>>>>       if (le32_to_cpu(ph.version) != HEADER_VERSION) {
>>>>           goto fail_format;
>>>>       }
>>>> -    if (memcmp(ph.magic, HEADER_MAGIC, 16)) {
>>>> +    if (!memcmp(ph.magic, HEADER_MAGIC, 16)) {
>>>> +        s->off_multiplier = 1;
>>>> +        bs->total_sectors = 0xffffffff & bs->total_sectors;
>>>> +    } else if (!memcmp(ph.magic, HEADER_MAGIC2, 16)) {
>>>> +        s->off_multiplier = le32_to_cpu(ph.tracks);
>>>> +    } else {
>>>>           goto fail_format;
>>>>       }
>>>> -    bs->total_sectors = 0xffffffff & le64_to_cpu(ph.nb_sectors);
>>>> -
>>>>       s->tracks = le32_to_cpu(ph.tracks);
>>>>       if (s->tracks == 0) {
>>>>           error_setg(errp, "Invalid image: Zero sectors per track");
>>>>           ret = -EINVAL;
>>>>           goto fail;
>>>>       }
>>>> +    if (s->tracks > INT32_MAX/513) {
>>>> +        error_setg(errp, "Invalid image: Too big cluster");
>>>> +        ret = -EFBIG;
>>>> +        goto fail;
>>>> +    }
>>>>       s->catalog_size = le32_to_cpu(ph.catalog_entries);
>>>>       if (s->catalog_size > INT_MAX / 4) {
>>>> @@ -139,7 +153,8 @@ static int64_t seek_to_sector(BlockDriverState *bs, int64_t sector_num)
>>>>       /* not allocated */
>>>>       if ((index > s->catalog_size) || (s->catalog_bitmap[index] == 0))
>>>>           return -1;
>>>> -    return (uint64_t)(s->catalog_bitmap[index] + offset) * 512;
>>>> +    return
>>>> +        ((uint64_t)s->catalog_bitmap[index] * s->off_multiplier + offset) * 512;
>>> This still does a cast to uint_64_t, instead of int64_t; not sure it
>>> really matters in practice, as we should be safe now from exceeding an
>>> int64_t value in the end result.
>> this is safe due to above check for s->tracks > INT32_MAX/513
>> Actually, original code has exactly the same cast and the situation
>> is exactly the same before the patch (uint32_t value * 1) and after
>> the patch (uint32_t * (something < INT32_MAX/513))
>>
>> Though I can change the cast to int64_t, I do not see much difference.
>> Should I do this?
>>
> Right, as I said in practice it should now be safe from exceeding an
> int64_t (due to the bounds check on s->tracks). I think it is worth
> changing if someone else requests a respin for another reason, but
> probably not to do a respin for this on its own.
>
> Another question - do you have any sample images?  If they compress
> well (bzip2 does a good job with most blank images) it would be nice
> to have a couple of parallels images (e.g. one "WithouFreSpacExt" and
> one "WithoutFreeSpace") in the tests sample_images directory.  If you
> can provide both types of images, I'll amend test 076 to include them.
>
> I can go ahead and give my R-b for this patch:
no prob, I have access to them. I'll provide a collection within next 
series.

There is some other incompatible stuff add...

> Reviewed-by: Jeff Cody <jcody@redhat.com>

Thank you :)



>
>>>>   }
>>>>   static int parallels_read(BlockDriverState *bs, int64_t sector_num,
>>>> -- 
>>>> 1.9.1
>>>>
>>>>
>>

Re: [Qemu-devel] [PATCH v2 4/4] parallels: 2TB+ parallels images support

[Denis V. Lunev]

On 07/08/14 19:22, Denis V. Lunev wrote:
> Another question - do you have any sample images?  If they compress
>> well (bzip2 does a good job with most blank images) it would be nice
>> to have a couple of parallels images (e.g. one "WithouFreSpacExt" and
>> one "WithoutFreeSpace") in the tests sample_images directory. If you
>> can provide both types of images, I'll amend test 076 to include them.
>>
>> I can go ahead and give my R-b for this patch:
> no prob, I have access to them. I'll provide a collection within next 
> series.
>
> There is some other incompatible stuff add...

/stuff add.../stuff to add.../

Re: [Qemu-devel] [PATCH v2 0/4] block/parallels: 2TB+ parallels images support

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1097 bytes --]

On Mon, Jul 28, 2014 at 08:23:51PM +0400, Denis V. Lunev wrote:
> Parallels has released in the recent updates of Parallels Server 5/6
> new addition to his image format. Images with signature WithouFreSpacExt
> have offsets in the catalog coded not as offsets in sectors (multiple
> of 512 bytes) but offsets coded in blocks (i.e. header->tracks * 512)
> 
> In this case to code the virtual disk size for such images nb_sectors
> field is extended to 64 bits. The reader of older images with signature
> WithoutFreeSpace must manually zero most valuable bits of nb_sectors
> on open.
> 
> Changes from v1:
> - fixed message in patch 1
> - added braces to conform qemu coding style in patches 3 & 4
> - added check for ph.tracks in patch 4 to avoid offset overflow as suggested
>   by Jeff
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Jeff Cody <jcody@redhat.com>

Sorry for the delay.

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]