Francesco Romani has posted comments on this change. Change subject: hostdev: add dynamic udev rule creation for iommu groups ......................................................................
Patch Set 4: Code-Review-1 (4 comments) question inside, -1 for visibility http://gerrit.ovirt.org/#/c/36268/4/vdsm/hostdev.py File vdsm/hostdev.py: Line 132: 'udev': supervdsm.getProxy(). Line 133: vfioAppropriateDevice}, Line 134: _REATTACH: {'libvirt': libvirt_device.reAttach, Line 135: 'udev': supervdsm.getProxy(). Line 136: vfioRmAppropriateDevices}} I wonder if code becomes a little cleaner without this top-level dictionary, e.g. with two dicts, one per action. I also wonder if a namedtuple looks nicer. E.g. instead of _detach['libvirt']() something like _detach.libvirt() Line 137: Line 138: iommu_group = device_params['iommu_group'] Line 139: Line 140: if device_params['capability'] in _DETACH_REQUIRING_UDEV: Line 144: if device_params['capability'] in _DETACH_REQUIRING_CAPS: Line 145: if action == _DETACH: Line 146: # Unfortunately, we need to pass None (to use the default driver) Line 147: # to detachFlags and nothing to reAttach Line 148: function_mapping[action]['libvirt'](None) you could use functools.partial and/or a lambda/helper if you think that difference (None vs. nothing) leads to too ugly code. *If* you drop the function_mapping dict I think this is bearable. Line 149: else: Line 150: function_mapping[action]['libvirt']() Line 151: Line 152: http://gerrit.ovirt.org/#/c/36268/4/vdsm/supervdsmServer File vdsm/supervdsmServer: Line 81: _UDEV_RULE_FILE_EXT = ".rules" Line 82: _UDEV_RULE_FILE_NAME = _UDEV_RULE_FILE_DIR + _UDEV_RULE_FILE_PREFIX + \ Line 83: "%s-%s" + _UDEV_RULE_FILE_EXT Line 84: _UDEV_RULE_FILE_NAME_VFIO = _UDEV_RULE_FILE_DIR + _UDEV_RULE_FILE_PREFIX + \ Line 85: "iommu_group_%s" + _UDEV_RULE_FILE_EXT I wonder why we don't use os.path.join, but this is probably stuff for another patch. Line 86: Line 87: RUN_AS_TIMEOUT = config.getint("irs", "process_pool_timeout") Line 88: Line 89: _running = True Line 302: def vfioAppropriateDevice(self, iommu_group): 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. I see your point, but creating and deleting rules on the fly strikes me. I vividly remember our last udev/selinux/systemd troubles with live extend. I'd _really_ like to avoid a mess like that :) There is any other option available? Maybe a more generic rule to be set ahead of time? Just for my understanding, was this option been evaluated? If so, why it was discarded? Line 307: """ Line 308: ruleFile = _UDEV_RULE_FILE_NAME_VFIO % iommu_group Line 309: rule = ('KERNEL=="{}", SUBSYSTEM=="vfio" RUN+="{} {}:{} ' Line 310: '/dev/vfio/{}"').format(iommu_group, EXT_CHOWN, -- 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: 4 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: 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
