Re: [libvirt] [PATCH for-4.0 v3 2/2] virtio: Provide version-specific variants of virtio PCI devices
On Tue, Nov 27, 2018 at 10:56:21AM -0200, Caio Carrara wrote:
Hello, Eduardo.
Just some minor comments regarding the Python code.
[...]
> +def devtype_implements(vm, devtype, implements):
> + return devtype in [d['name'] for d in
vm.command('qom-list-types', implements=implements)]
> +
> +def get_interfaces(vm, devtype, interfaces=['pci-express-device',
'conventional-pci-device']):
Usually is not a good idea to use a mutable (in this case a list)
default value as an argument:
def my_func(param=[1,2,3]):
param.append(666)
print(param)
my_func()
[1, 2, 3, 666]
my_func()
[1, 2, 3, 666, 666]
my_func()
[1, 2, 3, 666, 666, 666]
Yes, you're not changing the value right now, but it's something we can
not guarantee for the future when, for example, someone not Python
experienced have to change this code.
Oops, thanks for catching this!
Another way safer to write this function could be:
def get_interfaces(vm, devtype, interfaces=None):
if not interfaces:
interfaces = ['pci-express-device', 'conventional-pci-device']
return [i for i in interfaces if devtype_implements(vm, devtype, i)]
Maybe I should just use a tuple, then?
def get_interfaces(vm, devtype, interfaces=('pci-express-device',
'conventional-pci-device')):
return [i for i in interfaces if devtype_implements(vm, devtype, i)]
but probably I can simply eliminate the parameter because nobody
is using it:
def get_pci_interfaces(vm, devtype):
interfaces = ('pci-express-device', 'conventional-pci-device')
return [i for i in interfaces if devtype_implements(vm, devtype, i)]
> + return [i for i in interfaces if devtype_implements(vm, devtype, i)]
> +
> +class VirtioVersionCheck(Test):
> + """
> + Check if virtio-version-specific device types result in the
> + same device tree created by `disable-modern` and
> + `disable-legacy`.
> +
> + :avocado: enable
> + :avocado: tags=x86_64
> + """
> +
> + # just in case there are failures, show larger diff:
> + maxDiff = 4096
> +
> + def run_device(self, devtype, opts=None, machine='pc'):
> + """
> + Run QEMU with `-device DEVTYPE`, return device info from `query-pci`
> + """
> + with QEMUMachine(self.qemu_bin) as vm:
> + vm.set_machine(machine)
> + if opts:
> + devtype += ',' + opts
> + vm.add_args('-device', '%s,id=devfortest' % (devtype))
> + vm.add_args('-S')
> + vm.launch()
> +
> + pcibuses = vm.command('query-pci')
> + alldevs = [dev for bus in pcibuses for dev in bus['devices']]
> + import pprint
> + pprint.pprint(alldevs)
I think it's an undesired print here, right?
Oops! Yes, debugging leftover.
> + devfortest = [dev for dev in alldevs
> + if dev['qdev_id'] == 'devfortest']
> + return devfortest[0], get_interfaces(vm, devtype)
> +
> +
> + def assert_devids(self, dev, devid, non_transitional=False):
> + self.assertEqual(dev['id']['vendor'],
PCI_VENDOR_ID_REDHAT_QUMRANET)
> + self.assertEqual(dev['id']['device'], devid)
> + if non_transitional:
> + self.assertTrue(0x1040 <= dev['id']['device'] <=
0x107f)
> + self.assertGreaterEqual(dev['id']['subsystem'], 0x40)
> +
> + def check_all_variants(self, qemu_devtype, virtio_devid):
> + """Check if a virtio device type and its variants behave as
expected"""
> + # Force modern mode:
> + dev_modern,_ = self.run_device(qemu_devtype,
s/dev_modern,_/dev_modern, _/ (blank space after comma. There are other
cases bellow)
I will change it, thanks!
> +
'disable-modern=off,disable-legacy=on')
> + self.assert_devids(dev_modern, pci_modern_device_id(virtio_devid),
> + non_transitional=True)
[...]
--
Eduardo