Martin Polednik has posted comments on this change. Change subject: VDSM: implement nodeDeviceMapper ......................................................................
Patch Set 11: (7 comments) http://gerrit.ovirt.org/#/c/30209/11/vdsm/hostdev.py File vdsm/hostdev.py: Line 78: try: Line 79: vendorXML = caps.getElementsByTagName('vendor')[0] Line 80: self.vendor_id = vendorXML.getAttribute('id') Line 81: self.vendor = vendorXML.childNodes[0].data Line 82: productXML = caps.getElementsByTagName('product')[0] > would'nt be better to have two try/except blocks, one for vendor and one fo I'm all up for splitting into two blocks, the attribute is problematic as whole element can be missing, but I'll look into how this behaves and try to do it so we don't lose any data from libvirt. Line 83: self.product_id = productXML.getAttribute('id') Line 84: self.product = productXML.childNodes[0].data Line 85: except: Line 86: # althought the retrieval of product/vendor was not successful, Line 91: # get IOMMU group from sys Line 92: try: Line 93: return os.readlink('/sys/bus/pci/devices/%s/iommu_group' % Line 94: self._getSysAddress( Line 95: self.node.name())).split('/')[-1] > maybe os.path.basename could help here: yep, seems better Line 96: except OSError: Line 97: return '' Line 98: Line 99: def _getSysAddress(self, deviceName): Line 116: Line 117: Line 118: class HostDeviceMapper(object): Line 119: Line 120: _instance = None > what is this used for? as a reminder that it doesn't belong here ;) Line 121: Line 122: def __init__(self, vmContainer, log): Line 123: self._map = {} Line 124: self._log = log 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), ''] > I'd rather use None for 'no Vm attached' Using None would require additional layer in API to return empty field, are you sure? And I'm not sure that I understand your idea of using namedtuple, dict and list are used as the whole structure needs to be mutable (devices are being added and removed as they're detected, vmId chaning...) 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 ' Attachment is used as the format is deviceName -> [device, vmId] - it maps the device to vmId - I'd say attachment is appropriate term. 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 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 i This is because I consider this mapper the end of host device stack: from host's perspective the device is detached, but for us VM takes it's ownership. 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 lea indeed misleading, will fix 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': -- 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: Martin Polednik <[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
