Re: [PATCH 2/2] qemu-iotests: Test convert to qcow2 compressed to NBD

[Nir Soffer]

On Mon, Jul 27, 2020 at 1:05 PM Max Reitz  wrote:
>
> On 26.07.20 17:25, Nir Soffer wrote:
> > Add test for "qemu-img convert -O qcow2 -c" to NBD target. The use case
> > is writing compressed disk content to OVA archive.
> >
> > Signed-off-by: Nir Soffer 
> > ---
> >  tests/qemu-iotests/302 | 83 ++
> >  tests/qemu-iotests/302.out | 27 +
> >  tests/qemu-iotests/group   |  1 +
> >  3 files changed, 111 insertions(+)
> >  create mode 100755 tests/qemu-iotests/302
> >  create mode 100644 tests/qemu-iotests/302.out
> >
> > diff --git a/tests/qemu-iotests/302 b/tests/qemu-iotests/302
> > new file mode 100755
> > index 00..cefde1f7cf
> > --- /dev/null
> > +++ b/tests/qemu-iotests/302
> > @@ -0,0 +1,83 @@
> > +#!/usr/bin/env python3
> > +#
> > +# Tests conveting qcow2 compressed to NBD
>
> *converting
>
> > +#
> > +# Copyright (c) 2020 Nir Soffer 
> > +#
> > +# 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 .
> > +#
> > +# owner=nir...@gmail.com
> > +
> > +import json
> > +import iotests
> > +
> > +from iotests import (
> > +file_path,
> > +qemu_img,
> > +qemu_img_create,
> > +qemu_img_log,
> > +qemu_img_pipe,
> > +qemu_io,
> > +qemu_nbd,
> > +)
> > +
> > +iotests.script_initialize(supported_fmts=["qcow2"])
> > +
> > +# Create source disk, format does not matter.
> > +src_disk = file_path("disk.img")
> > +qemu_img_create("-f", "raw", src_disk, "10m")
>
> If the format doesn’t matter, why not just use qcow2 and so put
> iotests.imgfmt here?  (And everywhere else where you now have -f raw.)

I tried to use the simplest setup that is less likely to break, but
thinking about
CI environments with strange storage, maybe using qcow2 source disk will be
more robust.

> > +qemu_io("-f", "raw", "-c", "write 1m 64K", src_disk)
>
> (Except I think qemu_io already has -f qcow2 in its arguments by
> default, so specifying the format wouldn’t even be necessary here.)
>
> > +# The use case is writing qcow2 image directly into a tar file. Code to 
> > create
> > +# real tar file not included.
> > +#
> > +# offsetcontent
> > +# ---
> > +#  0first memebr header
>
> *member
>
> > +#512first member data
> > +#   1024second memeber header
>
> *member
>
> > +#   1536second member data
> > +
> > +tar_file = file_path("test.tar")
> > +out = qemu_img_pipe("measure", "-O", "qcow2", "--output", "json", src_disk)
> > +measure = json.loads(out)
> > +qemu_img_create("-f", "raw", tar_file, str(measure["required"]))
>
> Should this be measure["required"] + 1536?
>
> > +
> > +nbd_sock = file_path("nbd-sock", base_dir=iotests.sock_dir)
> > +nbd_uri = "nbd+unix:///exp?socket=" + nbd_sock
> > +
> > +# Use raw format to allow creating qcow2 directy into tar file.
> > +qemu_nbd(
> > +"--socket", nbd_sock,
> > +"--persistent",
> > +"--export-name", "exp",
> > +"--format", "raw",
> > +"--offset", "1536",
> > +tar_file)
> > +
> > +iotests.log("=== Target image info ===")
> > +qemu_img_log("info", nbd_uri)
> > +
> > +# Write image into the tar file. In a real applicatio we would write a tar
>
> *application
>
> > +# entry after writing the image.
> > +qemu_img("convert", "-f", "raw", "-O", "qcow2", "-c", src_disk, nbd_uri)
> > +
> > +iotests.log("=== Converted image info ===")
> > +qemu_img_log("info", nbd_uri)
> > +
> > +iotests.log("=== Converted image check ===")
> > +qemu_img_log("check", nbd_uri)
> > +
> > +iotests.log("=== Comparing to source disk ===")
> > +qemu_img_log("compare", src_disk, nbd_uri)
> > diff --git a/tests/qemu-iotests/302.out b/tests/qemu-iotests/302.out
> > new file mode 100644
> > index 00..babef3d574
> > --- /dev/null
> > +++ b/tests/qemu-iotests/302.out
> > @@ -0,0 +1,27 @@
> > +=== Target image info ===
> > +image: nbd+unix:///exp?socket=SOCK_DIR/PID-nbd-sock
> > +file format: raw
> > +virtual size: 446 KiB (457216 bytes)
> > +disk size: unavailable
> > +
> > +=== Converted image info ===
> > +image: nbd+unix:///exp?socket=SOCK_DIR/PID-nbd-sock
> > +file format: qcow2
> > +virtual size: 10 MiB (10485760 bytes)
> > +disk size: unavailable
> > +cluster_size: 65536
> > +Format specific information:
> > +compat: 1.1
> > +compression type: zlib
> > +lazy refcounts: false
> > +refcount bits: 16
> > +corrupt: false

Re: [PATCH 2/2] qemu-iotests: Test convert to qcow2 compressed to NBD

[Nir Soffer]

On Mon, Jul 27, 2020 at 5:41 PM Eric Blake  wrote:
>
> On 7/27/20 9:35 AM, Nir Soffer wrote:
>
> >> I guess it's okay that you don't create a real tar file here, but
> >> listing the commands to create it (even as a comment) is better than
> >> just saying "trust me".  And it doesn't seem like that much more work -
> >> it looks like the key to your test is that you created a tar file
> >> containing two files, where the first file was less than 512 bytes and
> >> the second file is your target destination that you will be rewriting.
> >
> > The real code is more complicated, something like:
> >
> >  offset = tar.fileobj.tell() + BLOCK_SIZE
> >
> >  with open(tar.name, "r+") as f:
> >  f.truncate(offset + measure["required"])
> >
> >  convert_image(image, tar.name, offset)
> >
> >  check = check_image(tar.name, offset)
> >  size = check["image-end-offset"]
> >
> >  member = tarfile.TarInfo(name)
> >  member.size = size
> >  tar.addfile(member)
> >
> >  tar_size = offset + round_up(size)
> >
> >  tar.fileobj.seek(tar_size)
> >  with open(tar.name, "r+") as f:
> >  f.truncate(tar_size)
> >
> > I'm not sure it helps qemu developers working on these tests.
>
> The closer the iotest is to reality, the more likely it will serve as a
> good regression test.  Cutting corners risks a test that passes in
> isolation even when we've done something that breaks the overall process
> in one of the corners you cut.

I'll add this code then.

> >>
> >> At any rate, given the urgency of getting pull requests for -rc2 in
> >> before slamming Peter tomorrow, I'll probably try to touch up the issues
> >> Max pointed out and queue it today.
> >
> > Thanks Max and Eric.
> >
> > Should I post a fixed version later today?
>
> A v2 would be helpful.

Will post later today.

Re: [PATCH 2/2] qemu-iotests: Test convert to qcow2 compressed to NBD

[Eric Blake]

On 7/27/20 9:35 AM, Nir Soffer wrote:


I guess it's okay that you don't create a real tar file here, but
listing the commands to create it (even as a comment) is better than
just saying "trust me".  And it doesn't seem like that much more work -
it looks like the key to your test is that you created a tar file
containing two files, where the first file was less than 512 bytes and
the second file is your target destination that you will be rewriting.


The real code is more complicated, something like:

 offset = tar.fileobj.tell() + BLOCK_SIZE

 with open(tar.name, "r+") as f:
 f.truncate(offset + measure["required"])

 convert_image(image, tar.name, offset)

 check = check_image(tar.name, offset)
 size = check["image-end-offset"]

 member = tarfile.TarInfo(name)
 member.size = size
 tar.addfile(member)

 tar_size = offset + round_up(size)

 tar.fileobj.seek(tar_size)
 with open(tar.name, "r+") as f:
 f.truncate(tar_size)

I'm not sure it helps qemu developers working on these tests.


The closer the iotest is to reality, the more likely it will serve as a 
good regression test.  Cutting corners risks a test that passes in 
isolation even when we've done something that breaks the overall process 
in one of the corners you cut.





At any rate, given the urgency of getting pull requests for -rc2 in
before slamming Peter tomorrow, I'll probably try to touch up the issues
Max pointed out and queue it today.


Thanks Max and Eric.

Should I post a fixed version later today?


A v2 would be helpful.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 2/2] qemu-iotests: Test convert to qcow2 compressed to NBD

[Nir Soffer]

On Mon, Jul 27, 2020 at 5:14 PM Eric Blake  wrote:
>
> On 7/27/20 5:04 AM, Max Reitz wrote:
> > On 26.07.20 17:25, Nir Soffer wrote:
> >> Add test for "qemu-img convert -O qcow2 -c" to NBD target. The use case
> >> is writing compressed disk content to OVA archive.
> >>
> >> Signed-off-by: Nir Soffer 
> >> ---
>
> >
> >> +# The use case is writing qcow2 image directly into a tar file. Code to 
> >> create
> >> +# real tar file not included.
> >> +#
> >> +# offsetcontent
> >> +# ---
> >> +#  0first memebr header
> >
> > *member

Sorry for the typos, I need to setup automated spelling check :-)

> >
> >> +#512first member data
> >> +#   1024second memeber header
> >
> > *member
> >
> >> +#   1536second member data
> >> +
> >> +tar_file = file_path("test.tar")
>
> I guess it's okay that you don't create a real tar file here, but
> listing the commands to create it (even as a comment) is better than
> just saying "trust me".  And it doesn't seem like that much more work -
> it looks like the key to your test is that you created a tar file
> containing two files, where the first file was less than 512 bytes and
> the second file is your target destination that you will be rewriting.

The real code is more complicated, something like:

offset = tar.fileobj.tell() + BLOCK_SIZE

with open(tar.name, "r+") as f:
f.truncate(offset + measure["required"])

convert_image(image, tar.name, offset)

check = check_image(tar.name, offset)
size = check["image-end-offset"]

member = tarfile.TarInfo(name)
member.size = size
tar.addfile(member)

tar_size = offset + round_up(size)

tar.fileobj.seek(tar_size)
with open(tar.name, "r+") as f:
f.truncate(tar_size)

I'm not sure it helps qemu developers working on these tests.

> >> +out = qemu_img_pipe("measure", "-O", "qcow2", "--output", "json", 
> >> src_disk)
> >> +measure = json.loads(out)
> >> +qemu_img_create("-f", "raw", tar_file, str(measure["required"]))
> >
> > Should this be measure["required"] + 1536?
>
> The test works without it (because of compression), but yes, if you are
> going to test writing into an offset, you should oversize your file by
> that same offset.

Right, in the real code using this I indeed use offset + required.

> >> +
> >> +nbd_sock = file_path("nbd-sock", base_dir=iotests.sock_dir)
> >> +nbd_uri = "nbd+unix:///exp?socket=" + nbd_sock
> >> +
> >> +# Use raw format to allow creating qcow2 directy into tar file.
> >> +qemu_nbd(
> >> +"--socket", nbd_sock,
> >> +"--persistent",
> >> +"--export-name", "exp",
> >> +"--format", "raw",
> >> +"--offset", "1536",
> >> +tar_file)
> >> +
> >> +iotests.log("=== Target image info ===")
> >> +qemu_img_log("info", nbd_uri)
> >> +
> >> +# Write image into the tar file. In a real applicatio we would write a tar
> >
> > *application
> >
>
> >> +=== Converted image check ===
> >> +No errors were found on the image.
> >> +1/160 = 0.62% allocated, 100.00% fragmented, 100.00% compressed clusters
> >> +Image end offset: 393216
> >
> > I hope none of this is fs-dependant.  (I don’t think it is, but who
> > knows.  I suppose we’ll find out.)
>
> Indeed - time to see what CI thinks of this.
>
> At any rate, given the urgency of getting pull requests for -rc2 in
> before slamming Peter tomorrow, I'll probably try to touch up the issues
> Max pointed out and queue it today.

Thanks Max and Eric.

Should I post a fixed version later today?

Re: [PATCH 2/2] qemu-iotests: Test convert to qcow2 compressed to NBD

[Eric Blake]

On 7/27/20 5:04 AM, Max Reitz wrote:

On 26.07.20 17:25, Nir Soffer wrote:

Add test for "qemu-img convert -O qcow2 -c" to NBD target. The use case
is writing compressed disk content to OVA archive.

Signed-off-by: Nir Soffer 
---





+# The use case is writing qcow2 image directly into a tar file. Code to create
+# real tar file not included.
+#
+# offsetcontent
+# ---
+#  0first memebr header


*member


+#512first member data
+#   1024second memeber header


*member


+#   1536second member data
+
+tar_file = file_path("test.tar")


I guess it's okay that you don't create a real tar file here, but 
listing the commands to create it (even as a comment) is better than 
just saying "trust me".  And it doesn't seem like that much more work - 
it looks like the key to your test is that you created a tar file 
containing two files, where the first file was less than 512 bytes and 
the second file is your target destination that you will be rewriting.



+out = qemu_img_pipe("measure", "-O", "qcow2", "--output", "json", src_disk)
+measure = json.loads(out)
+qemu_img_create("-f", "raw", tar_file, str(measure["required"]))


Should this be measure["required"] + 1536?


The test works without it (because of compression), but yes, if you are 
going to test writing into an offset, you should oversize your file by 
that same offset.





+
+nbd_sock = file_path("nbd-sock", base_dir=iotests.sock_dir)
+nbd_uri = "nbd+unix:///exp?socket=" + nbd_sock
+
+# Use raw format to allow creating qcow2 directy into tar file.
+qemu_nbd(
+"--socket", nbd_sock,
+"--persistent",
+"--export-name", "exp",
+"--format", "raw",
+"--offset", "1536",
+tar_file)
+
+iotests.log("=== Target image info ===")
+qemu_img_log("info", nbd_uri)
+
+# Write image into the tar file. In a real applicatio we would write a tar


*application




+=== Converted image check ===
+No errors were found on the image.
+1/160 = 0.62% allocated, 100.00% fragmented, 100.00% compressed clusters
+Image end offset: 393216


I hope none of this is fs-dependant.  (I don’t think it is, but who
knows.  I suppose we’ll find out.)


Indeed - time to see what CI thinks of this.

At any rate, given the urgency of getting pull requests for -rc2 in 
before slamming Peter tomorrow, I'll probably try to touch up the issues 
Max pointed out and queue it today.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 2/2] qemu-iotests: Test convert to qcow2 compressed to NBD

[Max Reitz]

On 26.07.20 17:25, Nir Soffer wrote:
> Add test for "qemu-img convert -O qcow2 -c" to NBD target. The use case
> is writing compressed disk content to OVA archive.
> 
> Signed-off-by: Nir Soffer 
> ---
>  tests/qemu-iotests/302 | 83 ++
>  tests/qemu-iotests/302.out | 27 +
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 111 insertions(+)
>  create mode 100755 tests/qemu-iotests/302
>  create mode 100644 tests/qemu-iotests/302.out
> 
> diff --git a/tests/qemu-iotests/302 b/tests/qemu-iotests/302
> new file mode 100755
> index 00..cefde1f7cf
> --- /dev/null
> +++ b/tests/qemu-iotests/302
> @@ -0,0 +1,83 @@
> +#!/usr/bin/env python3
> +#
> +# Tests conveting qcow2 compressed to NBD

*converting

> +#
> +# Copyright (c) 2020 Nir Soffer 
> +#
> +# 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 .
> +#
> +# owner=nir...@gmail.com
> +
> +import json
> +import iotests
> +
> +from iotests import (
> +file_path,
> +qemu_img,
> +qemu_img_create,
> +qemu_img_log,
> +qemu_img_pipe,
> +qemu_io,
> +qemu_nbd,
> +)
> +
> +iotests.script_initialize(supported_fmts=["qcow2"])
> +
> +# Create source disk, format does not matter.
> +src_disk = file_path("disk.img")
> +qemu_img_create("-f", "raw", src_disk, "10m")

If the format doesn’t matter, why not just use qcow2 and so put
iotests.imgfmt here?  (And everywhere else where you now have -f raw.)

> +qemu_io("-f", "raw", "-c", "write 1m 64K", src_disk)

(Except I think qemu_io already has -f qcow2 in its arguments by
default, so specifying the format wouldn’t even be necessary here.)

> +# The use case is writing qcow2 image directly into a tar file. Code to 
> create
> +# real tar file not included.
> +#
> +# offsetcontent
> +# ---
> +#  0first memebr header

*member

> +#512first member data
> +#   1024second memeber header

*member

> +#   1536second member data
> +
> +tar_file = file_path("test.tar")
> +out = qemu_img_pipe("measure", "-O", "qcow2", "--output", "json", src_disk)
> +measure = json.loads(out)
> +qemu_img_create("-f", "raw", tar_file, str(measure["required"]))

Should this be measure["required"] + 1536?

> +
> +nbd_sock = file_path("nbd-sock", base_dir=iotests.sock_dir)
> +nbd_uri = "nbd+unix:///exp?socket=" + nbd_sock
> +
> +# Use raw format to allow creating qcow2 directy into tar file.
> +qemu_nbd(
> +"--socket", nbd_sock,
> +"--persistent",
> +"--export-name", "exp",
> +"--format", "raw",
> +"--offset", "1536",
> +tar_file)
> +
> +iotests.log("=== Target image info ===")
> +qemu_img_log("info", nbd_uri)
> +
> +# Write image into the tar file. In a real applicatio we would write a tar

*application

> +# entry after writing the image.
> +qemu_img("convert", "-f", "raw", "-O", "qcow2", "-c", src_disk, nbd_uri)
> +
> +iotests.log("=== Converted image info ===")
> +qemu_img_log("info", nbd_uri)
> +
> +iotests.log("=== Converted image check ===")
> +qemu_img_log("check", nbd_uri)
> +
> +iotests.log("=== Comparing to source disk ===")
> +qemu_img_log("compare", src_disk, nbd_uri)
> diff --git a/tests/qemu-iotests/302.out b/tests/qemu-iotests/302.out
> new file mode 100644
> index 00..babef3d574
> --- /dev/null
> +++ b/tests/qemu-iotests/302.out
> @@ -0,0 +1,27 @@
> +=== Target image info ===
> +image: nbd+unix:///exp?socket=SOCK_DIR/PID-nbd-sock
> +file format: raw
> +virtual size: 446 KiB (457216 bytes)
> +disk size: unavailable
> +
> +=== Converted image info ===
> +image: nbd+unix:///exp?socket=SOCK_DIR/PID-nbd-sock
> +file format: qcow2
> +virtual size: 10 MiB (10485760 bytes)
> +disk size: unavailable
> +cluster_size: 65536
> +Format specific information:
> +compat: 1.1
> +compression type: zlib
> +lazy refcounts: false
> +refcount bits: 16
> +corrupt: false
> +
> +=== Converted image check ===
> +No errors were found on the image.
> +1/160 = 0.62% allocated, 100.00% fragmented, 100.00% compressed clusters
> +Image end offset: 393216

I hope none of this is fs-dependant.  (I don’t think it is, but who
knows.  I suppose we’ll find out.)

Max

> +=== Comparing to source disk ===
> +Images are identical.
> +
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 1d0252e1f0..1e1cb27bc8 100644
> --- a/tests/qemu-iotests/group
> +++