
From:  Peter Lieven 
Subject:  Re: [Qemudevel] [PATCH 4/5] block/vpc: make calculate_geometry spec conform 
Date:  Tue, 24 Feb 2015 07:49:30 +0100 
Useragent:  Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 
Am 23.02.2015 um 19:59 schrieb Max Reitz:
On 20150223 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, HyperV, 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/ffef50a507dd4cf8aaa3442c0673a029/Virtual%20Hard%20Disk%20Format%20Spec_10_18_06.doc Signedoffby: Peter Lieven <address@hidden>  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 bigendian 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 specconform or at least functionally specconform (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 * qemuimg 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 forloop 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
[Prev in Thread]  Current Thread  [Next in Thread] 