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

Message ID 1510654613-47868-6-git-send-email-anton.nefedov@virtuozzo.com
State New
Headers show
Series
  • compressed block-stream
Related show

Commit Message

Anton Nefedov Nov. 14, 2017, 10:16 a.m.
Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
---
 tests/qemu-iotests/030     | 69 +++++++++++++++++++++++++++++++++++++++++++++-
 tests/qemu-iotests/030.out |  4 +--
 2 files changed, 70 insertions(+), 3 deletions(-)

Comments

Max Reitz Nov. 15, 2017, 7:51 p.m. | #1
On 2017-11-14 11:16, Anton Nefedov wrote:
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> ---
>  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
>
Anton Nefedov Nov. 16, 2017, 1:15 p.m. | #2
On 15/11/2017 10:51 PM, Max Reitz wrote:
> On 2017-11-14 11:16, Anton Nefedov wrote:
>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>> ---
>>   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?
> 

Copied this from another test in this file :)
The source image format does not really matter. Will fix though.

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

It does, thank you.

>> +            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.
> 

Yeah, I really had to comment.

Exactly, it starts from the end as I meant it to accept numbers in the 
order they were written. So 'first_divider' wouldn't be right.
Not that important though, will rename and reorder the input.


>> +
>> +    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.
> 
Done.

>> +                                 (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
> 

Then the top image would be empty before stream starts.
Not very good as qmp-driven writes may be late and just overwrite
clusters of the image fully copied from backing.

I meant the concurrent write touching both top and backing clusters.
2 and 4 were a bad choice though.

I think setUp() should write every 3 to the top (so we have unallocated 
cluster pairs to cover qcow2 writing several clusters compressed).
And test_compressed() write every 4 with qmp.

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

Patch

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))
+        qemu_io('-f', 'raw', '-c', 'write -P 1 0 ' + str(TestCompressed.image_len), backing_img)
+        qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, test_img)
+
+        # write '4' in every 4th cluster
+        step=4
+        for i in range(TestCompressed.image_len / TestCompressed.cluster_size / step + 1):
+            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])
+
+    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\"' %
+                                 (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]),
+                            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