From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55881) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XAfHd-0000Gp-5l for qemu-devel@nongnu.org; Fri, 25 Jul 2014 09:11:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XAfHX-0003pC-Ax for qemu-devel@nongnu.org; Fri, 25 Jul 2014 09:11:37 -0400 Received: from relay.parallels.com ([195.214.232.42]:34928) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XAfHW-0003p5-UU for qemu-devel@nongnu.org; Fri, 25 Jul 2014 09:11:31 -0400 Message-ID: <53D257B6.4000303@parallels.com> Date: Fri, 25 Jul 2014 17:12:22 +0400 From: "Denis V. Lunev" MIME-Version: 1.0 References: <1406035177-221890-1-git-send-email-den@openvz.org> <1406035177-221890-5-git-send-email-den@openvz.org> <20140724192538.GD24801@localhost.localdomain> <53D1D453.2040105@parallels.com> <20140725130801.GA8019@localhost.localdomain> In-Reply-To: <20140725130801.GA8019@localhost.localdomain> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 4/4] block/parallels: 2TB+ parallels images support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jeff Cody Cc: Kevin Wolf , "Denis V. Lunev" , qemu-devel@nongnu.org, Stefan Hajnoczi 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 >>>> CC: Kevin Wolf >>>> CC: Stefan Hajnoczi >>>> --- >>>> 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 >>>> >>>>