[Qemu-devel] [PATCH 0/4] qemu-iotests: reduce iotests.py code duplication

[Qemu-devel] [PATCH 0/4] qemu-iotests: reduce iotests.py code duplication

[Stefan Hajnoczi]

Tests 030 and 041 are written in Python and use the iotests.py module.  Push
common code down into iotests.py.  My 'drive-backup' series takes advantage
will take advantage of this - kwolf pointed out that earlier revisions were
duplicating too much code.

I decided to split this into a separate patch series and send it before the
next 'drive-backup' revision.

Stefan Hajnoczi (4):
  qemu-iotests: make assert_no_active_block_jobs() common
  qemu-iotests: make cancel_and_wait() common
  qemu-iotests: make compare_images() common
  qemu-iotests: make create_image() common

 tests/qemu-iotests/030        |  99 ++++++++---------------
 tests/qemu-iotests/041        | 181 +++++++++++++++---------------------------
 tests/qemu-iotests/iotests.py |  38 +++++++++
 3 files changed, 135 insertions(+), 183 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH 1/4] qemu-iotests: make assert_no_active_block_jobs() common

[Stefan Hajnoczi]

Tests 030 and 041 both use query-block-jobs to check whether any block
jobs are active.  Make this code common so that 'drive-backup' and other
new feature tests will be able to reuse it.

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qemu-iotests/030        | 54 ++++++++++++++++++--------------------
 tests/qemu-iotests/041        | 60 ++++++++++++++++++++-----------------------
 tests/qemu-iotests/iotests.py |  4 +++
 3 files changed, 57 insertions(+), 61 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index dd4ef11..03dd6a6 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -31,10 +31,6 @@ test_img = os.path.join(iotests.test_dir, 'test.img')
 class ImageStreamingTestCase(iotests.QMPTestCase):
     '''Abstract base class for image streaming test cases'''
 
-    def assert_no_active_streams(self):
-        result = self.vm.qmp('query-block-jobs')
-        self.assert_qmp(result, 'return', [])
-
     def cancel_and_wait(self, drive='drive0'):
         '''Cancel a block job and wait for it to finish'''
         result = self.vm.qmp('block-job-cancel', device=drive)
@@ -48,7 +44,7 @@ class ImageStreamingTestCase(iotests.QMPTestCase):
                     self.assert_qmp(event, 'data/device', drive)
                     cancelled = True
 
-        self.assert_no_active_streams()
+        self.assert_no_active_block_jobs()
 
     def create_image(self, name, size):
         file = open(name, 'w')
@@ -77,7 +73,7 @@ class TestSingleDrive(ImageStreamingTestCase):
         os.remove(backing_img)
 
     def test_stream(self):
-        self.assert_no_active_streams()
+        self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('block-stream', device='drive0')
         self.assert_qmp(result, 'return', {})
@@ -92,7 +88,7 @@ class TestSingleDrive(ImageStreamingTestCase):
                     self.assert_qmp(event, 'data/len', self.image_len)
                     completed = True
 
-        self.assert_no_active_streams()
+        self.assert_no_active_block_jobs()
         self.vm.shutdown()
 
         self.assertEqual(qemu_io('-c', 'map', backing_img),
@@ -100,7 +96,7 @@ class TestSingleDrive(ImageStreamingTestCase):
                          'image file map does not match backing file after streaming')
 
     def test_stream_pause(self):
-        self.assert_no_active_streams()
+        self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('block-stream', device='drive0')
         self.assert_qmp(result, 'return', {})
@@ -129,7 +125,7 @@ class TestSingleDrive(ImageStreamingTestCase):
                     self.assert_qmp(event, 'data/len', self.image_len)
                     completed = True
 
-        self.assert_no_active_streams()
+        self.assert_no_active_block_jobs()
         self.vm.shutdown()
 
         self.assertEqual(qemu_io('-c', 'map', backing_img),
@@ -137,7 +133,7 @@ class TestSingleDrive(ImageStreamingTestCase):
                          'image file map does not match backing file after streaming')
 
     def test_stream_partial(self):
-        self.assert_no_active_streams()
+        self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('block-stream', device='drive0', base=mid_img)
         self.assert_qmp(result, 'return', {})
@@ -152,7 +148,7 @@ class TestSingleDrive(ImageStreamingTestCase):
                     self.assert_qmp(event, 'data/len', self.image_len)
                     completed = True
 
-        self.assert_no_active_streams()
+        self.assert_no_active_block_jobs()
         self.vm.shutdown()
 
         self.assertEqual(qemu_io('-c', 'map', mid_img),
@@ -177,7 +173,7 @@ class TestSmallerBackingFile(ImageStreamingTestCase):
     # If this hangs, then you are missing a fix to complete streaming when the
     # end of the backing file is reached.
     def test_stream(self):
-        self.assert_no_active_streams()
+        self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('block-stream', device='drive0')
         self.assert_qmp(result, 'return', {})
@@ -192,7 +188,7 @@ class TestSmallerBackingFile(ImageStreamingTestCase):
                     self.assert_qmp(event, 'data/len', self.image_len)
                     completed = True
 
-        self.assert_no_active_streams()
+        self.assert_no_active_block_jobs()
         self.vm.shutdown()
 
 class TestErrors(ImageStreamingTestCase):
@@ -243,7 +239,7 @@ class TestEIO(TestErrors):
         os.remove(self.blkdebug_file)
 
     def test_report(self):
-        self.assert_no_active_streams()
+        self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('block-stream', device='drive0')
         self.assert_qmp(result, 'return', {})
@@ -265,11 +261,11 @@ class TestEIO(TestErrors):
                     self.assert_qmp(event, 'data/len', self.image_len)
                     completed = True
 
-        self.assert_no_active_streams()
+        self.assert_no_active_block_jobs()
         self.vm.shutdown()
 
     def test_ignore(self):
-        self.assert_no_active_streams()
+        self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('block-stream', device='drive0', on_error='ignore')
         self.assert_qmp(result, 'return', {})
@@ -293,11 +289,11 @@ class TestEIO(TestErrors):
                     self.assert_qmp(event, 'data/len', self.image_len)
                     completed = True
 
-        self.assert_no_active_streams()
+        self.assert_no_active_block_jobs()
         self.vm.shutdown()
 
     def test_stop(self):
-        self.assert_no_active_streams()
+        self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('block-stream', device='drive0', on_error='stop')
         self.assert_qmp(result, 'return', {})
@@ -331,11 +327,11 @@ class TestEIO(TestErrors):
                     self.assert_qmp(event, 'data/len', self.image_len)
                     completed = True
 
-        self.assert_no_active_streams()
+        self.assert_no_active_block_jobs()
         self.vm.shutdown()
 
     def test_enospc(self):
-        self.assert_no_active_streams()
+        self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('block-stream', device='drive0', on_error='enospc')
         self.assert_qmp(result, 'return', {})
@@ -357,7 +353,7 @@ class TestEIO(TestErrors):
                     self.assert_qmp(event, 'data/len', self.image_len)
                     completed = True
 
-        self.assert_no_active_streams()
+        self.assert_no_active_block_jobs()
         self.vm.shutdown()
 
 class TestENOSPC(TestErrors):
@@ -379,7 +375,7 @@ class TestENOSPC(TestErrors):
         os.remove(self.blkdebug_file)
 
     def test_enospc(self):
-        self.assert_no_active_streams()
+        self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('block-stream', device='drive0', on_error='enospc')
         self.assert_qmp(result, 'return', {})
@@ -413,7 +409,7 @@ class TestENOSPC(TestErrors):
                     self.assert_qmp(event, 'data/len', self.image_len)
                     completed = True
 
-        self.assert_no_active_streams()
+        self.assert_no_active_block_jobs()
         self.vm.shutdown()
 
 class TestStreamStop(ImageStreamingTestCase):
@@ -431,7 +427,7 @@ class TestStreamStop(ImageStreamingTestCase):
         os.remove(backing_img)
 
     def test_stream_stop(self):
-        self.assert_no_active_streams()
+        self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('block-stream', device='drive0')
         self.assert_qmp(result, 'return', {})
@@ -459,7 +455,7 @@ class TestSetSpeed(ImageStreamingTestCase):
     # This is a short performance test which is not run by default.
     # Invoke "IMGFMT=qed ./030 TestSetSpeed.perf_test_throughput"
     def perf_test_throughput(self):
-        self.assert_no_active_streams()
+        self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('block-stream', device='drive0')
         self.assert_qmp(result, 'return', {})
@@ -477,10 +473,10 @@ class TestSetSpeed(ImageStreamingTestCase):
                     self.assert_qmp(event, 'data/len', self.image_len)
                     completed = True
 
-        self.assert_no_active_streams()
+        self.assert_no_active_block_jobs()
 
     def test_set_speed(self):
-        self.assert_no_active_streams()
+        self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('block-stream', device='drive0')
         self.assert_qmp(result, 'return', {})
@@ -511,12 +507,12 @@ class TestSetSpeed(ImageStreamingTestCase):
         self.cancel_and_wait()
 
     def test_set_speed_invalid(self):
-        self.assert_no_active_streams()
+        self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('block-stream', device='drive0', speed=-1)
         self.assert_qmp(result, 'error/class', 'GenericError')
 
-        self.assert_no_active_streams()
+        self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('block-stream', device='drive0')
         self.assert_qmp(result, 'return', {})
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 720eeff..ff89427 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -32,10 +32,6 @@ target_img = os.path.join(iotests.test_dir, 'target.img')
 class ImageMirroringTestCase(iotests.QMPTestCase):
     '''Abstract base class for image mirroring test cases'''
 
-    def assert_no_active_mirrors(self):
-        result = self.vm.qmp('query-block-jobs')
-        self.assert_qmp(result, 'return', [])
-
     def cancel_and_wait(self, drive='drive0', wait_ready=True):
         '''Cancel a block job and wait for it to finish'''
         if wait_ready:
@@ -64,7 +60,7 @@ class ImageMirroringTestCase(iotests.QMPTestCase):
                         self.assert_qmp(event, 'data/len', self.image_len)
                     cancelled = True
 
-        self.assert_no_active_mirrors()
+        self.assert_no_active_block_jobs()
 
     def complete_and_wait(self, drive='drive0', wait_ready=True):
         '''Complete a block job and wait for it to finish'''
@@ -91,7 +87,7 @@ class ImageMirroringTestCase(iotests.QMPTestCase):
                     self.assert_qmp(event, 'data/len', self.image_len)
                     completed = True
 
-        self.assert_no_active_mirrors()
+        self.assert_no_active_block_jobs()
 
     def create_image(self, name, size):
         file = open(name, 'w')
@@ -142,7 +138,7 @@ class TestSingleDrive(ImageMirroringTestCase):
             pass
 
     def test_complete(self):
-        self.assert_no_active_mirrors()
+        self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
                              target=target_img)
@@ -156,7 +152,7 @@ class TestSingleDrive(ImageMirroringTestCase):
                         'target image does not match source after mirroring')
 
     def test_cancel(self):
-        self.assert_no_active_mirrors()
+        self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
                              target=target_img)
@@ -168,7 +164,7 @@ class TestSingleDrive(ImageMirroringTestCase):
         self.vm.shutdown()
 
     def test_cancel_after_ready(self):
-        self.assert_no_active_mirrors()
+        self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
                              target=target_img)
@@ -182,7 +178,7 @@ class TestSingleDrive(ImageMirroringTestCase):
                         'target image does not match source after mirroring')
 
     def test_pause(self):
-        self.assert_no_active_mirrors()
+        self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
                              target=target_img)
@@ -208,7 +204,7 @@ class TestSingleDrive(ImageMirroringTestCase):
                         'target image does not match source after mirroring')
 
     def test_small_buffer(self):
-        self.assert_no_active_mirrors()
+        self.assert_no_active_block_jobs()
 
         # A small buffer is rounded up automatically
         result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
@@ -223,7 +219,7 @@ class TestSingleDrive(ImageMirroringTestCase):
                         'target image does not match source after mirroring')
 
     def test_small_buffer2(self):
-        self.assert_no_active_mirrors()
+        self.assert_no_active_block_jobs()
 
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'cluster_size=%d,size=%d'
                         % (TestSingleDrive.image_len, TestSingleDrive.image_len), target_img)
@@ -239,7 +235,7 @@ class TestSingleDrive(ImageMirroringTestCase):
                         'target image does not match source after mirroring')
 
     def test_large_cluster(self):
-        self.assert_no_active_mirrors()
+        self.assert_no_active_block_jobs()
 
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'cluster_size=%d,backing_file=%s'
                         % (TestSingleDrive.image_len, backing_img), target_img)
@@ -294,7 +290,7 @@ class TestMirrorNoBacking(ImageMirroringTestCase):
         os.remove(target_img)
 
     def test_complete(self):
-        self.assert_no_active_mirrors()
+        self.assert_no_active_block_jobs()
 
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, target_img)
         result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
@@ -309,7 +305,7 @@ class TestMirrorNoBacking(ImageMirroringTestCase):
                         'target image does not match source after mirroring')
 
     def test_cancel(self):
-        self.assert_no_active_mirrors()
+        self.assert_no_active_block_jobs()
 
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, target_img)
         result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
@@ -324,7 +320,7 @@ class TestMirrorNoBacking(ImageMirroringTestCase):
                         'target image does not match source after mirroring')
 
     def test_large_cluster(self):
-        self.assert_no_active_mirrors()
+        self.assert_no_active_block_jobs()
 
         # qemu-img create fails if the image is not there
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'size=%d'
@@ -365,7 +361,7 @@ class TestMirrorResized(ImageMirroringTestCase):
             pass
 
     def test_complete_top(self):
-        self.assert_no_active_mirrors()
+        self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('drive-mirror', device='drive0', sync='top',
                              target=target_img)
@@ -379,7 +375,7 @@ class TestMirrorResized(ImageMirroringTestCase):
                         'target image does not match source after mirroring')
 
     def test_complete_full(self):
-        self.assert_no_active_mirrors()
+        self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
                              target=target_img)
@@ -443,7 +439,7 @@ new_state = "1"
         os.remove(self.blkdebug_file)
 
     def test_report_read(self):
-        self.assert_no_active_mirrors()
+        self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
                              target=target_img)
@@ -467,11 +463,11 @@ new_state = "1"
                     self.assert_qmp(event, 'data/len', self.image_len)
                     completed = True
 
-        self.assert_no_active_mirrors()
+        self.assert_no_active_block_jobs()
         self.vm.shutdown()
 
     def test_ignore_read(self):
-        self.assert_no_active_mirrors()
+        self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
                              target=target_img, on_source_error='ignore')
@@ -487,7 +483,7 @@ new_state = "1"
         self.vm.shutdown()
 
     def test_large_cluster(self):
-        self.assert_no_active_mirrors()
+        self.assert_no_active_block_jobs()
 
         # Test COW into the target image.  The first half of the
         # cluster at MIRROR_GRANULARITY has to be copied from
@@ -513,7 +509,7 @@ new_state = "1"
                         'target image does not match source after mirroring')
 
     def test_stop_read(self):
-        self.assert_no_active_mirrors()
+        self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
                              target=target_img, on_source_error='stop')
@@ -544,7 +540,7 @@ new_state = "1"
         self.assert_qmp(result, 'return[0]/io-status', 'ok')
 
         self.complete_and_wait(wait_ready=False)
-        self.assert_no_active_mirrors()
+        self.assert_no_active_block_jobs()
         self.vm.shutdown()
 
 class TestWriteErrors(ImageMirroringTestCase):
@@ -594,7 +590,7 @@ new_state = "1"
         os.remove(self.blkdebug_file)
 
     def test_report_write(self):
-        self.assert_no_active_mirrors()
+        self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
                              mode='existing', target=self.target_img)
@@ -618,11 +614,11 @@ new_state = "1"
                     self.assert_qmp(event, 'data/len', self.image_len)
                     completed = True
 
-        self.assert_no_active_mirrors()
+        self.assert_no_active_block_jobs()
         self.vm.shutdown()
 
     def test_ignore_write(self):
-        self.assert_no_active_mirrors()
+        self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
                              mode='existing', target=self.target_img,
@@ -639,7 +635,7 @@ new_state = "1"
         self.vm.shutdown()
 
     def test_stop_write(self):
-        self.assert_no_active_mirrors()
+        self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
                              mode='existing', target=self.target_img,
@@ -671,7 +667,7 @@ new_state = "1"
                     ready = True
 
         self.complete_and_wait(wait_ready=False)
-        self.assert_no_active_mirrors()
+        self.assert_no_active_block_jobs()
         self.vm.shutdown()
 
 class TestSetSpeed(ImageMirroringTestCase):
@@ -690,7 +686,7 @@ class TestSetSpeed(ImageMirroringTestCase):
         os.remove(target_img)
 
     def test_set_speed(self):
-        self.assert_no_active_mirrors()
+        self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
                              target=target_img)
@@ -723,13 +719,13 @@ class TestSetSpeed(ImageMirroringTestCase):
         self.cancel_and_wait()
 
     def test_set_speed_invalid(self):
-        self.assert_no_active_mirrors()
+        self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
                              target=target_img, speed=-1)
         self.assert_qmp(result, 'error/class', 'GenericError')
 
-        self.assert_no_active_mirrors()
+        self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
                              target=target_img)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 569ca3d..0e7862c 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -170,6 +170,10 @@ class QMPTestCase(unittest.TestCase):
         result = self.dictpath(d, path)
         self.assertEqual(result, value, 'values not equal "%s" and "%s"' % (str(result), str(value)))
 
+    def assert_no_active_block_jobs(self):
+        result = self.vm.qmp('query-block-jobs')
+        self.assert_qmp(result, 'return', [])
+
 def notrun(reason):
     '''Skip this test suite'''
     # Each test in qemu-iotests has a number ("seq")
-- 
1.8.1.4

[Qemu-devel] [PATCH 2/4] qemu-iotests: make cancel_and_wait() common

[Stefan Hajnoczi]

The cancel_and_wait() function has been duplicated in 030 and 041.  Move
it into iotests.py and let it return the event so tests can perform
additional asserts.

Note that 041's cancel_and_wait(wait_ready=True) is replaced by
wait_ready_and_cancel(), which uses the new wait_ready() and
cancel_and_wait() underneath.

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qemu-iotests/030        | 15 -----------
 tests/qemu-iotests/041        | 58 +++++++++++++++----------------------------
 tests/qemu-iotests/iotests.py | 18 ++++++++++++++
 3 files changed, 38 insertions(+), 53 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 03dd6a6..1f55095 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -31,21 +31,6 @@ test_img = os.path.join(iotests.test_dir, 'test.img')
 class ImageStreamingTestCase(iotests.QMPTestCase):
     '''Abstract base class for image streaming test cases'''
 
-    def cancel_and_wait(self, drive='drive0'):
-        '''Cancel a block job and wait for it to finish'''
-        result = self.vm.qmp('block-job-cancel', device=drive)
-        self.assert_qmp(result, 'return', {})
-
-        cancelled = False
-        while not cancelled:
-            for event in self.vm.get_qmp_events(wait=True):
-                if event['event'] == 'BLOCK_JOB_CANCELLED':
-                    self.assert_qmp(event, 'data/type', 'stream')
-                    self.assert_qmp(event, 'data/device', drive)
-                    cancelled = True
-
-        self.assert_no_active_block_jobs()
-
     def create_image(self, name, size):
         file = open(name, 'w')
         i = 0
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index ff89427..c4ce75e 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -32,46 +32,28 @@ target_img = os.path.join(iotests.test_dir, 'target.img')
 class ImageMirroringTestCase(iotests.QMPTestCase):
     '''Abstract base class for image mirroring test cases'''
 
-    def cancel_and_wait(self, drive='drive0', wait_ready=True):
-        '''Cancel a block job and wait for it to finish'''
-        if wait_ready:
-            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
-
-        result = self.vm.qmp('block-job-cancel', device=drive,
-                             force=not wait_ready)
-        self.assert_qmp(result, 'return', {})
-
-        cancelled = False
-        while not cancelled:
+    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_COMPLETED' or \
-                   event['event'] == 'BLOCK_JOB_CANCELLED':
+                if event['event'] == 'BLOCK_JOB_READY':
                     self.assert_qmp(event, 'data/type', 'mirror')
                     self.assert_qmp(event, 'data/device', drive)
-                    if wait_ready:
-                        self.assertEquals(event['event'], 'BLOCK_JOB_COMPLETED')
-                        self.assert_qmp(event, 'data/offset', self.image_len)
-                        self.assert_qmp(event, 'data/len', self.image_len)
-                    cancelled = True
+                    ready = True
 
-        self.assert_no_active_block_jobs()
+    def wait_ready_and_cancel(self, drive='drive0'):
+        self.wait_ready(drive)
+        event = self.cancel_and_wait()
+        self.assertEquals(event['event'], 'BLOCK_JOB_COMPLETED')
+        self.assert_qmp(event, 'data/type', 'mirror')
+        self.assert_qmp(event, 'data/offset', self.image_len)
+        self.assert_qmp(event, 'data/len', self.image_len)
 
     def complete_and_wait(self, drive='drive0', wait_ready=True):
         '''Complete a block job and wait for it to finish'''
         if wait_ready:
-            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
+            self.wait_ready()
 
         result = self.vm.qmp('block-job-complete', device=drive)
         self.assert_qmp(result, 'return', {})
@@ -158,7 +140,7 @@ class TestSingleDrive(ImageMirroringTestCase):
                              target=target_img)
         self.assert_qmp(result, 'return', {})
 
-        self.cancel_and_wait(wait_ready=False)
+        self.cancel_and_wait(force=True)
         result = self.vm.qmp('query-block')
         self.assert_qmp(result, 'return[0]/inserted/file', test_img)
         self.vm.shutdown()
@@ -170,7 +152,7 @@ class TestSingleDrive(ImageMirroringTestCase):
                              target=target_img)
         self.assert_qmp(result, 'return', {})
 
-        self.cancel_and_wait()
+        self.wait_ready_and_cancel()
         result = self.vm.qmp('query-block')
         self.assert_qmp(result, 'return[0]/inserted/file', test_img)
         self.vm.shutdown()
@@ -312,7 +294,7 @@ class TestMirrorNoBacking(ImageMirroringTestCase):
                              mode='existing', target=target_img)
         self.assert_qmp(result, 'return', {})
 
-        self.cancel_and_wait()
+        self.wait_ready_and_cancel()
         result = self.vm.qmp('query-block')
         self.assert_qmp(result, 'return[0]/inserted/file', test_img)
         self.vm.shutdown()
@@ -705,7 +687,7 @@ class TestSetSpeed(ImageMirroringTestCase):
         self.assert_qmp(result, 'return[0]/device', 'drive0')
         self.assert_qmp(result, 'return[0]/speed', 8 * 1024 * 1024)
 
-        self.cancel_and_wait()
+        self.wait_ready_and_cancel()
 
         # Check setting speed in drive-mirror works
         result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
@@ -716,7 +698,7 @@ class TestSetSpeed(ImageMirroringTestCase):
         self.assert_qmp(result, 'return[0]/device', 'drive0')
         self.assert_qmp(result, 'return[0]/speed', 4 * 1024 * 1024)
 
-        self.cancel_and_wait()
+        self.wait_ready_and_cancel()
 
     def test_set_speed_invalid(self):
         self.assert_no_active_block_jobs()
@@ -734,7 +716,7 @@ class TestSetSpeed(ImageMirroringTestCase):
         result = self.vm.qmp('block-job-set-speed', device='drive0', speed=-1)
         self.assert_qmp(result, 'error/class', 'GenericError')
 
-        self.cancel_and_wait()
+        self.wait_ready_and_cancel()
 
 if __name__ == '__main__':
     iotests.main(supported_fmts=['qcow2', 'qed'])
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 0e7862c..bc9c71b 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -174,6 +174,24 @@ class QMPTestCase(unittest.TestCase):
         result = self.vm.qmp('query-block-jobs')
         self.assert_qmp(result, 'return', [])
 
+    def cancel_and_wait(self, drive='drive0', force=False):
+        '''Cancel a block job and wait for it to finish, returning the event'''
+        result = self.vm.qmp('block-job-cancel', device=drive, force=force)
+        self.assert_qmp(result, 'return', {})
+
+        cancelled = False
+        result = None
+        while not cancelled:
+            for event in self.vm.get_qmp_events(wait=True):
+                if event['event'] == 'BLOCK_JOB_COMPLETED' or \
+                   event['event'] == 'BLOCK_JOB_CANCELLED':
+                    self.assert_qmp(event, 'data/device', drive)
+                    result = event
+                    cancelled = True
+
+        self.assert_no_active_block_jobs()
+        return result
+
 def notrun(reason):
     '''Skip this test suite'''
     # Each test in qemu-iotests has a number ("seq")
-- 
1.8.1.4

[Qemu-devel] [PATCH 3/4] qemu-iotests: make compare_images() common

[Stefan Hajnoczi]

The iotests.compare_images() function returns True if two image files
have the identical data.  Previously this was implemented by converting
images to raw and then comparing their contents using Python.  Since
"qemu-img compare" is now available and is more efficient, switch to it.

This function will be reused by the 'drive-backup' test case.

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qemu-iotests/041        | 41 ++++++++++-------------------------------
 tests/qemu-iotests/iotests.py |  5 +++++
 2 files changed, 15 insertions(+), 31 deletions(-)

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index c4ce75e..7702074 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -80,27 +80,6 @@ class ImageMirroringTestCase(iotests.QMPTestCase):
             i = i + 512
         file.close()
 
-    def compare_images(self, img1, img2):
-        try:
-            qemu_img('convert', '-f', iotests.imgfmt, '-O', 'raw', img1, img1 + '.raw')
-            qemu_img('convert', '-f', iotests.imgfmt, '-O', 'raw', img2, img2 + '.raw')
-            file1 = open(img1 + '.raw', 'r')
-            file2 = open(img2 + '.raw', 'r')
-            return file1.read() == file2.read()
-        finally:
-            if file1 is not None:
-                file1.close()
-            if file2 is not None:
-                file2.close()
-            try:
-                os.remove(img1 + '.raw')
-            except OSError:
-                pass
-            try:
-                os.remove(img2 + '.raw')
-            except OSError:
-                pass
-
 class TestSingleDrive(ImageMirroringTestCase):
     image_len = 1 * 1024 * 1024 # MB
 
@@ -130,7 +109,7 @@ class TestSingleDrive(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):
@@ -156,7 +135,7 @@ class TestSingleDrive(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_pause(self):
@@ -182,7 +161,7 @@ class TestSingleDrive(ImageMirroringTestCase):
 
         self.complete_and_wait()
         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_small_buffer(self):
@@ -197,7 +176,7 @@ class TestSingleDrive(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_small_buffer2(self):
@@ -213,7 +192,7 @@ class TestSingleDrive(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_large_cluster(self):
@@ -229,7 +208,7 @@ class TestSingleDrive(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_medium_not_found(self):
@@ -256,7 +235,7 @@ class TestMirrorNoBacking(ImageMirroringTestCase):
 
     def compare_images(self, img1, img2):
         self.create_image(target_backing_img, TestMirrorNoBacking.image_len)
-        return ImageMirroringTestCase.compare_images(self, img1, img2)
+        return iotests.compare_images(img1, img2)
 
     def setUp(self):
         self.create_image(backing_img, TestMirrorNoBacking.image_len)
@@ -353,7 +332,7 @@ class TestMirrorResized(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_complete_full(self):
@@ -367,7 +346,7 @@ class TestMirrorResized(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 TestReadErrors(ImageMirroringTestCase):
@@ -487,7 +466,7 @@ new_state = "1"
 
         # Detach blkdebug to compare images successfully
         qemu_img('rebase', '-f', iotests.imgfmt, '-u', '-b', backing_img, test_img)
-        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_stop_read(self):
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index bc9c71b..733b82b 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -51,6 +51,11 @@ def qemu_io(*args):
     args = qemu_io_args + list(args)
     return subprocess.Popen(args, stdout=subprocess.PIPE).communicate()[0]
 
+def compare_images(img1, img2):
+    '''Return True if two image files are identical'''
+    return qemu_img('compare', '-f', imgfmt,
+                    '-F', imgfmt, img1, img2) == 0
+
 class VM(object):
     '''A QEMU VM'''
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 4/4] qemu-iotests: make create_image() common

[Stefan Hajnoczi]

Both 030 and 041 use create_image().  Move it to iotests.py.

Also drop ImageStreamingTestCase since the class now has no methods.

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qemu-iotests/030        | 32 +++++++++-----------------------
 tests/qemu-iotests/041        | 24 +++++++-----------------
 tests/qemu-iotests/iotests.py | 11 +++++++++++
 3 files changed, 27 insertions(+), 40 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 1f55095..ae56f3b 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -22,30 +22,16 @@ import time
 import os
 import iotests
 from iotests import qemu_img, qemu_io
-import struct
 
 backing_img = os.path.join(iotests.test_dir, 'backing.img')
 mid_img = os.path.join(iotests.test_dir, 'mid.img')
 test_img = os.path.join(iotests.test_dir, 'test.img')
 
-class ImageStreamingTestCase(iotests.QMPTestCase):
-    '''Abstract base class for image streaming test cases'''
-
-    def create_image(self, name, size):
-        file = open(name, 'w')
-        i = 0
-        while i < size:
-            sector = struct.pack('>l504xl', i / 512, i / 512)
-            file.write(sector)
-            i = i + 512
-        file.close()
-
-
-class TestSingleDrive(ImageStreamingTestCase):
+class TestSingleDrive(iotests.QMPTestCase):
     image_len = 1 * 1024 * 1024 # MB
 
     def setUp(self):
-        self.create_image(backing_img, TestSingleDrive.image_len)
+        iotests.create_image(backing_img, TestSingleDrive.image_len)
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, mid_img)
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % mid_img, test_img)
         self.vm = iotests.VM().add_drive(test_img)
@@ -145,12 +131,12 @@ class TestSingleDrive(ImageStreamingTestCase):
         self.assert_qmp(result, 'error/class', 'DeviceNotFound')
 
 
-class TestSmallerBackingFile(ImageStreamingTestCase):
+class TestSmallerBackingFile(iotests.QMPTestCase):
     backing_len = 1 * 1024 * 1024 # MB
     image_len = 2 * backing_len
 
     def setUp(self):
-        self.create_image(backing_img, self.backing_len)
+        iotests.create_image(backing_img, self.backing_len)
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, test_img, str(self.image_len))
         self.vm = iotests.VM().add_drive(test_img)
         self.vm.launch()
@@ -176,7 +162,7 @@ class TestSmallerBackingFile(ImageStreamingTestCase):
         self.assert_no_active_block_jobs()
         self.vm.shutdown()
 
-class TestErrors(ImageStreamingTestCase):
+class TestErrors(iotests.QMPTestCase):
     image_len = 2 * 1024 * 1024 # MB
 
     # this should match STREAM_BUFFER_SIZE/512 in block/stream.c
@@ -208,7 +194,7 @@ new_state = "1"
 class TestEIO(TestErrors):
     def setUp(self):
         self.blkdebug_file = backing_img + ".blkdebug"
-        self.create_image(backing_img, TestErrors.image_len)
+        iotests.create_image(backing_img, TestErrors.image_len)
         self.create_blkdebug_file(self.blkdebug_file, "read_aio", 5)
         qemu_img('create', '-f', iotests.imgfmt,
                  '-o', 'backing_file=blkdebug:%s:%s,backing_fmt=raw'
@@ -344,7 +330,7 @@ class TestEIO(TestErrors):
 class TestENOSPC(TestErrors):
     def setUp(self):
         self.blkdebug_file = backing_img + ".blkdebug"
-        self.create_image(backing_img, TestErrors.image_len)
+        iotests.create_image(backing_img, TestErrors.image_len)
         self.create_blkdebug_file(self.blkdebug_file, "read_aio", 28)
         qemu_img('create', '-f', iotests.imgfmt,
                  '-o', 'backing_file=blkdebug:%s:%s,backing_fmt=raw'
@@ -397,7 +383,7 @@ class TestENOSPC(TestErrors):
         self.assert_no_active_block_jobs()
         self.vm.shutdown()
 
-class TestStreamStop(ImageStreamingTestCase):
+class TestStreamStop(iotests.QMPTestCase):
     image_len = 8 * 1024 * 1024 * 1024 # GB
 
     def setUp(self):
@@ -423,7 +409,7 @@ class TestStreamStop(ImageStreamingTestCase):
 
         self.cancel_and_wait()
 
-class TestSetSpeed(ImageStreamingTestCase):
+class TestSetSpeed(iotests.QMPTestCase):
     image_len = 80 * 1024 * 1024 # MB
 
     def setUp(self):
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 7702074..1e923e7 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -22,7 +22,6 @@ import time
 import os
 import iotests
 from iotests import qemu_img, qemu_io
-import struct
 
 backing_img = os.path.join(iotests.test_dir, 'backing.img')
 target_backing_img = os.path.join(iotests.test_dir, 'target-backing.img')
@@ -71,20 +70,11 @@ class ImageMirroringTestCase(iotests.QMPTestCase):
 
         self.assert_no_active_block_jobs()
 
-    def create_image(self, name, size):
-        file = open(name, 'w')
-        i = 0
-        while i < size:
-            sector = struct.pack('>l504xl', i / 512, i / 512)
-            file.write(sector)
-            i = i + 512
-        file.close()
-
 class TestSingleDrive(ImageMirroringTestCase):
     image_len = 1 * 1024 * 1024 # MB
 
     def setUp(self):
-        self.create_image(backing_img, TestSingleDrive.image_len)
+        iotests.create_image(backing_img, TestSingleDrive.image_len)
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, test_img)
         self.vm = iotests.VM().add_drive(test_img)
         self.vm.launch()
@@ -230,15 +220,15 @@ class TestMirrorNoBacking(ImageMirroringTestCase):
     image_len = 2 * 1024 * 1024 # MB
 
     def complete_and_wait(self, drive='drive0', wait_ready=True):
-        self.create_image(target_backing_img, TestMirrorNoBacking.image_len)
+        iotests.create_image(target_backing_img, TestMirrorNoBacking.image_len)
         return ImageMirroringTestCase.complete_and_wait(self, drive, wait_ready)
 
     def compare_images(self, img1, img2):
-        self.create_image(target_backing_img, TestMirrorNoBacking.image_len)
+        iotests.create_image(target_backing_img, TestMirrorNoBacking.image_len)
         return iotests.compare_images(img1, img2)
 
     def setUp(self):
-        self.create_image(backing_img, TestMirrorNoBacking.image_len)
+        iotests.create_image(backing_img, TestMirrorNoBacking.image_len)
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, test_img)
         self.vm = iotests.VM().add_drive(test_img)
         self.vm.launch()
@@ -306,7 +296,7 @@ class TestMirrorResized(ImageMirroringTestCase):
     image_len = 2 * 1024 * 1024 # MB
 
     def setUp(self):
-        self.create_image(backing_img, TestMirrorResized.backing_len)
+        iotests.create_image(backing_img, TestMirrorResized.backing_len)
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, test_img)
         qemu_img('resize', test_img, '2M')
         self.vm = iotests.VM().add_drive(test_img)
@@ -381,7 +371,7 @@ new_state = "1"
 
     def setUp(self):
         self.blkdebug_file = backing_img + ".blkdebug"
-        self.create_image(backing_img, TestReadErrors.image_len)
+        iotests.create_image(backing_img, TestReadErrors.image_len)
         self.create_blkdebug_file(self.blkdebug_file, "read_aio", 5)
         qemu_img('create', '-f', iotests.imgfmt,
                  '-o', 'backing_file=blkdebug:%s:%s,backing_fmt=raw'
@@ -536,7 +526,7 @@ new_state = "1"
 
     def setUp(self):
         self.blkdebug_file = target_img + ".blkdebug"
-        self.create_image(backing_img, TestWriteErrors.image_len)
+        iotests.create_image(backing_img, TestWriteErrors.image_len)
         self.create_blkdebug_file(self.blkdebug_file, "write_aio", 5)
         qemu_img('create', '-f', iotests.imgfmt, '-obacking_file=%s' %(backing_img), test_img)
         self.vm = iotests.VM().add_drive(test_img)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 733b82b..8a8f181 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -23,6 +23,7 @@ import string
 import unittest
 import sys; sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'QMP'))
 import qmp
+import struct
 
 __all__ = ['imgfmt', 'imgproto', 'test_dir' 'qemu_img', 'qemu_io',
            'VM', 'QMPTestCase', 'notrun', 'main']
@@ -56,6 +57,16 @@ def compare_images(img1, img2):
     return qemu_img('compare', '-f', imgfmt,
                     '-F', imgfmt, img1, img2) == 0
 
+def create_image(name, size):
+    '''Create a fully-allocated raw image with sector markers'''
+    file = open(name, 'w')
+    i = 0
+    while i < size:
+        sector = struct.pack('>l504xl', i / 512, i / 512)
+        file.write(sector)
+        i = i + 512
+    file.close()
+
 class VM(object):
     '''A QEMU VM'''
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 0/4] qemu-iotests: reduce iotests.py code duplication

[Fam Zheng]

On Tue, 05/28 17:11, Stefan Hajnoczi wrote:
> Tests 030 and 041 are written in Python and use the iotests.py module.  Push
> common code down into iotests.py.  My 'drive-backup' series takes advantage
> will take advantage of this - kwolf pointed out that earlier revisions were
Do you mean s/takes advantage will take advantage/will take advantage/?
> duplicating too much code.
> 
> I decided to split this into a separate patch series and send it before the
> next 'drive-backup' revision.
> 
> Stefan Hajnoczi (4):
>   qemu-iotests: make assert_no_active_block_jobs() common
>   qemu-iotests: make cancel_and_wait() common
>   qemu-iotests: make compare_images() common
>   qemu-iotests: make create_image() common
> 
>  tests/qemu-iotests/030        |  99 ++++++++---------------
>  tests/qemu-iotests/041        | 181 +++++++++++++++---------------------------
>  tests/qemu-iotests/iotests.py |  38 +++++++++
>  3 files changed, 135 insertions(+), 183 deletions(-)
> 
> -- 
> 1.8.1.4
> 
> 

-- 
Fam

Re: [Qemu-devel] [PATCH 0/4] qemu-iotests: reduce iotests.py code duplication

[Stefan Hajnoczi]

On Wed, May 29, 2013 at 01:29:34PM +0800, Fam Zheng wrote:
> On Tue, 05/28 17:11, Stefan Hajnoczi wrote:
> > Tests 030 and 041 are written in Python and use the iotests.py module.  Push
> > common code down into iotests.py.  My 'drive-backup' series takes advantage
> > will take advantage of this - kwolf pointed out that earlier revisions were
> Do you mean s/takes advantage will take advantage/will take advantage/?

Yes :)

Re: [Qemu-devel] [PATCH 2/4] qemu-iotests: make cancel_and_wait() common

[Kevin Wolf]

Am 28.05.2013 um 17:11 hat Stefan Hajnoczi geschrieben:
> The cancel_and_wait() function has been duplicated in 030 and 041.  Move
> it into iotests.py and let it return the event so tests can perform
> additional asserts.
> 
> Note that 041's cancel_and_wait(wait_ready=True) is replaced by
> wait_ready_and_cancel(), which uses the new wait_ready() and
> cancel_and_wait() underneath.
> 
> Suggested-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

> diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
> index ff89427..c4ce75e 100755
> --- a/tests/qemu-iotests/041
> +++ b/tests/qemu-iotests/041
> @@ -32,46 +32,28 @@ target_img = os.path.join(iotests.test_dir, 'target.img')
>  class ImageMirroringTestCase(iotests.QMPTestCase):
>      '''Abstract base class for image mirroring test cases'''
>  
> -    def cancel_and_wait(self, drive='drive0', wait_ready=True):
> -        '''Cancel a block job and wait for it to finish'''
> -        if wait_ready:
> -            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
> -
> -        result = self.vm.qmp('block-job-cancel', device=drive,
> -                             force=not wait_ready)
> -        self.assert_qmp(result, 'return', {})
> -
> -        cancelled = False
> -        while not cancelled:
> +    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_COMPLETED' or \
> -                   event['event'] == 'BLOCK_JOB_CANCELLED':
> +                if event['event'] == 'BLOCK_JOB_READY':
>                      self.assert_qmp(event, 'data/type', 'mirror')
>                      self.assert_qmp(event, 'data/device', drive)
> -                    if wait_ready:
> -                        self.assertEquals(event['event'], 'BLOCK_JOB_COMPLETED')
> -                        self.assert_qmp(event, 'data/offset', self.image_len)
> -                        self.assert_qmp(event, 'data/len', self.image_len)
> -                    cancelled = True
> +                    ready = True

Why don't you just move the whole function including the wait_ready
parameter? It doesn't do any harm to other callers that don't need the
feature, and will likely be useful for other test cases later.

Kevin

Re: [Qemu-devel] [PATCH 0/4] qemu-iotests: reduce iotests.py code duplication

[Kevin Wolf]

Am 28.05.2013 um 17:11 hat Stefan Hajnoczi geschrieben:
> Tests 030 and 041 are written in Python and use the iotests.py module.  Push
> common code down into iotests.py.  My 'drive-backup' series takes advantage
> will take advantage of this - kwolf pointed out that earlier revisions were
> duplicating too much code.
> 
> I decided to split this into a separate patch series and send it before the
> next 'drive-backup' revision.
> 
> Stefan Hajnoczi (4):
>   qemu-iotests: make assert_no_active_block_jobs() common
>   qemu-iotests: make cancel_and_wait() common
>   qemu-iotests: make compare_images() common
>   qemu-iotests: make create_image() common
> 
>  tests/qemu-iotests/030        |  99 ++++++++---------------
>  tests/qemu-iotests/041        | 181 +++++++++++++++---------------------------
>  tests/qemu-iotests/iotests.py |  38 +++++++++
>  3 files changed, 135 insertions(+), 183 deletions(-)

Looks correct, but I had a question/comment about patch 2.

Kevin

Re: [Qemu-devel] [PATCH 2/4] qemu-iotests: make cancel_and_wait() common

[Stefan Hajnoczi]

On Wed, May 29, 2013 at 11:54:56AM +0200, Kevin Wolf wrote:
> Am 28.05.2013 um 17:11 hat Stefan Hajnoczi geschrieben:
> > -    def cancel_and_wait(self, drive='drive0', wait_ready=True):
> > -        '''Cancel a block job and wait for it to finish'''
> > -        if wait_ready:
> > -            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
> > -
> > -        result = self.vm.qmp('block-job-cancel', device=drive,
> > -                             force=not wait_ready)
> > -        self.assert_qmp(result, 'return', {})
> > -
> > -        cancelled = False
> > -        while not cancelled:
> > +    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_COMPLETED' or \
> > -                   event['event'] == 'BLOCK_JOB_CANCELLED':
> > +                if event['event'] == 'BLOCK_JOB_READY':
> >                      self.assert_qmp(event, 'data/type', 'mirror')
> >                      self.assert_qmp(event, 'data/device', drive)
> > -                    if wait_ready:
> > -                        self.assertEquals(event['event'], 'BLOCK_JOB_COMPLETED')
> > -                        self.assert_qmp(event, 'data/offset', self.image_len)
> > -                        self.assert_qmp(event, 'data/len', self.image_len)
> > -                    cancelled = True
> > +                    ready = True
> 
> Why don't you just move the whole function including the wait_ready
> parameter? It doesn't do any harm to other callers that don't need the
> feature, and will likely be useful for other test cases later.

The BLOCK_JOB_READY and block-job-complete concepts are used by
mirroring only.  Plus the mirroring test cases perform additional checks
on the event object which aren't common.

I figured the cleanest solution is the patch I posted.

Stefan

Re: [Qemu-devel] [PATCH 0/4] qemu-iotests: reduce iotests.py code duplication

[Kevin Wolf]

Am 28.05.2013 um 17:11 hat Stefan Hajnoczi geschrieben:
> Tests 030 and 041 are written in Python and use the iotests.py module.  Push
> common code down into iotests.py.  My 'drive-backup' series takes advantage
> will take advantage of this - kwolf pointed out that earlier revisions were
> duplicating too much code.
> 
> I decided to split this into a separate patch series and send it before the
> next 'drive-backup' revision.
> 
> Stefan Hajnoczi (4):
>   qemu-iotests: make assert_no_active_block_jobs() common
>   qemu-iotests: make cancel_and_wait() common
>   qemu-iotests: make compare_images() common
>   qemu-iotests: make create_image() common
> 
>  tests/qemu-iotests/030        |  99 ++++++++---------------
>  tests/qemu-iotests/041        | 181 +++++++++++++++---------------------------
>  tests/qemu-iotests/iotests.py |  38 +++++++++
>  3 files changed, 135 insertions(+), 183 deletions(-)

Thanks, applied to the block branch.

Kevin