Re: [Qemu-devel] [PATCH 10/10] qemu-iotests/118: Test media change with qdev name
Dear Eric,
Eric Blake writes:
> arglist = (all other parameters)
> if self.device_name:
> arglist.append(id=self.device_name)
> else:
> arglist.append(device='drive0')
> invoke(self.vm.qmp, arglist)
That would be:
qmp_args = {'cmd': 'blockdev-change-medium', 'filename': new_img,
'format': iotests.imgfmt}
if self.device_name is not None:
qmp_args['id'] = self.device_name
else:
qmp_args['device'] = 'drive0'
result = self.vm.qmp(**qmp_args)
Or:
qmp_args = {'filename': new_img, 'format': iotests.imgfmt}
if self.device_name is not None:
qmp_args['id'] = self.device_name
else:
qmp_args['device'] = 'drive0'
result = self.vm.qmp('blockdev-change-medium', **qmp_args)
FWIW these days I tend to use Python magics (like *args / **kwargs) only
if there's a _significant_ improvement in either functionality or
readability; especially for code bases that are being worked on by
developers who don't speak Python "natively". Using magics sparingly
also makes it easier for static analysis tools like pylint.
Sascha
--
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH 10/10] qemu-iotests/118: Test media change with qdev name
On 09/15/2016 11:28 AM, Sascha Silbe wrote:
> Dear Eric,
>
> Sascha Silbe writes:
>
>> result = self.vm.qmp('blockdev-change-medium',
>> id=self.device_name or 'drive0',
>> filename=new_img,
>> format=iotests.imgfmt)
>
> Sorry, my eyes deceived me. I thought the original code was setting the
> "id" parameter in both cases. QEMUMachine.qmp() and
> QEMUMonitorProtocol.cmd() together translate the keyword arguments
> directly into QMP arguments, so passing None is different from not
> passing anything at all. The if/else is probably the best approach in
> that case; everything else I can think of would just be more
> complicated.
If there were some way to do:
arglist = (all other parameters)
if self.device_name:
arglist.append(id=self.device_name)
else:
arglist.append(device='drive0')
invoke(self.vm.qmp, arglist)
that might be a little cleaner (no repetition on all the other
parameters); but I don't know enough python to know if there is some
sort of invoke() mechanism for pairing a method object and an arbitrary
list as though it were a normal method call (I suspect there is, and I
just don't know it).
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 10/10] qemu-iotests/118: Test media change with qdev name
Dear Eric,
Sascha Silbe writes:
> result = self.vm.qmp('blockdev-change-medium',
> id=self.device_name or 'drive0',
> filename=new_img,
> format=iotests.imgfmt)
Sorry, my eyes deceived me. I thought the original code was setting the
"id" parameter in both cases. QEMUMachine.qmp() and
QEMUMonitorProtocol.cmd() together translate the keyword arguments
directly into QMP arguments, so passing None is different from not
passing anything at all. The if/else is probably the best approach in
that case; everything else I can think of would just be more
complicated.
Sascha
--
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH 10/10] qemu-iotests/118: Test media change with qdev name
On 09/15/2016 11:16 AM, Sascha Silbe wrote:
>>> +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).
>
> Actually there is a direct equivalent of "?:" (which unfortunately isn't
> in C11) in Python: "or". So you can use:
>
> result = self.vm.qmp('blockdev-change-medium',
> id=self.device_name or 'drive0',
> filename=new_img,
Not what we want. We want to use a different parameter, based on the
overall condition (so it is NOT as simple as a conditional assignment to
the 'id' parameter, but an actual swap between whether we have the 'id'
or the 'device' parameter). It's not obvious whether:
result = foo(id=self.device_name,
device='drive0' if not self.device_name else None)
would have the same semantics as:
if self.device_name:
result = foo(id=self.device_name)
else
result = foo(device='drive0')
and even if it does have the same semantics, it's not obvious if there's
a shorter way to write it. So we end up with verbose duplication of all
the remaining parameters.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 10/10] qemu-iotests/118: Test media change with qdev name
Dear Eric,
(replying only to the Python coding part, haven't looked at the patch
itself)
Eric Blake writes:
> On 08/19/2016 11:50 AM, Kevin Wolf wrote:
>> @@ -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).
Actually there is a direct equivalent of "?:" (which unfortunately isn't
in C11) in Python: "or". So you can use:
result = self.vm.qmp('blockdev-change-medium',
id=self.device_name or 'drive0', filename=new_img,
format=iotests.imgfmt)
Sascha
--
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH 10/10] qemu-iotests/118: Test media change with qdev name
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
> > ---
> > 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:
if else
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
pgpITYGaXaWoN.pgp
Description: PGP signature
Re: [Qemu-devel] [PATCH 10/10] qemu-iotests/118: Test media change with qdev name
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
> ---
> 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).
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).
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [PATCH 10/10] qemu-iotests/118: Test media change with qdev name
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
---
tests/qemu-iotests/118| 85 ++-
tests/qemu-iotests/iotests.py | 5 +++
2 files changed, 73 insertions(+), 17 deletions(-)
diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118
index 9e5951f..0380069 100755
--- a/tests/qemu-iotests/118
+++ b/tests/qemu-iotests/118
@@ -62,6 +62,9 @@ class ChangeBaseClass(iotests.QMPTestCase):
self.fail('Timeout while waiting for the tray to close')
class GeneralChangeTestsBaseClass(ChangeBaseClass):
+
+device_name = None
+
def test_change(self):
result = self.vm.qmp('change', device='drive0', target=new_img,
arg=iotests.imgfmt)
@@ -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)
+
self.assert_qmp(result, 'return', {})
self.wait_for_open()
@@ -90,7 +99,10 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
def test_eject(self):
-result = self.vm.qmp('eject', device='drive0', force=True)
+if self.device_name is not None:
+result = self.vm.qmp('eject', id=self.device_name, force=True)
+else:
+result = self.vm.qmp('eject', device='drive0', force=True)
self.assert_qmp(result, 'return', {})
self.wait_for_open()
@@ -101,7 +113,10 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
self.assert_qmp_absent(result, 'return[0]/inserted')
def test_tray_eject_change(self):
-result = self.vm.qmp('eject', device='drive0', force=True)
+if self.device_name is not None:
+result = self.vm.qmp('eject', id=self.device_name, force=True)
+else:
+result = self.vm.qmp('eject', device='drive0', force=True)
self.assert_qmp(result, 'return', {})
self.wait_for_open()
@@ -111,9 +126,12 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
self.assert_qmp(result, 'return[0]/tray_open', True)
self.assert_qmp_absent(result, 'return[0]/inserted')
-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)
self.assert_qmp(result, 'return', {})
self.wait_for_close()
@@ -124,7 +142,12 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
def test_tray_open_close(self):
-result = self.vm.qmp('blockdev-open-tray', device='drive0', force=True)
+if self.device_name is not None:
+result = self.vm.qmp('blockdev-open-tray',
+ id=self.device_name, force=True)
+else:
+result = self.vm.qmp('blockdev-open-tray',
+ device='drive0', force=True)
self.assert_qmp(result, 'return', {})
self.wait_for_open()
@@ -137,7 +160,10 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
else:
self.assert_qmp(result, 'return[0]/inserted/image/filename',
old_img)
-result = self.vm.qmp('blockdev-close-tray', device='drive0')
+if self.device_name is not None:
+result = self.vm.qmp('blockdev-close-tray', id=self.device_name)
+else:
+
