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

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

[Arthur Gautier]

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.

Reviewed-by: William Dauchy <william@gandi.net>
Signed-off-by: Arthur Gautier <baloo@gandi.net>
---
 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);
diff --git a/tests/qemu-iotests/106 b/tests/qemu-iotests/106
new file mode 100755
index 0000000..d17c588
--- /dev/null
+++ b/tests/qemu-iotests/106
@@ -0,0 +1,67 @@
+#!/bin/bash
+#
+# Create then convert raw to vmdk with a little image
+#
+# Copyright (C) 2014 Red Hat, Inc.
+#               2014 Gandi SAS
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=baloo@gandi.net
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+	_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt vmdk
+_supported_proto generic
+_supported_os Linux
+
+IMG_SIZE=1K
+
+TEST_IMG="$TEST_IMG.base" _make_test_img $IMG_SIZE | _filter_img_create
+
+#echo "=== Convert image to vmdk ==="
+#echo
+$QEMU_IMG convert -f raw -O vmdk "$TEST_IMG.base" "$TEST_IMG"
+
+echo "=== Check for correct number of cylinders ==="
+echo
+
+# Cylinders are in the top of the file
+CYLINDERS=`grep -a 'cylinders' $TEST_IMG | head -1 | sed -r 's/.*"([0-9]+)".*/\1/' | tr -d '\n'`
+
+
+# Check the cylinders number is plausible
+[ "x$CYLINDERS" = "x0" ] && (echo "Cylinders cannot be zero" >&2 && exit 1)
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/106.out b/tests/qemu-iotests/106.out
new file mode 100644
index 0000000..5d2f0a3
--- /dev/null
+++ b/tests/qemu-iotests/106.out
@@ -0,0 +1,5 @@
+QA output created by 106
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=1024 
+=== Check for correct number of cylinders ===
+
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index b230996..0c68320 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -106,3 +106,4 @@
 103 rw auto quick
 104 rw auto
 105 rw auto quick
+106 rw auto quick
-- 
2.1.1

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 <william@gandi.net>
> Signed-off-by: Arthur Gautier <baloo@gandi.net>
> ---
>  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);
[]

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

[Markus Armbruster]

Michael Tokarev <mjt@tls.msk.ru> writes:

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

Too many questions for -trivial, copying VMDK maintainers.

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1169 bytes --]

On Thu, Oct 23, 2014 at 10:03:25AM +0200, Markus Armbruster wrote:
> Michael Tokarev <mjt@tls.msk.ru> writes:
> 
> > 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?
> 
> Too many questions for -trivial, copying VMDK maintainers.

Fam: What do you think about this change?

Stefan


[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

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

[Fam Zheng]

On Tue, 10/28 16:00, Stefan Hajnoczi wrote:
> On Thu, Oct 23, 2014 at 10:03:25AM +0200, Markus Armbruster wrote:
> > Michael Tokarev <mjt@tls.msk.ru> writes:
> > 
> > > 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?

I'd like to hear an answer to this question too, so we know why it's right and
worth to have.

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

I don't remember a reason why truncating (I doubt there is any), OTOH I'm not
sure what is the right thing to do if the guest tries to write to the last
incomplete CHS either.

Fam

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

[Arthur Gautier]

On Wed, Oct 29, 2014 at 09:28:52AM +0800, Fam Zheng wrote:
> On Tue, 10/28 16:00, Stefan Hajnoczi wrote:
> > On Thu, Oct 23, 2014 at 10:03:25AM +0200, Markus Armbruster wrote:
> > > Michael Tokarev <mjt@tls.msk.ru> writes:
> > > 
> > > > 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?
> 
> I'd like to hear an answer to this question too, so we know why it's right and
> worth to have.
> 
Sincerely, I didn't tried myself.

> > > >
> > > > 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?
> 
> I don't remember a reason why truncating (I doubt there is any), OTOH I'm not
> sure what is the right thing to do if the guest tries to write to the last
> incomplete CHS either.
> 
> Fam

Maybe we can expand the disk to the size matching cylinders * sectors *
sector_size and issue a warning to the user? This way it will always be
safe.

WDYT?

-- 
\o/ Arthur
 G  Gandi.net

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

[Fam Zheng]

On Thu, 10/30 10:09, Arthur Gautier wrote:
> On Wed, Oct 29, 2014 at 09:28:52AM +0800, Fam Zheng wrote:
> > On Tue, 10/28 16:00, Stefan Hajnoczi wrote:
> > > On Thu, Oct 23, 2014 at 10:03:25AM +0200, Markus Armbruster wrote:
> > > > Michael Tokarev <mjt@tls.msk.ru> writes:
> > > > 
> > > > > 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?
> > 
> > I'd like to hear an answer to this question too, so we know why it's right and
> > worth to have.
> > 
> Sincerely, I didn't tried myself.
> 
> > > > >
> > > > > 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?
> > 
> > I don't remember a reason why truncating (I doubt there is any), OTOH I'm not
> > sure what is the right thing to do if the guest tries to write to the last
> > incomplete CHS either.
> > 
> > Fam
> 
> Maybe we can expand the disk to the size matching cylinders * sectors *
> sector_size and issue a warning to the user? This way it will always be
> safe.
> 

Possible, but I don't know if it's worth it. Could you explain what doesn't
work now, in order to show *why* you need this change?

Thanks,
Fam

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

[Arthur Gautier]

On Thu, Oct 30, 2014 at 06:27:20PM +0800, Fam Zheng wrote:
> On Thu, 10/30 10:09, Arthur Gautier wrote:
> > On Wed, Oct 29, 2014 at 09:28:52AM +0800, Fam Zheng wrote:
> > > On Tue, 10/28 16:00, Stefan Hajnoczi wrote:
> > > > On Thu, Oct 23, 2014 at 10:03:25AM +0200, Markus Armbruster wrote:
> > > > > Michael Tokarev <mjt@tls.msk.ru> writes:
> > > > > 
> > > > > > 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?
> > > 
> > > I'd like to hear an answer to this question too, so we know why it's right and
> > > worth to have.
> > > 
> > Sincerely, I didn't tried myself.
> > 
> > > > > >
> > > > > > 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?
> > > 
> > > I don't remember a reason why truncating (I doubt there is any), OTOH I'm not
> > > sure what is the right thing to do if the guest tries to write to the last
> > > incomplete CHS either.
> > > 
> > > Fam
> > 
> > Maybe we can expand the disk to the size matching cylinders * sectors *
> > sector_size and issue a warning to the user? This way it will always be
> > safe.
> > 
> 
> Possible, but I don't know if it's worth it. Could you explain what doesn't
> work now, in order to show *why* you need this change?
> 
> Thanks,
> Fam

Please take a look at the test in the patch. I have been able to
generate an image with a number of cylinders of zero which I believe is
wrong.



-- 
\o/ Arthur
 G  Gandi.net

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

[Fam Zheng]

On Thu, 10/30 15:28, Arthur Gautier wrote:
> On Thu, Oct 30, 2014 at 06:27:20PM +0800, Fam Zheng wrote:
> > On Thu, 10/30 10:09, Arthur Gautier wrote:
> > > On Wed, Oct 29, 2014 at 09:28:52AM +0800, Fam Zheng wrote:
> > > > On Tue, 10/28 16:00, Stefan Hajnoczi wrote:
> > > > > On Thu, Oct 23, 2014 at 10:03:25AM +0200, Markus Armbruster wrote:
> > > > > > Michael Tokarev <mjt@tls.msk.ru> writes:
> > > > > > 
> > > > > > > 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?
> > > > 
> > > > I'd like to hear an answer to this question too, so we know why it's right and
> > > > worth to have.
> > > > 
> > > Sincerely, I didn't tried myself.
> > > 
> > > > > > >
> > > > > > > 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?
> > > > 
> > > > I don't remember a reason why truncating (I doubt there is any), OTOH I'm not
> > > > sure what is the right thing to do if the guest tries to write to the last
> > > > incomplete CHS either.
> > > > 
> > > > Fam
> > > 
> > > Maybe we can expand the disk to the size matching cylinders * sectors *
> > > sector_size and issue a warning to the user? This way it will always be
> > > safe.
> > > 
> > 
> > Possible, but I don't know if it's worth it. Could you explain what doesn't
> > work now, in order to show *why* you need this change?
> > 
> > Thanks,
> > Fam
> 
> Please take a look at the test in the patch. I have been able to
> generate an image with a number of cylinders of zero which I believe is
> wrong.
> 

Why do you believe it is wrong? We are also able to create images with length
0.

Do you have a real use case for this? If we "fix" this to round up the
cylinders, there might be other issues for those users who actually use this
field.

Do yo know how VMware handle this?

Fam