From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46724) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YQ9JM-0007hO-E9 for qemu-devel@nongnu.org; Tue, 24 Feb 2015 01:49:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YQ9JJ-0006Xs-52 for qemu-devel@nongnu.org; Tue, 24 Feb 2015 01:49:40 -0500 Received: from mx-v6.kamp.de ([2a02:248:0:51::16]:48259 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YQ9JI-0006Xi-Kw for qemu-devel@nongnu.org; Tue, 24 Feb 2015 01:49:37 -0500 Message-ID: <54EC1EFA.8050005@kamp.de> Date: Tue, 24 Feb 2015 07:49:30 +0100 From: Peter Lieven MIME-Version: 1.0 References: <1424701661-21241-1-git-send-email-pl@kamp.de> <1424701661-21241-5-git-send-email-pl@kamp.de> <54EB7887.7020805@redhat.com> In-Reply-To: <54EB7887.7020805@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 4/5] block/vpc: make calculate_geometry spec conform List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-devel@nongnu.org Cc: kwolf@redhat.com, carnold@suse.com, jcody@redhat.com, stefanha@redhat.com Am 23.02.2015 um 19:59 schrieb Max Reitz: > On 2015-02-23 at 09:27, Peter Lieven wrote: >> The VHD spec [1] allows for total_sectors of 65535 x 16 x 255 (~127GB) >> represented by a CHS geometry. If total_sectors is greater >> than 65535 x 16 x 255 this geometry is set as a maximum. >> >> Qemu, Hyper-V, VirtualBox and disk2vhd use this special geometry as an >> indicator to use the image current size from the footer as disk size. >> >> This patch changes vpc_create to effectively calculate a CxHxS geometry >> for the given image size if possible while rounding up if necessary. >> If the image size is too big to be represented in CHS we set the maximum >> and write the exact requested image size into the footer. >> >> This partly reverts commit 258d2edb, but leaves support for >127G disks >> intact. >> >> [1] http://download.microsoft.com/download/f/f/e/ffef50a5-07dd-4cf8-aaa3-442c0673a029/Virtual%20Hard%20Disk%20Format%20Spec_10_18_06.doc >> >> Signed-off-by: Peter Lieven >> --- >> block/vpc.c | 45 ++++++++++++++++++++++++--------------------- >> 1 file changed, 24 insertions(+), 21 deletions(-) >> >> diff --git a/block/vpc.c b/block/vpc.c >> index 11d3c86..9c5301b 100644 >> --- a/block/vpc.c >> +++ b/block/vpc.c >> @@ -46,6 +46,7 @@ enum vhd_type { >> #define VHD_TIMESTAMP_BASE 946684800 >> #define VHD_MAX_SECTORS (65535LL * 255 * 255) >> +#define VHD_MAX_GEOMETRY (65535LL * 16 * 255) >> // always big-endian >> typedef struct vhd_footer { >> @@ -218,7 +219,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, >> /* Images that have exactly the maximum geometry are probably bigger and >> * would be truncated if we adhered to the geometry for them. Rely on >> * footer->size for them. */ >> - if (bs->total_sectors == 65535ULL * 16 * 255) { >> + if (bs->total_sectors == VHD_MAX_GEOMETRY) { >> bs->total_sectors = be64_to_cpu(footer->size) / BDRV_SECTOR_SIZE; >> } >> @@ -642,26 +643,20 @@ static int calculate_geometry(int64_t total_sectors, uint16_t* cyls, >> { >> uint32_t cyls_times_heads; >> - /* Allow a maximum disk size of approximately 2 TB */ >> - if (total_sectors > 65535LL * 255 * 255) { >> - return -EFBIG; >> - } >> + total_sectors = MIN(total_sectors, VHD_MAX_GEOMETRY); >> - if (total_sectors > 65535 * 16 * 63) { >> + if (total_sectors > 65535LL * 16 * 63) { > > While it is >= in the specification, the "else" branch does work fine with the total_sectors = 65535 * 16 * 63 case. So I'm leaving it up to you whether to make it really really spec-conform or at least functionally spec-conform (although the spec says > 16 heads, 16191 cylinders, and 255 sectors per cylinder for this case, whereas the "else" branch leaves us with 16 heads, 65535 cylinders, and 63 sectors per cylinder). I will adjust it to match the spec exactly. > >> *secs_per_cyl = 255; >> - if (total_sectors > 65535 * 16 * 255) { >> - *heads = 255; >> - } else { >> - *heads = 16; >> - } >> + *heads = 16; >> cyls_times_heads = total_sectors / *secs_per_cyl; >> } else { >> *secs_per_cyl = 17; >> cyls_times_heads = total_sectors / *secs_per_cyl; >> *heads = (cyls_times_heads + 1023) / 1024; >> - if (*heads < 4) >> + if (*heads < 4) { >> *heads = 4; >> + } >> if (cyls_times_heads >= (*heads * 1024) || *heads > 16) { >> *secs_per_cyl = 31; >> @@ -817,19 +812,27 @@ static int vpc_create(const char *filename, QemuOpts *opts, Error **errp) >> * Calculate matching total_size and geometry. Increase the number of >> * sectors requested until we get enough (or fail). This ensures that >> * qemu-img convert doesn't truncate images, but rather rounds up. >> + * >> + * If the image size can't be represented by a spec conform CHS geometry, >> + * we set the geometry to 65535 x 16 x 255 (CxHxS) sectors and use >> + * the image size from the VHD footer to calculate total_sectors. >> */ >> - total_sectors = total_size / BDRV_SECTOR_SIZE; >> + total_sectors = MIN(VHD_MAX_GEOMETRY, >> + DIV_ROUND_UP(total_size, BDRV_SECTOR_SIZE)); > > DIV_ROUND_UP() is unnecessary, total_size is a multiple of BDRV_SECTOR_SIZE anyway. ok > > I find the MIN() unnecessary, too, because calculate_geometry() will take care of that itself. Actually the MIN() is the guarantee that the for-loop terminates for images with total_sectors > VHD_MAX_GEOMETRY. Otherwise (int64_t)cyls * heads * secs_per_cyl would always be VHD_MAX_GEOMETRY which is always smaller than VHD_MAX_GEOMETRY. This would keep us looping for good. > >> for (i = 0; total_sectors > (int64_t)cyls * heads * secs_per_cyl; i++) { >> - if (calculate_geometry(total_sectors + i, &cyls, &heads, >> - &secs_per_cyl)) >> - { >> - ret = -EFBIG; >> - goto out; >> - } >> + calculate_geometry(total_sectors + i, &cyls, &heads, &secs_per_cyl); >> } >> - total_sectors = (int64_t) cyls * heads * secs_per_cyl; >> - total_size = total_sectors * BDRV_SECTOR_SIZE; >> + if ((int64_t)cyls * heads * secs_per_cyl == VHD_MAX_GEOMETRY) { >> + total_sectors = DIV_ROUND_UP(total_size, BDRV_SECTOR_SIZE); > > Again, DIV_ROUND_UP() is unnecessary. > > So this is equivalent to total_sectors = total_size / BDRV_SECTOR_SIZE, which, if you omit the MIN() above, is exactly what total_sectors already contains, so you can just omit this assignment. > >> + /* Allow a maximum disk size of approximately 2 TB */ >> + if (total_sectors > VHD_MAX_SECTORS) { >> + return -EFBIG; > > No goto out;? Oops. > >> + } >> + } else { >> + total_sectors = (int64_t)cyls * heads * secs_per_cyl; >> + total_size = total_sectors * BDRV_SECTOR_SIZE; >> + } > > An alternative way of writing this would be: > > if ((int64_t)cyls * heads * secs_per_cyl < VHD_MAX_GEOMETRY) { > total_sectors = (int64_t)cyls * heads * secs_per_cyl; > } > if (total_sectors > VHD_MAX_SECTORS) { > ret = -EFBIG; > goto out; > } > total_size = total_sectors * BDRV_SECTOR_SIZE; > > Or, alternatively, it'd be possible to drop the total_size assignment and just use total_sectors * BDRV_SECTOR_SIZE in the assignments of footer->orig_size and footer->size. Will look at this. Thank you, Peter