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