From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42171) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XFPVE-0003wg-8Q for qemu-devel@nongnu.org; Thu, 07 Aug 2014 11:21:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XFPV9-0004C3-8z for qemu-devel@nongnu.org; Thu, 07 Aug 2014 11:21:16 -0400 Received: from relay.parallels.com ([195.214.232.42]:35498) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XFPV8-00047D-SJ for qemu-devel@nongnu.org; Thu, 07 Aug 2014 11:21:11 -0400 Message-ID: <53E399C1.8040504@openvz.org> Date: Thu, 7 Aug 2014 19:22:41 +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> <53E39530.4060300@openvz.org> <20140807151404.GD18633@localhost.localdomain> In-Reply-To: <20140807151404.GD18633@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 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 >>>> 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? >> > 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 Thank you :) > >>>> } >>>> static int parallels_read(BlockDriverState *bs, int64_t sector_num, >>>> -- >>>> 1.9.1 >>>> >>>> >>