Re: [Qemu-devel] [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 <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.

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

Thanks,
--js