Change in vdsm[master]: vdsm: introduce hostdev module
danken at redhat.com
danken at redhat.com
Thu Sep 4 22:21:17 UTC 2014
Dan Kenigsberg has posted comments on this change.
Change subject: vdsm: introduce hostdev module
......................................................................
Patch Set 6:
(9 comments)
please add a unit test for each new function.
http://gerrit.ovirt.org/#/c/32313/6/vdsm/hostdev.py
File vdsm/hostdev.py:
Line 53: def _get_devices_from_vms(vmContainer):
Line 54: devices = {}
Line 55:
Line 56: # loop through VMs and find their host devices
Line 57: for vmId, params in vmContainer.items():
vmContainer is a mapping of vmId -> Vm object. not "params".
Line 58: try:
Line 59: for device in params.conf['devices']:
Line 60: if device['device'] == 'hostdev':
Line 61: try:
Line 59: for device in params.conf['devices']:
Line 60: if device['device'] == 'hostdev':
Line 61: try:
Line 62: devices[device['name']] = vmId
Line 63: except KeyError:
We have two dictionary lookup inside the try-block. It's safer two have only one.
Line 64: # VM has device which is no (longer) present
Line 65: # on the host - althought this should be handled by
Line 66: # user, warning can help debugging libvirt errors
Line 67: logging.warning('VM %s has device %s which is not '
Line 67: logging.warning('VM %s has device %s which is not '
Line 68: 'available!', vmId, device['name'])
Line 69: # BC: ignore old VM's without conf['devices'] section, no handling
Line 70: # needed as hostdev was not supported
Line 71: except KeyError:
KeyError can be raised by all sort of funny mistakes in the try-block. When you would like to catch a specific error, make sure that you catch it *alone*. In this case,
if 'devices' not in params.conf:
continue
would do.
Line 72: pass
Line 73:
Line 74: return devices
Line 75:
Line 73:
Line 74: return devices
Line 75:
Line 76:
Line 77: def _get_devices_from_libvirt(vmContainer):
no need for the arg
Line 78: """
Line 79: Method that scans all VMs in supported vmContainer, locates node
Line 80: devices and pairs them with VM in internal mapping, removing VMs or
Line 81: devices that no longer exist
Line 84: host_devices = [(dev.name(), dev) for dev
Line 85: in libvirtconnection.get().listAllDevices(0)]
Line 86:
Line 87: for (device_name, device) in host_devices:
Line 88: devices[device_name] = _parse_device_params(device.XMLDesc())
device_name is used only once. The code would be much simpler if you evaluate it right here.
I suspect that all you need is
return dict(device.name(), _parse_device_params(device.XMLDesc())
for device in libvirtconnection.get().listAllDevices(0))
Line 89:
Line 90: return devices
Line 91:
Line 92:
Line 103: libvirt_devices = _get_devices_from_libvirt(vmContainer)
Line 104: device_to_vm = _get_devices_from_vms(vmContainer)
Line 105:
Line 106: for name, params in libvirt_devices.items():
Line 107: devices[name] = {'params': params, 'vmId': ''}
if the device is not owned by any vm, a nicer API is to leave vmId out of the dictionary.
Line 108: try:
Line 109: devices[name]['vmId'] = device_to_vm[name]
Line 110: except KeyError:
Line 111: pass
Line 108: try:
Line 109: devices[name]['vmId'] = device_to_vm[name]
Line 110: except KeyError:
Line 111: pass
Line 112: logging.info('DBG: %s', devices)
debug time leftover?
Line 113:
Line 114: for device_name, assignment in devices.items():
Line 115: if caps and assignment['params']['capability'] not in caps:
Line 116: del(devices[device_name])
Line 110: except KeyError:
Line 111: pass
Line 112: logging.info('DBG: %s', devices)
Line 113:
Line 114: for device_name, assignment in devices.items():
"assignment" is still a bad name. it's a dictionary that has both params and owning vmId.
Line 115: if caps and assignment['params']['capability'] not in caps:
Line 116: del(devices[device_name])
Line 117:
Line 112: logging.info('DBG: %s', devices)
Line 113:
Line 114: for device_name, assignment in devices.items():
Line 115: if caps and assignment['params']['capability'] not in caps:
Line 116: del(devices[device_name])
It's wasteful to add the devince_name to the dictionary, and now delete it. If it does not have the right caps - do not add it.
Line 117:
--
To view, visit http://gerrit.ovirt.org/32313
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2bba96db5be180d00cb74fb89b10c5b09e5bd180
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list