Francesco Romani has posted comments on this change. Change subject: VDSM: implement nodeDeviceMapper ......................................................................
Patch Set 11: (6 comments) a few more comments http://gerrit.ovirt.org/#/c/30209/11/vdsm/hostdev.py File vdsm/hostdev.py: Line 138: # add previously unknown devices Line 139: for (deviceName, device) in hostDevices: Line 140: if deviceName not in self._map: Line 141: self._log.info('Found new host device: %s', deviceName) Line 142: self._map[deviceName] = [HostDevice(device), ''] > Is this list always composed by two elements? I'd rather use None for 'no Vm attached' Line 143: Line 144: # remove devices that are no longer found on host and remove no longer Line 145: # existing hosts from devices Line 146: for deviceName, attachment in self._map.items(): Line 142: self._map[deviceName] = [HostDevice(device), ''] Line 143: Line 144: # remove devices that are no longer found on host and remove no longer Line 145: # existing hosts from devices Line 146: for deviceName, attachment in self._map.items(): 'attachment' is probably not the most appropriate term here... what about 'ownerVm'? 'owningVm'? or something like this. Line 147: if attachment[1] and attachment[1] not in self._vmContainer: Line 148: self._log.info('Removing host %s from host device: %s', Line 149: attachment[1], deviceName) Line 150: self.release(deviceName) Line 157: try: Line 158: for device in params.conf['devices']: Line 159: if device['device'] == 'hostdev': Line 160: try: Line 161: self._map[device['name']][1] = vm just 'vm' is misleading here, use vmId (if is that what you mean, see also comment at line 182) Line 162: except KeyError: Line 163: # VM has device which is no (longer) present Line 164: # on the host - althought this should be handled by Line 165: # user, warning can help debugging libvirt errors Line 175: Method to account ownership of device to specified vmId and detach Line 176: it from the host if it isn't already acquired Line 177: """ Line 178: self._log.info('Attemting to detach passthrough device %s ' Line 179: 'from %s', deviceName, vmId) This deseves better explanation, it is surprising to see device detaching in a method called 'acquire' Line 180: try: Line 181: if not self._map[deviceName][1]: Line 182: self._map[deviceName][1] = vmId Line 183: # dettach needs to be called for pci, usb devices Line 178: self._log.info('Attemting to detach passthrough device %s ' Line 179: 'from %s', deviceName, vmId) Line 180: try: Line 181: if not self._map[deviceName][1]: Line 182: self._map[deviceName][1] = vmId At line 161 you are assigning a vm, here a vmId, this is suspicious, at least one is either wrong or has a very misleading name Line 183: # dettach needs to be called for pci, usb devices Line 184: if self._map[deviceName][0].capability in ('pci', 'usb_device'): Line 185: self._map[deviceName][0].node.dettach() Line 186: if self._map[deviceName][0].capability == 'pci': Line 205: self._log.info('Attemting to reattach passthrough device %s ' Line 206: 'from %s', deviceName) Line 207: try: Line 208: if self._map[deviceName][1] not in self._vmContainer: Line 209: self._map[deviceName][1] = '' please sse second comment at line 142 Line 210: self._map[deviceName][0].node.reAttach() Line 211: Line 212: return Line 213: else: -- To view, visit http://gerrit.ovirt.org/30209 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6f21d465a90cfe2eb16ba70943e5fcf1683f1656 Gerrit-PatchSet: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Martin Polednik <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Martin Betak <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
