From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36411) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XFPCM-00055S-EK for qemu-devel@nongnu.org; Thu, 07 Aug 2014 11:01:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XFPCH-0005aF-Ed for qemu-devel@nongnu.org; Thu, 07 Aug 2014 11:01:46 -0400 Received: from relay.parallels.com ([195.214.232.42]:60294) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XFPCH-0005a8-7j for qemu-devel@nongnu.org; Thu, 07 Aug 2014 11:01:41 -0400 Message-ID: <53E39530.4060300@openvz.org> Date: Thu, 7 Aug 2014 19:03:12 +0400 From: "Denis V. Lunev" MIME-Version: 1.0 References: <1406564635-261591-1-git-send-email-den@openvz.org> <1406564635-261591-5-git-send-email-den@openvz.org> <20140807143921.GC18633@localhost.localdomain> In-Reply-To: <20140807143921.GC18633@localhost.localdomain> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 4/4] parallels: 2TB+ parallels images support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jeff Cody Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi 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 >> CC: Jeff Cody >> CC: Kevin Wolf >> CC: Stefan Hajnoczi >> --- >> 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? >> } >> >> static int parallels_read(BlockDriverState *bs, int64_t sector_num, >> -- >> 1.9.1 >> >>