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 <mpoled...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches