Re: [Qemu-devel] [PATCH v2 37/37] iotests: Add test for change-related QMP commands

[Max Reitz <mreitz@redhat.com>]

On 2015-02-09 at 19:06, Eric Blake wrote:
> On 02/09/2015 10:11 AM, Max Reitz wrote:
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   tests/qemu-iotests/118     | 653 +++++++++++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/118.out |   5 +
>>   tests/qemu-iotests/group   |   1 +
>>   3 files changed, 659 insertions(+)
>>   create mode 100755 tests/qemu-iotests/118
>>   create mode 100644 tests/qemu-iotests/118.out
>>
>> +
>> +    def process_events(self):
>> +        for event in self.vm.get_qmp_events(wait=True):
>> +            if event['event'] == 'DEVICE_TRAY_MOVED' and event['data']['device'] == 'drive0':
> Long line; I think you can wrap it as:
>
> if (event['event'] == 'DEVICE_TRAY_MOVED' and
>      event['data']['device'] == 'drive0'):

If that works, I'll be glad to (if I have to respin, which I don't deem 
too unlikely).

>> +class GeneralChangeTestsBaseClass(ChangeBaseClass):
>> +    def test_change(self):
>> +        result = self.vm.qmp('change', device='drive0', target=new_img,
>> +                                       arg=iotests.imgfmt)
> Unusual indentation. [1]
>
>> +        self.assert_qmp(result, 'return', {})
>> +
>> +        while not self.has_opened:
>> +            self.process_events()
>> +        while not self.has_closed:
>> +            self.process_events()
>> +
> Are we guaranteed that loops like this will gracefully timeout and fail
> the test if the event doesn't happen, instead of hanging forever?

Well, let's say it should be guaranteed, but this is a test so it may 
break. I will try adding a timeout if I respin, though.

> But
> that's probably something affecting multiple tests, so I won't hold up
> review of this one.
>
>> +        result = self.vm.qmp('query-block')
>> +        self.assert_qmp(result, 'return[0]/tray_open', False)
>> +        self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
>> +
>> +    def test_blockdev_change_medium(self):
>> +        result = self.vm.qmp('blockdev-change-medium', device='drive0',
>> +                                                       filename=new_img,
>> +                                                       format=iotests.imgfmt)
> [1] I guess your choice of not flushing arguments to the ( is intentional.
>
> The long line thing is minor, so:
> Reviewed-by: Eric Blake <eblake@redhat.com>

Once again, thank you!

Max