Francesco Romani has posted comments on this change. Change subject: VDSM: implement nodeDeviceMapper ......................................................................
Patch Set 11: (9 comments) Initial review, let's start gently with the easy parts. Please add some unit tests for the new hostdev code. http://gerrit.ovirt.org/#/c/30209/11/vdsm/hostdev.py File vdsm/hostdev.py: Line 1: # this is a new module with new code from scratch, so we should apply https://www.ovirt.org/Vdsm_Coding_Guidelines in particular use names_with_underscores Line 2: # Copyright 2008-2014 Red Hat, Inc. Line 3: # Line 4: # This program is free software; you can redistribute it and/or modify Line 5: # it under the terms of the GNU General Public License as published by Line 25: Line 26: import supervdsm Line 27: Line 28: Line 29: class DeviceAlreadyAcquired(Exception): We can probably drop the 'Device' prefix Line 30: pass Line 31: Line 32: Line 33: class DeviceNotPresent(Exception): Line 29: class DeviceAlreadyAcquired(Exception): Line 30: pass Line 31: Line 32: Line 33: class DeviceNotPresent(Exception): Ditto Line 34: pass Line 35: Line 36: Line 37: class HostDevice(object): 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 for product? If one of those two fields can be missing, and we know it, probably we can just use if xml.hasAttribute(...): do_stuff(xml) 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 81: self.vendor = vendorXML.childNodes[0].data Line 82: productXML = caps.getElementsByTagName('product')[0] Line 83: self.product_id = productXML.getAttribute('id') Line 84: self.product = productXML.childNodes[0].data Line 85: except: please use more specific exception Line 86: # althought the retrieval of product/vendor was not successful, Line 87: # we can still report back the name Line 88: pass Line 89: 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: return os.path.basename(os.readlink('/sys/bus/pci/devices/%s/iommu_group' % self._getSysAddress(sef.node.name()))) not tried, so not sure. Line 96: except OSError: Line 97: return '' Line 98: Line 99: def _getSysAddress(self, deviceName): Line 99: def _getSysAddress(self, deviceName): Line 100: try: Line 101: return '{0}:{1}:{2}.{3}'.format(*deviceName.split('_')[1:]) Line 102: except IndexError: Line 103: print deviceName.split('_')[1:] remove from final patch :) Line 104: return '' Line 105: Line 106: @property Line 107: def extendedParams(self): Line 116: Line 117: Line 118: class HostDeviceMapper(object): Line 119: Line 120: _instance = None what is this used for? 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), ''] Is this list always composed by two elements? if so a collections.namedtuple could probably improve the readability 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(): -- 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
