Re: [Qemu-block] [PATCH 3/3] iotests: Test format probes

[Colin Lord]

On 07/12/2016 05:17 PM, John Snow wrote:
> 
> 
> On 07/11/2016 03:50 PM, Colin Lord wrote:
>> Adds a new iotest for testing that the format probing functions work as
>> expected. This is done by booting up a vm with a disk image without
>> specifying the image format. Then the format is checked using a call to
>> query-block.
>>
>> For any format specified, at least one check is done with an image of
>> the given format, and one check is done against a raw image. This is to
>> ensure that a probe correctly returns a high score when the format is
>> matched, and does not return a high score when the format should not be
>> matched, as would be the case with raw images.
>>
>> Signed-off-by: Colin Lord 
>> ---
>>  tests/qemu-iotests/158| 53 
>> +++
>>  tests/qemu-iotests/158.out|  5 
>>  tests/qemu-iotests/group  |  1 +
>>  tests/qemu-iotests/iotests.py |  5 
>>  4 files changed, 64 insertions(+)
>>  create mode 100644 tests/qemu-iotests/158
>>  create mode 100644 tests/qemu-iotests/158.out
>>
>> diff --git a/tests/qemu-iotests/158 b/tests/qemu-iotests/158
>> new file mode 100644
>> index 000..70ad298
>> --- /dev/null
>> +++ b/tests/qemu-iotests/158
>> @@ -0,0 +1,53 @@
>> +#!/usr/bin/env python
>> +
> 
>  No license? It will default to GPL2, but it may be better to
> explicitly state the license for a new file. 
> 
>> +import iotests
>> +
>> +imgs = {'raw': ['grub_mbr.raw'], 'bochs': ['empty.bochs'],
>> +'cloop': ['simple-pattern.cloop'],
>> +'parallels': ['parallels-v1', 'parallels-v2'],
>> +'qcow': ['empty.qcow'], 'qcow2': ['empty.qcow2'],
>> +'qed': ['empty.qed'], 'vdi': ['empty.vdi'],
>> +'vpc': ['virtualpc-dynamic.vhd'], 'vhdx': 
>> ['iotest-dynamic-1G.vhdx'],
>> +'vmdk': ['iotest-version3.vmdk'], 'luks': ['empty.luks'],
>> +'dmg': ['empty.dmg']}
>> +
> 
> Might be nicer to do one format->file mapping per line.
> 
>> +class TestProbingFormats(iotests.QMPTestCase):
>> +def setUp(self):
>> +self.vm = iotests.VM()
>> +self.img_paths = []
>> +luks_opts = ''
>> +if iotests.imgfmt == 'luks':
>> +luks_opts = (',key-secret=sec0')
>> +for img_name in imgs[iotests.imgfmt]:
>> +self.img_paths.append(iotests.use_sample_image(img_name))
>> +self.vm.add_drive_raw('file=%s,id=drive%s,media=cdrom%s' %
>> +  (self.img_paths[-1], len(self.img_paths) 
>> - 1,
>> +  luks_opts))
> 
> Perhaps id=drive%s could be id=drive%d, but I'm no pythonista.
> 
>> +if iotests.imgfmt == 'luks':
>> +self.vm.use_luks()
>> +if iotests.imgfmt != 'raw':
>> +self.img_paths.append(iotests.use_sample_image(imgs['raw'][0]))
>> +self.vm.add_drive_raw('file=%s,id=drive%s,media=cdrom' %
>> +  (self.img_paths[-1], len(self.img_paths) 
>> - 1))
>> +self.vm.launch()
>> +
> 
> Oh, everything as a CDROM? I guess there's no reason that doesn't work,
> but it strikes me as a bit unconventional. I guess you'll be testing
> some quite esoteric configurations this way.
> 
Yeah it looks a little odd. I actually did it this way because some of
the image formats are read-only and so they throw errors if you try to
mount them as writable. I wasn't finding an option anywhere to mount
drives as read only so I just took the quick way out and went with
cdrom. Is there a better way to mount drives as read only?

>> +def tearDown(self):
>> +self.vm.shutdown()
>> +for img in self.img_paths:
>> +iotests.rm_test_image(img)
>> +
>> +def test_probe_detects_format(self):
>> +result = self.vm.qmp('query-block')
>> +for i in range(len(self.img_paths) - 1):
>> +self.assert_qmp(result, 'return[%s]/inserted/image/format' %
>> +i, iotests.imgfmt)
>> +
> 
> Seems sane.
> 
>> +def test_probe_detects_raw(self):
>> +result = self.vm.qmp('query-block')
>> +self.assert_qmp(result, 'return[%s]/inserted/image/format' %
>> +(len(self.img_paths) - 1), 'raw')
>> +
> 
> Why do we need to test raw against every format, exactly? The absence or
> presence of other drives shouldn't affect the probing, so allowing us to
> test the raw probe with
> 
> ./check -v -raw 158
> 
> should be enough.
> 
> Unless I'm misunderstanding the point?
> 
I guess it might be unnecessary. I mostly had it in there to prevent a
hypothetical probe which always returns 100 from passing the tests. For
example, if I'm testing the bochs probe on a bochs image, if it returned
100 it would pass (which is correct). But it should also be tested
against another format to make sure the other format doesn't incorrectly
get detected as bochs. I guess this situation gets fixed naturally if
you test more than one format, it's 

Re: [Qemu-block] [PATCH 3/3] iotests: Test format probes

[John Snow]

On 07/11/2016 03:50 PM, Colin Lord wrote:
> Adds a new iotest for testing that the format probing functions work as
> expected. This is done by booting up a vm with a disk image without
> specifying the image format. Then the format is checked using a call to
> query-block.
> 
> For any format specified, at least one check is done with an image of
> the given format, and one check is done against a raw image. This is to
> ensure that a probe correctly returns a high score when the format is
> matched, and does not return a high score when the format should not be
> matched, as would be the case with raw images.
> 
> Signed-off-by: Colin Lord 
> ---
>  tests/qemu-iotests/158| 53 
> +++
>  tests/qemu-iotests/158.out|  5 
>  tests/qemu-iotests/group  |  1 +
>  tests/qemu-iotests/iotests.py |  5 
>  4 files changed, 64 insertions(+)
>  create mode 100644 tests/qemu-iotests/158
>  create mode 100644 tests/qemu-iotests/158.out
> 
> diff --git a/tests/qemu-iotests/158 b/tests/qemu-iotests/158
> new file mode 100644
> index 000..70ad298
> --- /dev/null
> +++ b/tests/qemu-iotests/158
> @@ -0,0 +1,53 @@
> +#!/usr/bin/env python
> +

 No license? It will default to GPL2, but it may be better to
explicitly state the license for a new file. 

> +import iotests
> +
> +imgs = {'raw': ['grub_mbr.raw'], 'bochs': ['empty.bochs'],
> +'cloop': ['simple-pattern.cloop'],
> +'parallels': ['parallels-v1', 'parallels-v2'],
> +'qcow': ['empty.qcow'], 'qcow2': ['empty.qcow2'],
> +'qed': ['empty.qed'], 'vdi': ['empty.vdi'],
> +'vpc': ['virtualpc-dynamic.vhd'], 'vhdx': ['iotest-dynamic-1G.vhdx'],
> +'vmdk': ['iotest-version3.vmdk'], 'luks': ['empty.luks'],
> +'dmg': ['empty.dmg']}
> +

Might be nicer to do one format->file mapping per line.

> +class TestProbingFormats(iotests.QMPTestCase):
> +def setUp(self):
> +self.vm = iotests.VM()
> +self.img_paths = []
> +luks_opts = ''
> +if iotests.imgfmt == 'luks':
> +luks_opts = (',key-secret=sec0')
> +for img_name in imgs[iotests.imgfmt]:
> +self.img_paths.append(iotests.use_sample_image(img_name))
> +self.vm.add_drive_raw('file=%s,id=drive%s,media=cdrom%s' %
> +  (self.img_paths[-1], len(self.img_paths) - 
> 1,
> +  luks_opts))

Perhaps id=drive%s could be id=drive%d, but I'm no pythonista.

> +if iotests.imgfmt == 'luks':
> +self.vm.use_luks()
> +if iotests.imgfmt != 'raw':
> +self.img_paths.append(iotests.use_sample_image(imgs['raw'][0]))
> +self.vm.add_drive_raw('file=%s,id=drive%s,media=cdrom' %
> +  (self.img_paths[-1], len(self.img_paths) - 
> 1))
> +self.vm.launch()
> +

Oh, everything as a CDROM? I guess there's no reason that doesn't work,
but it strikes me as a bit unconventional. I guess you'll be testing
some quite esoteric configurations this way.

> +def tearDown(self):
> +self.vm.shutdown()
> +for img in self.img_paths:
> +iotests.rm_test_image(img)
> +
> +def test_probe_detects_format(self):
> +result = self.vm.qmp('query-block')
> +for i in range(len(self.img_paths) - 1):
> +self.assert_qmp(result, 'return[%s]/inserted/image/format' %
> +i, iotests.imgfmt)
> +

Seems sane.

> +def test_probe_detects_raw(self):
> +result = self.vm.qmp('query-block')
> +self.assert_qmp(result, 'return[%s]/inserted/image/format' %
> +(len(self.img_paths) - 1), 'raw')
> +

Why do we need to test raw against every format, exactly? The absence or
presence of other drives shouldn't affect the probing, so allowing us to
test the raw probe with

./check -v -raw 158

should be enough.

Unless I'm misunderstanding the point?

> +if __name__ == '__main__':
> +iotests.main(supported_fmts=['raw', 'bochs', 'cloop', 'parallels', 
> 'qcow',
> + 'qcow2', 'qed', 'vdi', 'vpc', 'vhdx', 
> 'vmdk',
> + 'luks', 'dmg'])

I suppose this is explicitly to overcome the "any image" declaration
actually excluding a lot of images.

hey, you know what would be slick? populating this based off of the keys
present in the imgs dict.

> diff --git a/tests/qemu-iotests/158.out b/tests/qemu-iotests/158.out
> new file mode 100644
> index 000..fbc63e6
> --- /dev/null
> +++ b/tests/qemu-iotests/158.out
> @@ -0,0 +1,5 @@
> +..
> +--
> +Ran 2 tests
> +
> +OK
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 1c6fcb6..4563e81 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -156,3 +156,4 @@
>  154 rw auto backing quick
>  155 rw auto
>  

[Qemu-block] [PATCH 3/3] iotests: Test format probes

[Colin Lord]

Adds a new iotest for testing that the format probing functions work as
expected. This is done by booting up a vm with a disk image without
specifying the image format. Then the format is checked using a call to
query-block.

For any format specified, at least one check is done with an image of
the given format, and one check is done against a raw image. This is to
ensure that a probe correctly returns a high score when the format is
matched, and does not return a high score when the format should not be
matched, as would be the case with raw images.

Signed-off-by: Colin Lord 
---
 tests/qemu-iotests/158| 53 +++
 tests/qemu-iotests/158.out|  5 
 tests/qemu-iotests/group  |  1 +
 tests/qemu-iotests/iotests.py |  5 
 4 files changed, 64 insertions(+)
 create mode 100644 tests/qemu-iotests/158
 create mode 100644 tests/qemu-iotests/158.out

diff --git a/tests/qemu-iotests/158 b/tests/qemu-iotests/158
new file mode 100644
index 000..70ad298
--- /dev/null
+++ b/tests/qemu-iotests/158
@@ -0,0 +1,53 @@
+#!/usr/bin/env python
+
+import iotests
+
+imgs = {'raw': ['grub_mbr.raw'], 'bochs': ['empty.bochs'],
+'cloop': ['simple-pattern.cloop'],
+'parallels': ['parallels-v1', 'parallels-v2'],
+'qcow': ['empty.qcow'], 'qcow2': ['empty.qcow2'],
+'qed': ['empty.qed'], 'vdi': ['empty.vdi'],
+'vpc': ['virtualpc-dynamic.vhd'], 'vhdx': ['iotest-dynamic-1G.vhdx'],
+'vmdk': ['iotest-version3.vmdk'], 'luks': ['empty.luks'],
+'dmg': ['empty.dmg']}
+
+class TestProbingFormats(iotests.QMPTestCase):
+def setUp(self):
+self.vm = iotests.VM()
+self.img_paths = []
+luks_opts = ''
+if iotests.imgfmt == 'luks':
+luks_opts = (',key-secret=sec0')
+for img_name in imgs[iotests.imgfmt]:
+self.img_paths.append(iotests.use_sample_image(img_name))
+self.vm.add_drive_raw('file=%s,id=drive%s,media=cdrom%s' %
+  (self.img_paths[-1], len(self.img_paths) - 1,
+  luks_opts))
+if iotests.imgfmt == 'luks':
+self.vm.use_luks()
+if iotests.imgfmt != 'raw':
+self.img_paths.append(iotests.use_sample_image(imgs['raw'][0]))
+self.vm.add_drive_raw('file=%s,id=drive%s,media=cdrom' %
+  (self.img_paths[-1], len(self.img_paths) - 
1))
+self.vm.launch()
+
+def tearDown(self):
+self.vm.shutdown()
+for img in self.img_paths:
+iotests.rm_test_image(img)
+
+def test_probe_detects_format(self):
+result = self.vm.qmp('query-block')
+for i in range(len(self.img_paths) - 1):
+self.assert_qmp(result, 'return[%s]/inserted/image/format' %
+i, iotests.imgfmt)
+
+def test_probe_detects_raw(self):
+result = self.vm.qmp('query-block')
+self.assert_qmp(result, 'return[%s]/inserted/image/format' %
+(len(self.img_paths) - 1), 'raw')
+
+if __name__ == '__main__':
+iotests.main(supported_fmts=['raw', 'bochs', 'cloop', 'parallels', 'qcow',
+ 'qcow2', 'qed', 'vdi', 'vpc', 'vhdx', 'vmdk',
+ 'luks', 'dmg'])
diff --git a/tests/qemu-iotests/158.out b/tests/qemu-iotests/158.out
new file mode 100644
index 000..fbc63e6
--- /dev/null
+++ b/tests/qemu-iotests/158.out
@@ -0,0 +1,5 @@
+..
+--
+Ran 2 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 1c6fcb6..4563e81 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -156,3 +156,4 @@
 154 rw auto backing quick
 155 rw auto
 156 rw auto quick
+158 quick
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 3ba0de2..101ae91 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -49,6 +49,7 @@ if os.environ.get('QEMU_OPTIONS'):
 imgfmt = os.environ.get('IMGFMT', 'raw')
 imgproto = os.environ.get('IMGPROTO', 'file')
 test_dir = os.environ.get('TEST_DIR')
+imgkeysecret = os.environ.get('IMGKEYSECRET')
 output_dir = os.environ.get('OUTPUT_DIR', '.')
 cachemode = os.environ.get('CACHEMODE')
 qemu_default_machine = os.environ.get('QEMU_DEFAULT_MACHINE')
@@ -208,6 +209,10 @@ class VM(object):
 self._num_drives += 1
 return self
 
+def use_luks(self):
+self._args.append('-object')
+self._args.append('secret,id=sec0,data=%s' % imgkeysecret)
+
 def pause_drive(self, drive, event=None):
 '''Pause drive r/w operations'''
 if not event:
-- 
2.5.5