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 <address@hidden>
> > > > CC: Jeff Cody <address@hidden>
> > > > CC: Kevin Wolf <address@hidden>
> > > > CC: Stefan Hajnoczi <address@hidden>
> > > > ---
> > > >   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 <address@hidden>

Thank you :)



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