Re: [Qemu-devel] [PATCH] qemu-iotests: fix 030 for faster machines

[Fam Zheng <famz@redhat.com>]

On Wed, 10/16 20:45, Max Reitz wrote:
> On 2013-10-15 04:41, Fam Zheng wrote:
> >If the block job completes too fast, the test can fail. Change the
> >numbers so the qmp events are more stably captured by the script.
> >
> >A sleep is removed for the same reason.
> >
> >Signed-off-by: Fam Zheng <famz@redhat.com>
> >---
> >  tests/qemu-iotests/030 | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> >
> >diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
> >index ae56f3b..188b182 100755
> >--- a/tests/qemu-iotests/030
> >+++ b/tests/qemu-iotests/030
> >@@ -403,14 +403,13 @@ class TestStreamStop(iotests.QMPTestCase):
> >          result = self.vm.qmp('block-stream', device='drive0')
> >          self.assert_qmp(result, 'return', {})
> >-        time.sleep(0.1)
> 
> Hm, I'm not sure whether removing the sleep actually removes the
> underlying race condition… It should work in most cases and the
> foreseeable future, though.
> 
> >          events = self.vm.get_qmp_events(wait=False)
> >          self.assertEqual(events, [], 'unexpected QMP event: %s' % events)
> >          self.cancel_and_wait()
> >  class TestSetSpeed(iotests.QMPTestCase):
> >-    image_len = 80 * 1024 * 1024 # MB
> >+    image_len = 512 * 1024 * 1024 # MB
> >      def setUp(self):
> >          qemu_img('create', backing_img, str(TestSetSpeed.image_len))
> >@@ -457,23 +456,23 @@ class TestSetSpeed(iotests.QMPTestCase):
> >          self.assert_qmp(result, 'return[0]/device', 'drive0')
> >          self.assert_qmp(result, 'return[0]/speed', 0)
> >-        result = self.vm.qmp('block-job-set-speed', device='drive0', speed=8 * 1024 * 1024)
> >+        result = self.vm.qmp('block-job-set-speed', device='drive0', speed=8 * 1024)
> 
> So the limit was already 8 MB/s? Doesn't this mean that the job
> should have taken 10 seconds anyway? Sounds to me like the block job
> speed is basically disregarded anyway.

No, see below...

> 
> If I re-add the sleep you removed in this patch, this test fails
> again for me. This further suggests block-job-set-speed to be kind
> of a no-op and the changes concerning the image length and the block
> job speed not really contributing to fixing the test.
> 
> So I think removing the sleep is all that would have to be done
> right now. OTOH, this is not really a permanent fix, either (the
> fundamental race condition remains). Furthermore, I guess there is
> some reason for having a sleep there - else it would not exist in
> the first place (and it apparently already caused problems some time
> ago which were "fixed" by replacing the previous "sleep(1)" by
> "sleep(0.1)").
> 
> All in all, if someone can assure me of the uneccessity of the sleep
> in question, I think removing it is all that's needed.
> 
> Max
> 

Both failure cases are just that setting speed or checking status comes too
late: the streaming finishes or goes close to finish in negligible no time once
the job is started. In other words dropping the speed change but only increase
image_len and remove sleep will fix it for me too.

Fam