Re: [Qemu-devel] [Qemu-trivial] [PATCH] vmdk: Fix cylinders number during convert

[Michael Tokarev]

On 10/22/2014 05:25 PM, Arthur Gautier wrote:
> We can not rely on int cast to get a correct number of cylinders. The
> cylinders information was wrong in 49.9999% of cases.
> 
> This ensures the cylinders always gets the ceiling value.

Good thing, especially the good probability :), and also a good patch
which comes with a test.

But I wonder if anything can break this way?  Migration, windows guest
being unable to find its partitions, something else?

And more.  What-if our drive size in cylinders will be larger than
the size in bytes?  The proposed div_round_up() will increase number
of cylinders, so size in CHS will be larger than size in bytes.  Maybe
there was a reason why originally the size in cylinders was calculated
by truncating extra fractional part?  What-if guest will try to access
the very last CHS which is incomplete?

Thanks,

/mjt

> Reviewed-by: William Dauchy <address@hidden>
> Signed-off-by: Arthur Gautier <address@hidden>
> ---
>  block/vmdk.c               |  4 +--
>  tests/qemu-iotests/106     | 67 
> ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/106.out |  5 ++++
>  tests/qemu-iotests/group   |  1 +
>  4 files changed, 75 insertions(+), 2 deletions(-)
>  create mode 100755 tests/qemu-iotests/106
>  create mode 100644 tests/qemu-iotests/106.out
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 4ae6c75..c22a6c6 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1928,8 +1928,8 @@ static int vmdk_create(const char *filename, QemuOpts 
> *opts, Error **errp)
>                             parent_desc_line,
>                             ext_desc_lines->str,
>                             (flags & BLOCK_FLAG_COMPAT6 ? 6 : 4),
> -                           total_size /
> -                               (int64_t)(63 * number_heads * 
> BDRV_SECTOR_SIZE),
> +                           DIV_ROUND_UP(total_size,
> +                               (int64_t)(63 * number_heads * 
> BDRV_SECTOR_SIZE)),
>                             number_heads,
>                             adapter_type);
>      desc_len = strlen(desc);
[]