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: