Re: [Qemu-devel] [PATCH for-4.0 v3 2/2] virtio: Provide version-specific variants of virtio PCI devices

[Caio Carrara <ccarrara@redhat.com>]

On Tue, Nov 27, 2018 at 11:06:17AM -0200, Eduardo Habkost wrote:
> 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?

Yes. It should work as well.

> 
>     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

-- 
Caio Carrara
Software Engineer, Virt Team - Red Hat
ccarrara@redhat.com