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

["Denis V. Lunev" <den@openvz.org>]

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