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

["Denis V. Lunev" <den@parallels.com>]

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