Dan Kenigsberg has posted comments on this change. Change subject: hostdev: add dynamic udev rule creation for iommu groups ......................................................................
Patch Set 6: Code-Review-1 (5 comments) http://gerrit.ovirt.org/#/c/36268/6/vdsm/hostdev.py File vdsm/hostdev.py: Line 18: # Refer to the README and COPYING files for full details of the license Line 19: # Line 20: Line 21: from functools import partial Line 22: from collections import namedtuple please keep sorted Line 23: import xml.etree.ElementTree as etree Line 24: Line 25: from vdsm import libvirtconnection Line 26: import supervdsm Line 25: from vdsm import libvirtconnection Line 26: import supervdsm Line 27: Line 28: _DETACH_REQUIRING_CAPS = ('usb_device', 'pci') Line 29: _DETACH_REQUIRING_UDEV = ('pci') this is a tuple of caps that require UDEV rule. UDEV_REQUIRING_CAPS is a better name. Line 30: _DETACH = 0 Line 31: _REATTACH = 1 Line 32: Line 33: _functions_template = namedtuple('functions_template', ['libvirt', 'udev']) Line 142: Line 143: iommu_group = device_params['iommu_group'] Line 144: Line 145: if device_params['capability'] in _DETACH_REQUIRING_UDEV: Line 146: function_mapping[action].udev(iommu_group) I understand the pain of writing a repetitious code, but remember: code is read much more than it is written. I believe that a not-so-long if-else clauses inside detach_detachable() and reattach_detachable() would make this code much simpler, with no dynamic programming and one-use named tuples. Please try it out and let reviewers judge. Line 147: supervdsm.getProxy().udevTrigger(iommu_group) Line 148: Line 149: if device_params['capability'] in _DETACH_REQUIRING_CAPS: Line 150: function_mapping[action].libvirt() http://gerrit.ovirt.org/#/c/36268/6/vdsm/supervdsmServer File vdsm/supervdsmServer: Line 298: fails.append(r) Line 299: return fails Line 300: Line 301: @logDecorator Line 302: def vfioAppropriateDevice(self, iommu_group): appropriateIommuGroup seems a better name Line 303: """ Line 304: Create udev rule in /etc/udev/rules.d/ to change ownership Line 305: of /dev/vfio/$iommu_group to qemu:qemu. This method should be called Line 306: when detaching a device from the host. Line 314: self.log.debug("Creating rule %s: %r", ruleFile, rule) Line 315: rf.write(rule) Line 316: Line 317: @logDecorator Line 318: def vfioRmAppropriateDevices(self, iommu_group): rmAppropriateIommuGroupRule ? Line 319: """ Line 320: Remove udev rule in /etc/udev/rules.d/ created by Line 321: vfioAppropriateDevice. Line 322: """ -- To view, visit http://gerrit.ovirt.org/36268 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ieac8d58e01d7277e535a2101d522961816ea88eb Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Martin Polednik <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Martin Polednik <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
