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

[Jeff Cody <jcody@redhat.com>]

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
> 
>