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():