Re: [Qemu-devel] [PATCH 3/4] qemu-iotests: Make block job methods common

[John Snow <jsnow@redhat.com>]

On 05/05/2015 08:46 AM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  tests/qemu-iotests/041        | 66 ++++++++++---------------------------------
>  tests/qemu-iotests/iotests.py | 28 ++++++++++++++++++
>  2 files changed, 43 insertions(+), 51 deletions(-)
> 
> diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
> index 59a8f73..3d46ed7 100755
> --- a/tests/qemu-iotests/041
> +++ b/tests/qemu-iotests/041
> @@ -34,38 +34,8 @@ quorum_img3 = os.path.join(iotests.test_dir, 'quorum3.img')
>  quorum_repair_img = os.path.join(iotests.test_dir, 'quorum_repair.img')
>  quorum_snapshot_file = os.path.join(iotests.test_dir, 'quorum_snapshot.img')
>  
> -class ImageMirroringTestCase(iotests.QMPTestCase):
> -    '''Abstract base class for image mirroring test cases'''
>  
> -    def wait_ready(self, drive='drive0'):
> -        '''Wait until a block job BLOCK_JOB_READY event'''
> -        ready = False
> -        while not ready:
> -            for event in self.vm.get_qmp_events(wait=True):
> -                if event['event'] == 'BLOCK_JOB_READY':
> -                    self.assert_qmp(event, 'data/type', 'mirror')
> -                    self.assert_qmp(event, 'data/device', drive)
> -                    ready = True
> -
> -    def wait_ready_and_cancel(self, drive='drive0'):
> -        self.wait_ready(drive=drive)
> -        event = self.cancel_and_wait(drive=drive)
> -        self.assertEquals(event['event'], 'BLOCK_JOB_COMPLETED')
> -        self.assert_qmp(event, 'data/type', 'mirror')
> -        self.assert_qmp(event, 'data/offset', event['data']['len'])
> -
> -    def complete_and_wait(self, drive='drive0', wait_ready=True):
> -        '''Complete a block job and wait for it to finish'''
> -        if wait_ready:
> -            self.wait_ready(drive=drive)
> -
> -        result = self.vm.qmp('block-job-complete', device=drive)
> -        self.assert_qmp(result, 'return', {})
> -
> -        event = self.wait_until_completed(drive=drive)
> -        self.assert_qmp(event, 'data/type', 'mirror')
> -
> -class TestSingleDrive(ImageMirroringTestCase):
> +class TestSingleDrive(iotests.QMPTestCase):
>      image_len = 1 * 1024 * 1024 # MB
>  
>      def setUp(self):
> @@ -221,17 +191,9 @@ class TestSingleDriveUnalignedLength(TestSingleDrive):
>      test_small_buffer2 = None
>      test_large_cluster = None
>  
> -class TestMirrorNoBacking(ImageMirroringTestCase):
> +class TestMirrorNoBacking(iotests.QMPTestCase):
>      image_len = 2 * 1024 * 1024 # MB
>  
> -    def complete_and_wait(self, drive='drive0', wait_ready=True):
> -        iotests.create_image(target_backing_img, TestMirrorNoBacking.image_len)
> -        return ImageMirroringTestCase.complete_and_wait(self, drive, wait_ready)
> -
> -    def compare_images(self, img1, img2):
> -        iotests.create_image(target_backing_img, TestMirrorNoBacking.image_len)
> -        return iotests.compare_images(img1, img2)
> -
>      def setUp(self):
>          iotests.create_image(backing_img, TestMirrorNoBacking.image_len)
>          qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, test_img)
> @@ -242,7 +204,10 @@ class TestMirrorNoBacking(ImageMirroringTestCase):
>          self.vm.shutdown()
>          os.remove(test_img)
>          os.remove(backing_img)
> -        os.remove(target_backing_img)
> +        try:
> +            os.remove(target_backing_img)
> +        except:
> +            pass
>          os.remove(target_img)
>  
>      def test_complete(self):
> @@ -257,7 +222,7 @@ class TestMirrorNoBacking(ImageMirroringTestCase):
>          result = self.vm.qmp('query-block')
>          self.assert_qmp(result, 'return[0]/inserted/file', target_img)
>          self.vm.shutdown()
> -        self.assertTrue(self.compare_images(test_img, target_img),
> +        self.assertTrue(iotests.compare_images(test_img, target_img),
>                          'target image does not match source after mirroring')
>  
>      def test_cancel(self):
> @@ -272,7 +237,7 @@ class TestMirrorNoBacking(ImageMirroringTestCase):
>          result = self.vm.qmp('query-block')
>          self.assert_qmp(result, 'return[0]/inserted/file', test_img)
>          self.vm.shutdown()
> -        self.assertTrue(self.compare_images(test_img, target_img),
> +        self.assertTrue(iotests.compare_images(test_img, target_img),
>                          'target image does not match source after mirroring')
>  
>      def test_large_cluster(self):
> @@ -283,7 +248,6 @@ class TestMirrorNoBacking(ImageMirroringTestCase):
>                          %(TestMirrorNoBacking.image_len), target_backing_img)
>          qemu_img('create', '-f', iotests.imgfmt, '-o', 'cluster_size=%d,backing_file=%s'
>                          % (TestMirrorNoBacking.image_len, target_backing_img), target_img)
> -        os.remove(target_backing_img)
>  
>          result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
>                               mode='existing', target=target_img)
> @@ -293,10 +257,10 @@ class TestMirrorNoBacking(ImageMirroringTestCase):
>          result = self.vm.qmp('query-block')
>          self.assert_qmp(result, 'return[0]/inserted/file', target_img)
>          self.vm.shutdown()
> -        self.assertTrue(self.compare_images(test_img, target_img),
> +        self.assertTrue(iotests.compare_images(test_img, target_img),
>                          'target image does not match source after mirroring')
>  
> -class TestMirrorResized(ImageMirroringTestCase):
> +class TestMirrorResized(iotests.QMPTestCase):
>      backing_len = 1 * 1024 * 1024 # MB
>      image_len = 2 * 1024 * 1024 # MB
>  
> @@ -344,7 +308,7 @@ class TestMirrorResized(ImageMirroringTestCase):
>          self.assertTrue(iotests.compare_images(test_img, target_img),
>                          'target image does not match source after mirroring')
>  
> -class TestReadErrors(ImageMirroringTestCase):
> +class TestReadErrors(iotests.QMPTestCase):
>      image_len = 2 * 1024 * 1024 # MB
>  
>      # this should be a multiple of twice the default granularity
> @@ -498,7 +462,7 @@ new_state = "1"
>          self.assert_no_active_block_jobs()
>          self.vm.shutdown()
>  
> -class TestWriteErrors(ImageMirroringTestCase):
> +class TestWriteErrors(iotests.QMPTestCase):
>      image_len = 2 * 1024 * 1024 # MB
>  
>      # this should be a multiple of twice the default granularity
> @@ -624,7 +588,7 @@ new_state = "1"
>          self.assert_no_active_block_jobs()
>          self.vm.shutdown()
>  
> -class TestSetSpeed(ImageMirroringTestCase):
> +class TestSetSpeed(iotests.QMPTestCase):
>      image_len = 80 * 1024 * 1024 # MB
>  
>      def setUp(self):
> @@ -690,7 +654,7 @@ class TestSetSpeed(ImageMirroringTestCase):
>  
>          self.wait_ready_and_cancel()
>  
> -class TestUnbackedSource(ImageMirroringTestCase):
> +class TestUnbackedSource(iotests.QMPTestCase):
>      image_len = 2 * 1024 * 1024 # MB
>  
>      def setUp(self):
> @@ -731,7 +695,7 @@ class TestUnbackedSource(ImageMirroringTestCase):
>          self.complete_and_wait()
>          self.assert_no_active_block_jobs()
>  
> -class TestRepairQuorum(ImageMirroringTestCase):
> +class TestRepairQuorum(iotests.QMPTestCase):
>      """ This class test quorum file repair using drive-mirror.
>          It's mostly a fork of TestSingleDrive """
>      image_len = 1 * 1024 * 1024 # MB
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index e93e623..2e07cc4 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -326,6 +326,34 @@ class QMPTestCase(unittest.TestCase):
>          self.assert_no_active_block_jobs()
>          return event
>  
> +    def wait_ready(self, drive='drive0'):
> +        '''Wait until a block job BLOCK_JOB_READY event'''
> +        ready = False
> +        while not ready:
> +            for event in self.vm.get_qmp_events(wait=True):
> +                if event['event'] == 'BLOCK_JOB_READY':
> +                    self.assert_qmp(event, 'data/type', 'mirror')
> +                    self.assert_qmp(event, 'data/device', drive)
> +                    ready = True
> +

I know you only moved the code, but since it's going into the general
purpose iotests instead of inside of a specific test case...

get_qmp_events will completely empty the queue, but we're only
interested in one specific event. This may make writing tests harder
depending on what order events arrive in.

You may want to rely upon self.event_wait() instead, which allows you to
specify an event to match, and any events it finds that don't match this
are left alone to be pulled by future calls.

Something like this: (not tested...)

def wait_ready(self, drive='drive0'):
  filter = {'data': {'type': 'mirror', 'device': drive } }
  event = self.event_wait(BLOCK_JOB_READY, match=filter)

This way we can use wait_ready to test in asynchronous cases with other
stuff flying around.

(Hmm, looks like I never converted the existing users of get_qmp_events,
which can/would cause similar difficulties...)

> +    def wait_ready_and_cancel(self, drive='drive0'):
> +        self.wait_ready(drive=drive)
> +        event = self.cancel_and_wait(drive=drive)
> +        self.assertEquals(event['event'], 'BLOCK_JOB_COMPLETED')
> +        self.assert_qmp(event, 'data/type', 'mirror')
> +        self.assert_qmp(event, 'data/offset', event['data']['len'])
> +
> +    def complete_and_wait(self, drive='drive0', wait_ready=True):
> +        '''Complete a block job and wait for it to finish'''
> +        if wait_ready:
> +            self.wait_ready(drive=drive)
> +
> +        result = self.vm.qmp('block-job-complete', device=drive)
> +        self.assert_qmp(result, 'return', {})
> +
> +        event = self.wait_until_completed(drive=drive)
> +        self.assert_qmp(event, 'data/type', 'mirror')
> +
>  def notrun(reason):
>      '''Skip this test suite'''
>      # Each test in qemu-iotests has a number ("seq")
>