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

[Max Reitz <mreitz@redhat.com>]

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.

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

>           self.assert_qmp(result, 'return', {})
>   
>           # Ensure the speed we set was accepted
>           result = self.vm.qmp('query-block-jobs')
>           self.assert_qmp(result, 'return[0]/device', 'drive0')
> -        self.assert_qmp(result, 'return[0]/speed', 8 * 1024 * 1024)
> +        self.assert_qmp(result, 'return[0]/speed', 8 * 1024)
>   
>           self.cancel_and_wait()
>   
>           # Check setting speed in block-stream works
> -        result = self.vm.qmp('block-stream', device='drive0', speed=4 * 1024 * 1024)
> +        result = self.vm.qmp('block-stream', device='drive0', speed=4 * 1024)
>           self.assert_qmp(result, 'return', {})
>   
>           result = self.vm.qmp('query-block-jobs')
>           self.assert_qmp(result, 'return[0]/device', 'drive0')
> -        self.assert_qmp(result, 'return[0]/speed', 4 * 1024 * 1024)
> +        self.assert_qmp(result, 'return[0]/speed', 4 * 1024)
>   
>           self.cancel_and_wait()
>