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 <mpole...@redhat.com> Gerrit-Reviewer: Antoni Segura Puimedon <asegu...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Douglas Schilling Landgraf <dougsl...@redhat.com> Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Martin Polednik <mpole...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches