Re: [Qemu-block] [PATCH 5/5] iotest 030: add compressed block-stream test

[Max Reitz]

On 2017-11-14 11:16, Anton Nefedov wrote:
> Signed-off-by: Anton Nefedov <address@hidden>
> ---
>  tests/qemu-iotests/030     | 69 
> +++++++++++++++++++++++++++++++++++++++++++++-
>  tests/qemu-iotests/030.out |  4 +--
>  2 files changed, 70 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
> index 1883894..52cbe5d 100755
> --- a/tests/qemu-iotests/030
> +++ b/tests/qemu-iotests/030
> @@ -21,7 +21,7 @@
>  import time
>  import os
>  import iotests
> -from iotests import qemu_img, qemu_io
> +from iotests import qemu_img, qemu_img_pipe, qemu_io
>  
>  backing_img = os.path.join(iotests.test_dir, 'backing.img')
>  mid_img = os.path.join(iotests.test_dir, 'mid.img')
> @@ -800,5 +800,72 @@ class TestSetSpeed(iotests.QMPTestCase):
>  
>          self.cancel_and_wait()
>  
> +class TestCompressed(iotests.QMPTestCase):
> +    supported_fmts = ['qcow2']
> +    cluster_size = 64 * 1024;
> +    image_len = 1 * 1024 * 1024;
> +
> +    def setUp(self):
> +        qemu_img('create', backing_img, str(TestCompressed.image_len))

First, this is missing the '-f raw', if you want to keep it in line with
the next command.

> +        qemu_io('-f', 'raw', '-c', 'write -P 1 0 ' + 
> str(TestCompressed.image_len), backing_img)

But why would you want this to be raw?

> +        qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % 
> backing_img, test_img)
> +
> +        # write '4' in every 4th cluster
> +        step=4

I'd prefer spaces around operators.  (Also in _pattern() and in the call
to pattern ([1,4,2] should be [1, 4, 2]).)

> +        for i in range(TestCompressed.image_len / 
> TestCompressed.cluster_size / step + 1):

As far as I know, range() has an actual step parameter, maybe that would
be simpler -- and I don't know what the +1 is supposed to mean.

How about "for ofs in range(0, TestCompressed.image_len,
                            TestCompressed.cluster_size * step)"?
Would that work?

> +            qemu_io('-c', 'write -P %d %d %d' %
> +                    (step, step * i * TestCompressed.cluster_size,> +        
>              TestCompressed.cluster_size),
> +                    test_img)
> +
> +        self.vm = iotests.VM().add_drive(test_img)
> +        self.vm.launch()
> +
> +    def tearDown(self):
> +        os.remove(test_img)
> +        os.remove(backing_img)
> +
> +    def _pattern(self, x, divs):
> +        return divs[-1] if not x%divs[-1] else self._pattern(x, divs[:-1])

I have no idea what this function does.

...OK, having looked into how it's used, I understand better.  A comment
would certainly be helpful, though.

Maybe also call it differently.  It doesn't really return a pattern.
What this function does is it returns the first integer from @divs
(starting from the end, which is weird) which is a divider of @x.  So
maybe call it _first_divider(self, x, dividers), that would help already.

> +
> +    def test_compressed(self):
> +        self.assert_no_active_block_jobs()
> +
> +        result = self.vm.qmp('block-stream', device='drive0', compress=True)
> +        if iotests.imgfmt not in TestCompressed.supported_fmts:
> +            self.assert_qmp(
> +                result, 'error/desc',
> +                'Compression is not supported for this drive drive0')
> +            return
> +        self.assert_qmp(result, 'return', {})
> +
> +        # write '2' in every 2nd cluster
> +        step = 2
> +        for i in range(TestCompressed.image_len / 
> TestCompressed.cluster_size / step + 1):
> +            result = self.vm.qmp('human-monitor-command',
> +                                 command_line=
> +                                 'qemu-io drive0 \"write -P %d %d %d\"' %

Just " instead of \" would be enough, I think.

> +                                 (step, step * i * 
> TestCompressed.cluster_size,
> +                                  TestCompressed.cluster_size))
> +            self.assert_qmp(result, 'return', "")
> +
> +        self.wait_until_completed()
> +        self.assert_no_active_block_jobs()
> +
> +        self.vm.shutdown()
> +
> +        for i in range(TestCompressed.image_len / 
> TestCompressed.cluster_size):
> +            outp = qemu_io('-c', 'read -P %d %d %d' %
> +                           (self._pattern(i, [1,4,2]),

The 4 is useless, because everything that's divisible by 4 is divisible
by 2 already.

Also, I don't quite see what this should achieve exactly.  You first
write the backing image full of 1s, OK.  Then you write 4s to every
fourth cluster in the top image.  Then you start streaming, and then you
write 2s to ever second cluster in the top image, which overwrites all
of the 4s you wrote.

Do you maybe not want to write 2 into every second cluster of the
backing image in setUp() instead of 4 into the top image?  (And then 4th
into every fourth cluster here in test_compressed().)  Then you would
need [1, 2, 4] here, which makes much more sense.

Max

> +                            i * TestCompressed.cluster_size,
> +                            TestCompressed.cluster_size),
> +                           test_img)
> +            self.assertTrue(not 'fail' in outp)
> +            self.assertTrue('read' in outp and 'at offset' in outp)
> +
> +        self.assertTrue(
> +            "File contains external, encrypted or compressed clusters."
> +            in qemu_img_pipe('map', test_img))
> +
>  if __name__ == '__main__':
>      iotests.main(supported_fmts=['qcow2', 'qed'])
> diff --git a/tests/qemu-iotests/030.out b/tests/qemu-iotests/030.out
> index 391c857..42314e9 100644
> --- a/tests/qemu-iotests/030.out
> +++ b/tests/qemu-iotests/030.out
> @@ -1,5 +1,5 @@
> -.......................
> +........................
>  ----------------------------------------------------------------------
> -Ran 23 tests
> +Ran 24 tests
>  
>  OK
>

signature.asc
Description: OpenPGP digital signature