Re: [Qemu-devel] [PATCH 4/5] block/vpc: make calculate_geometry spec conform

[Max Reitz <mreitz@redhat.com>]

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 <pl@kamp.de>
> ---
>   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).

>           *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.

I find the MIN() unnecessary, too, because calculate_geometry() will 
take care of that itself.

>       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;?

> +        }
> +    } 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.

Maybe you like that better. I do, but this is not my patch, so it's up 
to you.

All in all, apart from the "return -EFBIG" instead of "goto out", the 
patch is correct (if you ignore the minor difference for total_sectors 
== 65535 * 16 * 63).

Max

>       /* Prepare the Hard Disk Footer */
>       memset(buf, 0, 1024);