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 <mpolednik(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Martin Betak <mbetak(a)redhat.com>
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes