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

[Qemu-devel] [PATCH 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.

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

[Qemu-devel] [PATCH 1/4] block/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 explicitely
  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

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 the next patch.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@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..c44df87 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 = (uint32_t)le64_to_cpu(ph.nb_sectors);
 
     s->tracks = le32_to_cpu(ph.tracks);
     if (s->tracks == 0) {
-- 
1.9.1

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

[Denis V. Lunev]

Signed-off-by: Denis V. Lunev <den@openvz.org>
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 c44df87..8f9ec8a 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 3/4] block/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: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 8f9ec8a..02739cf 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -85,12 +85,10 @@ 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 = (uint32_t)le64_to_cpu(ph.nb_sectors);
 
@@ -120,6 +118,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 4/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 all 64 bits of header->nb_sectors are used for image size.

This patch implements support of this for qemu-img.

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

diff --git a/block/parallels.c b/block/parallels.c
index 02739cf..d9cb04f 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,13 +89,18 @@ 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 = (uint32_t)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 = (uint32_t)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");
@@ -137,7 +146,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 1/4] block/parallels: extend parallels format header with actual data values

[Jeff Cody]

On Tue, Jul 22, 2014 at 05:19:34PM +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 explicitely

s/explicitely/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

I think you may have forgotten to finish this sentence :)

> 
> 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 the next patch.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@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..c44df87 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 = (uint32_t)le64_to_cpu(ph.nb_sectors);

I think an explicit bit mask on the upper 32 bits would fit better
here than a cast, especially since neither 'bs->total_sectors' nor
'ph.nb_sectors' is a uint32_t. E.g.:

    bs->total_sectors = 0xffffffff & le64_to_cpu(ph.nb_sectors);


>  
>      s->tracks = le32_to_cpu(ph.tracks);
>      if (s->tracks == 0) {
> -- 
> 1.9.1
> 
> 

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

[Jeff Cody]

On Tue, Jul 22, 2014 at 05:19:35PM +0400, Denis V. Lunev wrote:
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> 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 c44df87..8f9ec8a 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
> 
>

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

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

[Jeff Cody]

On Tue, Jul 22, 2014 at 05:19:36PM +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: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/block/parallels.c b/block/parallels.c
> index 8f9ec8a..02739cf 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -85,12 +85,10 @@ 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;

QEMU coding style dictates these statements have curly braces, even
though they are just one liners.  (If you run your patches against
scripts/checkpatch.pl, it should catch most style issues).

I think this patch could also just be squashed into patch 4, if
desired.

>  
>      bs->total_sectors = (uint32_t)le64_to_cpu(ph.nb_sectors);
>  
> @@ -120,6 +118,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
> 
> 

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

[Jeff Cody]

On Tue, Jul 22, 2014 at 05:19:37PM +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.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/block/parallels.c b/block/parallels.c
> index 02739cf..d9cb04f 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,13 +89,18 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>          goto fail;
>      }
>  
> +    bs->total_sectors = le64_to_cpu(ph.nb_sectors);
> +

bs->total_sectors is a int64_t, and ph.nb_sectors is a uint64_t.
Should we do a sanity check here on the max number of 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 = (uint32_t)bs->total_sectors;

(same comment as in patch 1, w.r.t. cast vs. bitmask)

> +    } else if (!memcmp(ph.magic, HEADER_MAGIC2, 16))
> +        s->off_multiplier = le32_to_cpu(ph.tracks);

Is there a maximum size in the specification for ph.tracks?


> +    else
>          goto fail_format;

(same comment as the last patch, w.r.t. braces)

>  
> -    bs->total_sectors = (uint32_t)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");
> @@ -137,7 +146,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;

Do we need to worry about overflow here, depending on the value of
off_multiplier?

Also, (and this existed prior to this patch), - we are casting to
uint64_t, although the function returns int64_t.

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

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

[Denis V. Lunev]

On 24/07/14 22:34, Jeff Cody wrote:
> On Tue, Jul 22, 2014 at 05:19:34PM +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 explicitely
> s/explicitely/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
> I think you may have forgotten to finish this sentence :)
ok
>
>> 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 the next patch.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> CC: Stefan Hajnoczi <stefanha@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..c44df87 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 = (uint32_t)le64_to_cpu(ph.nb_sectors);
> I think an explicit bit mask on the upper 32 bits would fit better
> here than a cast, especially since neither 'bs->total_sectors' nor
> 'ph.nb_sectors' is a uint32_t. E.g.:
>
>      bs->total_sectors = 0xffffffff & le64_to_cpu(ph.nb_sectors);
>
ok, will do

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

[Denis V. Lunev]

On 24/07/14 22:50, Jeff Cody wrote:
> On Tue, Jul 22, 2014 at 05:19:36PM +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: Kevin Wolf <kwolf@redhat.com>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>   block/parallels.c | 13 +++++++------
>>   1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index 8f9ec8a..02739cf 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -85,12 +85,10 @@ 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;
> QEMU coding style dictates these statements have curly braces, even
> though they are just one liners.  (If you run your patches against
> scripts/checkpatch.pl, it should catch most style issues).
ok, I just used a kernel convention. Will redo this here and in the
next path. This is not a problem :) Thank you for pointing this
out.

> I think this patch could also just be squashed into patch 4, if
> desired.
I'd prefer not to do this to have behavior changing and behavior
non-changing stuff separated.

>>   
>>       bs->total_sectors = (uint32_t)le64_to_cpu(ph.nb_sectors);
>>   
>> @@ -120,6 +118,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
>>
>>

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

[Denis V. Lunev]

On 24/07/14 23:25, Jeff Cody wrote:
> On Tue, Jul 22, 2014 at 05:19:37PM +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.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>   block/parallels.c | 20 +++++++++++++++-----
>>   1 file changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index 02739cf..d9cb04f 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,13 +89,18 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>           goto fail;
>>       }
>>   
>> +    bs->total_sectors = le64_to_cpu(ph.nb_sectors);
>> +
> bs->total_sectors is a int64_t, and ph.nb_sectors is a uint64_t.
> Should we do a sanity check here on the max number of sectors?
in reality such image will not be usable as we will later have to multiply
this count with 512 to calculate offset in the file
>>       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 = (uint32_t)bs->total_sectors;
> (same comment as in patch 1, w.r.t. cast vs. bitmask)
ok
>> +    } else if (!memcmp(ph.magic, HEADER_MAGIC2, 16))
>> +        s->off_multiplier = le32_to_cpu(ph.tracks);
> Is there a maximum size in the specification for ph.tracks?
no, there is no limitation. Though in reality I have seen the
following images only:
   252 sectors, 63 sectors, 256 sectors, 2048 sectors
and only 2048 was used with this new header.

This is just an observation.

>> +    else
>>           goto fail_format;
> (same comment as the last patch, w.r.t. braces)
>
>>   
>> -    bs->total_sectors = (uint32_t)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");
>> @@ -137,7 +146,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;
> Do we need to worry about overflow here, depending on the value of
> off_multiplier?
>
> Also, (and this existed prior to this patch), - we are casting to
> uint64_t, although the function returns int64_t.
good catch. I'll change the declaration to uint64_t and will return 0
from previous if aka

if ((index > s->catalog_size) || (s->catalog_bitmap[index] == 0))

It is not possible to obtain 0 as a real offset of any sector as
this is an offset of the header.

Regarding overflow check. Do you think that we should return
error to the upper layer or we could silently fill result data
with zeroes as was done originally for the following case
'index > s->catalog_size' ?
>>   }
>>   
>>   static int parallels_read(BlockDriverState *bs, int64_t sector_num,
>> -- 
>> 1.9.1
>>
>>

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

[Jeff Cody]

On Fri, Jul 25, 2014 at 07:51:47AM +0400, Denis V. Lunev wrote:
> On 24/07/14 23:25, Jeff Cody wrote:
> >On Tue, Jul 22, 2014 at 05:19:37PM +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.
> >>
> >>Signed-off-by: Denis V. Lunev <den@openvz.org>
> >>CC: Kevin Wolf <kwolf@redhat.com>
> >>CC: Stefan Hajnoczi <stefanha@redhat.com>
> >>---
> >>  block/parallels.c | 20 +++++++++++++++-----
> >>  1 file changed, 15 insertions(+), 5 deletions(-)
> >>
> >>diff --git a/block/parallels.c b/block/parallels.c
> >>index 02739cf..d9cb04f 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,13 +89,18 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
> >>          goto fail;
> >>      }
> >>+    bs->total_sectors = le64_to_cpu(ph.nb_sectors);
> >>+
> >bs->total_sectors is a int64_t, and ph.nb_sectors is a uint64_t.
> >Should we do a sanity check here on the max number of sectors?
> in reality such image will not be usable as we will later have to multiply
> this count with 512 to calculate offset in the file
> >>      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 = (uint32_t)bs->total_sectors;
> >(same comment as in patch 1, w.r.t. cast vs. bitmask)
> ok
> >>+    } else if (!memcmp(ph.magic, HEADER_MAGIC2, 16))
> >>+        s->off_multiplier = le32_to_cpu(ph.tracks);
> >Is there a maximum size in the specification for ph.tracks?
> no, there is no limitation. Though in reality I have seen the
> following images only:
>   252 sectors, 63 sectors, 256 sectors, 2048 sectors
> and only 2048 was used with this new header.
> 
> This is just an observation.
> 
> >>+    else
> >>          goto fail_format;
> >(same comment as the last patch, w.r.t. braces)
> >
> >>-    bs->total_sectors = (uint32_t)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");
> >>@@ -137,7 +146,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;
> >Do we need to worry about overflow here, depending on the value of
> >off_multiplier?
> >
> >Also, (and this existed prior to this patch), - we are casting to
> >uint64_t, although the function returns int64_t.
> good catch. I'll change the declaration to uint64_t and will return 0
> from previous if aka
>

Thanks.

I'm not sure that is the best option though - the only place the
return value from this function is used is in parallels_read(), which
passes the output into bdrv_pread() as the offset.  The offset
parameter for bdrv_pread/pwrite is an int64_t, not a uint64_t.


> if ((index > s->catalog_size) || (s->catalog_bitmap[index] == 0))
> 
> It is not possible to obtain 0 as a real offset of any sector as
> this is an offset of the header.
> 
> Regarding overflow check. Do you think that we should return
> error to the upper layer or we could silently fill result data
> with zeroes as was done originally for the following case
> 'index > s->catalog_size' ?

I think it would be best to return error (anything < 0 will also cause
bdrv_pread to return -EIO, and it is also checked for in
parallels_read).

I also don't mean to over-complicate things here for you, which is why
I asked about the max value of ph.tracks.  If that has a reasonable
bounds check, then we don't need to worry about overflow at all, and
can just change the cast to an int64_t instead of uint64_t.  

I think we are safe so long as ph.tracks <= (INT32_MAX/513), and it
that seems like it would be a reasonably flexible bounds limit check
when first opening the image (since QEMU can't really support higher
than that, anyway).

Thanks,
Jeff

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

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

[Denis V. Lunev]

On 25/07/14 17:08, Jeff Cody wrote:
> On Fri, Jul 25, 2014 at 07:51:47AM +0400, Denis V. Lunev wrote:
>> On 24/07/14 23:25, Jeff Cody wrote:
>>> On Tue, Jul 22, 2014 at 05:19:37PM +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.
>>>>
>>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>>> CC: Kevin Wolf <kwolf@redhat.com>
>>>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>>>> ---
>>>>   block/parallels.c | 20 +++++++++++++++-----
>>>>   1 file changed, 15 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/block/parallels.c b/block/parallels.c
>>>> index 02739cf..d9cb04f 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,13 +89,18 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>>>           goto fail;
>>>>       }
>>>> +    bs->total_sectors = le64_to_cpu(ph.nb_sectors);
>>>> +
>>> bs->total_sectors is a int64_t, and ph.nb_sectors is a uint64_t.
>>> Should we do a sanity check here on the max number of sectors?
>> in reality such image will not be usable as we will later have to multiply
>> this count with 512 to calculate offset in the file
>>>>       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 = (uint32_t)bs->total_sectors;
>>> (same comment as in patch 1, w.r.t. cast vs. bitmask)
>> ok
>>>> +    } else if (!memcmp(ph.magic, HEADER_MAGIC2, 16))
>>>> +        s->off_multiplier = le32_to_cpu(ph.tracks);
>>> Is there a maximum size in the specification for ph.tracks?
>> no, there is no limitation. Though in reality I have seen the
>> following images only:
>>    252 sectors, 63 sectors, 256 sectors, 2048 sectors
>> and only 2048 was used with this new header.
>>
>> This is just an observation.
>>
>>>> +    else
>>>>           goto fail_format;
>>> (same comment as the last patch, w.r.t. braces)
>>>
>>>> -    bs->total_sectors = (uint32_t)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");
>>>> @@ -137,7 +146,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;
>>> Do we need to worry about overflow here, depending on the value of
>>> off_multiplier?
>>>
>>> Also, (and this existed prior to this patch), - we are casting to
>>> uint64_t, although the function returns int64_t.
>> good catch. I'll change the declaration to uint64_t and will return 0
>> from previous if aka
>>
> Thanks.
>
> I'm not sure that is the best option though - the only place the
> return value from this function is used is in parallels_read(), which
> passes the output into bdrv_pread() as the offset.  The offset
> parameter for bdrv_pread/pwrite is an int64_t, not a uint64_t.
>
>
>> if ((index > s->catalog_size) || (s->catalog_bitmap[index] == 0))
>>
>> It is not possible to obtain 0 as a real offset of any sector as
>> this is an offset of the header.
>>
>> Regarding overflow check. Do you think that we should return
>> error to the upper layer or we could silently fill result data
>> with zeroes as was done originally for the following case
>> 'index > s->catalog_size' ?
> I think it would be best to return error (anything < 0 will also cause
> bdrv_pread to return -EIO, and it is also checked for in
> parallels_read).
>
> I also don't mean to over-complicate things here for you, which is why
> I asked about the max value of ph.tracks.  If that has a reasonable
> bounds check, then we don't need to worry about overflow at all, and
> can just change the cast to an int64_t instead of uint64_t.
>
> I think we are safe so long as ph.tracks <= (INT32_MAX/513), and it
> that seems like it would be a reasonably flexible bounds limit check
> when first opening the image (since QEMU can't really support higher
> than

sounds good to me

Thank you for an idea :)

> that, anyway).
>
> Thanks,
> Jeff
>
>>>>   }
>>>>   static int parallels_read(BlockDriverState *bs, int64_t sector_num,
>>>> -- 
>>>> 1.9.1
>>>>
>>>>