Change in vdsm[master]: vdsm: initial split of vm devices
fromani at redhat.com
fromani at redhat.com
Fri Aug 29 11:44:53 UTC 2014
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 <mpolednik at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list