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

[Max Reitz <mreitz@redhat.com>]

On 2015-02-24 at 01:49, Peter Lieven wrote:
> 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 <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).
>
> 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.

Right. Very well then.

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

Well, it won't work so well with the MIN() being necessary...

Max