Francesco Romani has posted comments on this change. Change subject: vdsm: initial split of vm devices ......................................................................
Patch Set 4: (7 comments) The patch is nice, the benefits are clear, but this patch is just too big. We need to split this in smaller patches. The best way is to make this a serie and move one device at time. http://gerrit.ovirt.org/#/c/29250/4/debian/vdsm.install File debian/vdsm.install: Line 148: ./usr/share/vdsm/virt/migration.py Line 149: ./usr/share/vdsm/virt/sampling.py Line 150: ./usr/share/vdsm/virt/vm.py Line 151: ./usr/share/vdsm/virt/vmchannels.py Line 152: ./usr/share/vdsm/virt/vmdevices.py I'm not really fond of the 'vm' prefix. I see a few good reason tho use it: - name clashing/too generic name (vmxml) - legacy - really specific vm code (vmpowerdown, vmexitreason, ...) In the case of this module, I believe 'devices' is good enough, being specific enough and clear within the context. Line 153: ./usr/share/vdsm/virt/vmexitreason.py Line 154: ./usr/share/vdsm/virt/vmpowerdown.py Line 155: ./usr/share/vdsm/virt/vmstatus.py Line 156: ./usr/share/vdsm/virt/vmxml.py http://gerrit.ovirt.org/#/c/29250/4/tests/vmTests.py File tests/vmTests.py: Line 331: def testSmartcardXML(self): Line 332: smartcardXML = '<smartcard mode="passthrough" type="spicevmc"/>' Line 333: dev = {'device': 'smartcard', Line 334: 'specParams': {'mode': 'passthrough', 'type': 'spicevmc'}} Line 335: smartcard = vmdevices.SmartCardDevice(self.conf, self.log, **dev) Now we can drop the 'Device' suffix: devices.SmartCard looks way nicer. Same for all the other device types. Line 336: self.assertXML(smartcard.getXML(), smartcardXML) Line 337: Line 338: def testTpmXML(self): Line 339: tpmXML = """ http://gerrit.ovirt.org/#/c/29250/4/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 361: https://bugzilla.redhat.com/show_bug.cgi?id=1114492 Line 362: """ Line 363: ioTuneInfo = [] Line 364: Line 365: for disk in self._vm._devices[vmdevices.DISK_DEVICES]: We need a better replacemente here. Probably just DISKS is good enough: Or, even better, switch to vm.getDiskDevices() In a separate patch, of course :) Line 366: if "ioTune" in disk.specParams: Line 367: ioTuneInfo.append({ Line 368: "name": disk.name, Line 369: "path": disk.path, Line 666: raise Line 667: return f Line 668: Line 669: Line 670: class NetworkInterfaceDevice(vmdevices.VmDevice): Uhm, vmdevices.VmDevice looks... redundant. :) Probably something like devices.Base is better (not enterely convinced, though. Feel free to pick better names). Line 671: __slots__ = ('nicModel', 'macAddr', 'network', 'bootOrder', 'address', Line 672: 'linkActive', 'portMirroring', 'filter', Line 673: 'sndbufParam', 'driver', 'name') Line 674: Line 680: if attr == 'nicModel' and value == 'pv': Line 681: kwargs[attr] = 'virtio' Line 682: elif attr == 'network' and value == '': Line 683: kwargs[attr] = DUMMY_BRIDGE Line 684: vmdevices.VmDevice.__init__(self, conf, log, **kwargs) time to switch to super(). Either in the same patch or -better- in another one. Here and in all the other places Line 685: self.sndbufParam = False Line 686: self._customize() Line 687: Line 688: def _customize(self): Line 1229: (vmdevices.CONSOLE_DEVICES, vmdevices.ConsoleDevice), Line 1230: (vmdevices.REDIR_DEVICES, vmdevices.RedirDevice), Line 1231: (vmdevices.RNG_DEVICES, vmdevices.RngDevice), Line 1232: (vmdevices.SMARTCARD_DEVICES, vmdevices.SmartCardDevice), Line 1233: (vmdevices.TPM_DEVICES, vmdevices.TpmDevice)) This could be moved into the vmdevices module as well - maybe in a later patch. Line 1234: Line 1235: def _makeDeviceDict(self): Line 1236: return dict((dev, []) for dev, _ in self.DeviceMapping) Line 1237: Line 2477: domxml.appendEmulator() Line 2478: Line 2479: self._appendDevices(domxml) Line 2480: Line 2481: for graphDev in self._devices[vmdevices.GRAPHICS_DEVICES]: we must use helpers here. A rebase will help, and let's add the missing ones - in separate patches. Line 2482: if graphDev.device == 'spice': Line 2483: domxml._devices.appendChild(graphDev.getSpiceVmcChannelsXML()) Line 2484: break Line 2485: -- To view, visit http://gerrit.ovirt.org/29250 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I63b602851c17697259e86dfad3a9063447c57e50 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Martin Polednik <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Martin Polednik <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
