From patchwork Tue Jul 26 20:26:38 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Serge E. Hallyn" X-Patchwork-Id: 106935 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [140.186.70.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id B43ECB6F87 for ; Wed, 27 Jul 2011 06:26:23 +1000 (EST) Received: from localhost ([::1]:59466 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QloCn-0000ML-1c for incoming@patchwork.ozlabs.org; Tue, 26 Jul 2011 16:26:17 -0400 Received: from eggs.gnu.org ([140.186.70.92]:60692) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QloCh-0000M5-Ju for qemu-devel@nongnu.org; Tue, 26 Jul 2011 16:26:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QloCg-0004xN-81 for qemu-devel@nongnu.org; Tue, 26 Jul 2011 16:26:11 -0400 Received: from 50-56-35-84.static.cloud-ips.com ([50.56.35.84]:48379 helo=mail) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QloCg-0004xJ-4M for qemu-devel@nongnu.org; Tue, 26 Jul 2011 16:26:10 -0400 Received: by mail (Postfix, from userid 1000) id 9E3D4100EE4; Tue, 26 Jul 2011 20:26:38 +0000 (UTC) Date: Tue, 26 Jul 2011 20:26:38 +0000 From: "Serge E. Hallyn" To: Kevin Wolf Message-ID: <20110726202638.GA11351@hallyn.com> References: <20110725183435.GA26649@hallyn.com> <4E2E8255.8010908@redhat.com> <20110726160830.GA32510@hallyn.com> <4E2EE953.2060005@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <4E2EE953.2060005@redhat.com> User-Agent: Mutt/1.5.20 (2009-06-14) X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 50.56.35.84 Cc: qemu-devel@nongnu.org Subject: Re: [Qemu-devel] [PATCH 1/1] block/vpc.c: Detect too-large vpc file X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Quoting Kevin Wolf (kwolf@redhat.com): > Am 26.07.2011 18:08, schrieb Serge E. Hallyn: > > Quoting Kevin Wolf (kwolf@redhat.com): > >> Am 25.07.2011 20:34, schrieb Serge E. Hallyn: > >>> VHD files technically can be up to 2Tb, but virtual pc is limited > >>> to 127G. Currently qemu-img refused to create vpc files > 127G, > >>> but it is failing to return error when converting from a non-vpc > >>> VHD file which is >127G. It returns success, but creates a truncated > >>> converted image. Also, qemu-img info claims the vpc file is 127G > >>> (and clean). > >>> > >>> This patch detects a too-large vpc file and returns -EFBIG. Without > >>> this patch, > >>> > >>> ============================================================= > >>> root@ip-10-38-123-242:~/qemu-fixed# qemu-img info /mnt/140g-dynamic.vhd > >>> image: /mnt/140g-dynamic.vhd > >>> file format: vpc > >>> virtual size: 127G (136899993600 bytes) > >>> disk size: 284K > >>> root@ip-10-38-123-242:~/qemu-fixed# qemu-img convert -f vpc -O raw /mnt/140g-dynamic.vhd /mnt/y > >>> root@ip-10-38-123-242:~/qemu-fixed# echo $? > >>> 0 > >>> root@ip-10-38-123-242:~/qemu-fixed# qemu-img info /mnt/y > >>> image: /mnt/y > >>> file format: raw > >>> virtual size: 127G (136899993600 bytes) > >>> disk size: 0 > >>> ============================================================= > >>> > >>> (The 140G image was truncated with no warning or error.) > >>> > >>> With the patch, I get: > >>> > >>> ============================================================= > >>> root@ip-10-38-123-242:~/qemu-fixed# ./qemu-img info /mnt/140g-dynamic.vhd > >>> qemu-img: Could not open '/mnt/140g-dynamic.vhd': File too large > >>> root@ip-10-38-123-242:~/qemu-fixed# ./qemu-img convert -f vpc -O raw /mnt/140g-dynamic.vhd /mnt/y > >>> qemu-img: Could not open '/mnt/140g-dynamic.vhd': File too large > >>> qemu-img: Could not open '/mnt/140g-dynamic.vhd' > >>> ============================================================= > >>> > >>> See https://bugs.launchpad.net/qemu/+bug/814222 for details. > >>> > >>> Signed-off-by: Serge Hallyn > >>> --- > >>> block/vpc.c | 8 +++++++- > >>> 1 files changed, 7 insertions(+), 1 deletions(-) > >>> > >>> diff --git a/block/vpc.c b/block/vpc.c > >>> index 56865da..fdd5236 100644 > >>> --- a/block/vpc.c > >>> +++ b/block/vpc.c > >>> @@ -156,6 +156,7 @@ static int vpc_open(BlockDriverState *bs, int flags) > >>> struct vhd_dyndisk_header* dyndisk_header; > >>> uint8_t buf[HEADER_SIZE]; > >>> uint32_t checksum; > >>> + int err = -1; > >>> > >>> if (bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE) != HEADER_SIZE) > >>> goto fail; > >>> @@ -176,6 +177,11 @@ static int vpc_open(BlockDriverState *bs, int flags) > >>> bs->total_sectors = (int64_t) > >>> be16_to_cpu(footer->cyls) * footer->heads * footer->secs_per_cyl; > >>> > >>> + if (bs->total_sectors >= 65535 * 16 * 255) { > >>> + err = -EFBIG; > >>> + goto fail; > >>> + } > >> > >> I wonder why this works. If bs->total_sectors was right, shouldn't it > > > > bs->total_sectors is exactly 65535 * 16 * 255. It never gets bigger. > > So really the check could be > > > > if (bs->total_sectors == 65535 * 16 * 255) { > > > > and it would still work. > > Oh, now I understand. I think this is a bit too restrictive as it > forbids the largest possible size. Yeah, it does. > >> have converted the full 140 GB? I can't see where else we would limit it > >> to 127 GB, so what I had expected is that the CHS geometry stored in the > >> image header is already too small. > > > > If I understand you correctly, what you're saying is exactly what I'm > > finding. total_sectors is not reflective of the true size. > > > > I assume the true size is somewhere to be found :) But I haven't found a > > nice spec for these. > > I think that footer->size contains the real size. Maybe we should do > something like this: > if (footer->size > 65536 * 16 * 255 * 512) { > bs->total_sectors = footer->size / 512; > } else { > bs->total_sectors = (int64_t) > be16_to_cpu(footer->cyls) * footer->heads * footer->secs_per_cyl > } The footer->size appears to be double the 'real' size. So I'm actually doing the blow. Does this seem sensible? Doing it this way, trying to convert the image gives me '-EPERM' rather than -EFBIG. Not sure where we are best off catching that. Subject: [PATCH 1/1] vpc: accurately detect file size VHD files technically can be up to 2Tb, but virtual pc uses CHS which is limited to 127G. Currently qemu-img refused to create vpc files > 127G, but it reports any file >= 127G as being 127G. If asked to convert such a file, it creates a 127G (truncated) file without returning error. This patch uses the size reported in the footer to detect whether the size is > 127G, and reports the size stored in footer if so. qemu-img info now reports the correct size. qemu-img convert refuses, but returns -EPERM rather than -EFBIG. See https://bugs.launchpad.net/qemu/+bug/814222 for details. Changelog: follow Kevin Wolf's suggestion for detecting true file size. Signed-off-by: Serge Hallyn --- block/vpc.c | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-) diff --git a/block/vpc.c b/block/vpc.c index 56865da..37eebc2 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -173,8 +173,14 @@ static int vpc_open(BlockDriverState *bs, int flags) // The visible size of a image in Virtual PC depends on the geometry // rather than on the size stored in the footer (the size in the footer // is too large usually) - bs->total_sectors = (int64_t) - be16_to_cpu(footer->cyls) * footer->heads * footer->secs_per_cyl; + // If the size stored in the footer is larger than CHS can represent, + // then use the size stored in the footer. + if (footer->size > (uint64_t) 65536 * 16 * 255 * 2) { + bs->total_sectors = footer->size / 2; + } else { + bs->total_sectors = (int64_t) + be16_to_cpu(footer->cyls) * footer->heads * footer->secs_per_cyl; + } if (bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf, HEADER_SIZE) != HEADER_SIZE)