Change in vdsm[master]: vdsm: add support for hostdev passthrough

fromani at redhat.com fromani at redhat.com
Tue Jun 17 09:06:51 UTC 2014


Francesco Romani has posted comments on this change.

Change subject: vdsm: add support for hostdev passthrough
......................................................................


Patch Set 15: Code-Review-1

(5 comments)

Looking good, just a few final touches and it is OK for me. -1 only for visibility.

http://gerrit.ovirt.org/#/c/22462/15/vdsm/caps.py
File vdsm/caps.py:

Line 478: 
Line 479: 
Line 480: def _getHostdev():
Line 481:     devices = []
Line 482:     supportedDevices = ('pci',)
this seems to be used only in the for loop at line 512; if that's the case, please remove this variable
Line 483: 
Line 484:     for device in libvirtconnection.get().listAllDevices(0):
Line 485:         devXML = minidom.parseString(device.XMLDesc(0))
Line 486:         dev = {}


Line 505:                 # we can still report back the name
Line 506:                 pass
Line 507:         except IndexError:
Line 508:             # should device not have a name, there is nothing engine could send
Line 509:             # back that we could use to uniquely identify and initiate a device
Then please consider to add a warning here.
Line 510:             continue
Line 511: 
Line 512:         if capability in supportedDevices:
Line 513:             devices.append(dev)


http://gerrit.ovirt.org/#/c/22462/15/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 1486: 
Line 1487:     def __init__(self, *args, **kwargs):
Line 1488:         super(HostDevice, self).__init__(*args, **kwargs)
Line 1489: 
Line 1490:         self._nodeptr = self._connection.nodeDeviceLookupByName(self.name)
silly nit: why nodeptr? I'd just use self._node
Line 1491: 
Line 1492:         # the device needs to be detached from the host
Line 1493:         self.log.debug('Detaching hostdev %s', self.name)
Line 1494:         self._nodeptr.dettach()


Line 1488:         super(HostDevice, self).__init__(*args, **kwargs)
Line 1489: 
Line 1490:         self._nodeptr = self._connection.nodeDeviceLookupByName(self.name)
Line 1491: 
Line 1492:         # the device needs to be detached from the host
Please add another line in this comment explaining why the device needs to be detached (just a  link to e.g. libvirt docs is fine)
Line 1493:         self.log.debug('Detaching hostdev %s', self.name)
Line 1494:         self._nodeptr.dettach()
Line 1495: 
Line 1496:     def getPciAddr(self):


Line 4569: 
Line 4570:         # we have to manually reattach passthrough devices
Line 4571:         for dev in self._devices[HOSTDEV_DEVICES]:
Line 4572:             self.log.debug('Retaching device %s', dev.name)
Line 4573:             dev._nodeptr.reAttach()
We have the same snippet twice, so we reached the threshold for moving this code into an helper method, with possibly the comment converting in a docstring and with and added explanation/link/remainder of why we need this deattach/reattach dance.
Line 4574: 
Line 4575:         hooks.before_vm_destroy(self._lastXMLDesc, self.conf)
Line 4576:         self.destroyed = True
Line 4577: 


-- 
To view, visit http://gerrit.ovirt.org/22462
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I363d2622d72ca2db75f60032fe0892c348bab121
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpoledni at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Martin Polednik <mpoledni at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr 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