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 <address@hidden>
>> ---
>>  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 0000000..70ad298
>> --- /dev/null
>> +++ b/tests/qemu-iotests/158
>> @@ -0,0 +1,53 @@
>> +#!/usr/bin/env python
>> +
> 
> <eblake> No license? It will default to GPL2, but it may be better to
> explicitly state the license for a new file. </eblake>
> 
>> +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 just a little weird to think that
you could test a probe for one format but then not be sure that it was
functioning correctly until you also tested it against another format.
It seems if you ran iotests on qcow2, you'd want to be able to assume
that the qcow2 format probe was correct without also having to run
another test using another format.

However, since testing should generally be done against multiple formats
anyway, maybe it doesn't matter too much to take it out. It would also
simplify the script somewhat.

>> +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.
> 
I actually didn't know that there was an "any image" declaration. Would
it be good to use that here? If not, I do like the idea of populating
based off of the dictionary keys.

>> diff --git a/tests/qemu-iotests/158.out b/tests/qemu-iotests/158.out
>> new file mode 100644
>> index 0000000..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:
>>
> 
> Seems fine otherwise; let's figure out how to check in the sample DMG
> image we have (that Mark Cave-Ayland made for us, thank you!) in with
> the series, because it's incomplete without it.
> 
> (The sample dmg image, for everyone else, is about 200kB compressed --
> can we just send it straight to the list?)
> 
No, the dmg image is < 1KB, you're thinking of the luks image that's 200
KB (which also needs to be checked in). There's actually around 5 or 6
other images that also need to be part of this series, but most of them
are < 1KB, luks is kind of the exception here.

> Thanks,
> --js
>