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

[Max Reitz]

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 <address@hidden>
> > ---
> >   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 <address@hidden>

Once again, thank you!

Max