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

[Jeff Cody <jcody@redhat.com>]

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