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