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(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Martin Polednik <mpoledni(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes