Re: [Qemu-devel] [PATCH 10/10] qemu-iotests/118: Test media change with qdev name

From: Kevin Wolf <kwolf@redhat.com>
To: Eric Blake <eblake@redhat.com>
Cc: qemu-block@nongnu.org, mreitz@redhat.com, qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH 10/10] qemu-iotests/118: Test media change with qdev name
Date: Thu, 15 Sep 2016 10:47:03 +0200	[thread overview]
Message-ID: <20160915084703.GC4726@noname.redhat.com> (raw)
In-Reply-To: <32842c09-6dce-b68b-d95e-de3813099faa@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 3065 bytes --]

Am 15.09.2016 um 00:13 hat Eric Blake geschrieben:
> On 08/19/2016 11:50 AM, Kevin Wolf wrote:
> > We just added the option to use qdev device names in all device related
> > block QMP commands. This patch converts some of the test cases in 118 to
> > use qdev device names instead of BlockBackend names to cover the new
> > way. It converts cases for each of the media change commands, but only
> > for CD-ROM and not everywhere, so that the old way is still tested, too.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  tests/qemu-iotests/118        | 85 ++++++++++++++++++++++++++++++++++---------
> >  tests/qemu-iotests/iotests.py |  5 +++
> >  2 files changed, 73 insertions(+), 17 deletions(-)
> > 
> 
> > @@ -76,9 +79,15 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
> >          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)
> > +        if self.device_name is not None:
> > +            result = self.vm.qmp('blockdev-change-medium',
> > +                                 id=self.device_name, filename=new_img,
> > +                                 format=iotests.imgfmt)
> > +        else:
> > +            result = self.vm.qmp('blockdev-change-medium',
> > +                                 device='drive0', filename=new_img,
> > +                                 format=iotests.imgfmt)
> 
> I'm not enough of a python guru to know if there is any way to compress
> this to a shorter format (I do know, however, that the lack of an
> obvious ?: operator in python can indeed result in verbose if/else
> clauses compared to other languages).

Not a Python guru either, but it does have an equivalent for the ?:
operator, just with a weird syntax:

    <value1> if <condition> else <value2>

However, I'm not sure if that would be applicable here. It depends on
whether not passing an option at all and passing None does the same
thing, or if None sends something like an empty object or JSON null.

> At any rate, the ultimate test is whether the change still passes; and
> looks like you have good coverage of using exactly one or the other
> argument.  Do you also want to add tests (either here, or in 11/10) that
> validate that providing neither 'device' nor 'id' gives a sane error,
> likewise that providing both has sane behavior?  (For now, our behavior
> is that we fail, although it could also be argued that sane behavior
> would validate that 'id' happens to be currently in use by 'device' and
> only fail if they are not pointing to the same backend).

I think failure is right. If you're using the new interface, you should
be using the new interface only.

Anyway, adding tests for this probably makes sense, I'll have a look.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]