Change in vdsm[master]: hostdev: add dynamic udev rule creation for iommu groups
fromani at redhat.com
fromani at redhat.com
Thu Jan 29 14:13:44 UTC 2015
Francesco Romani has posted comments on this change.
Change subject: hostdev: add dynamic udev rule creation for iommu groups
......................................................................
Patch Set 6:
(2 comments)
http://gerrit.ovirt.org/#/c/36268/6/vdsm/hostdev.py
File vdsm/hostdev.py:
Line 130: for device in libvirtconnection.get().listAllDevices(0))
Line 131:
Line 132:
Line 133: def _device_routine_internal(libvirt_device, device_params, action):
Line 134: function_mapping = {
too generic. something like device_operation or something like it along these lines it is more readable. (device_setup?)
Line 135: _DETACH: _functions_template(partial(libvirt_device.detachFlags,
Line 136: driverName=None),
Line 137: supervdsm.getProxy().
Line 138: vfioAppropriateDevice),
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
TL;DR:
Even though I like version 6, Dan has a good point, and I find version 7 *slightly* better overall, so I'm ok with that.
---
Longer Rationale:
I don't like when patches start ping-ponging between different requests from reviewers. I of course acknowledge that could be useful or needed, but nevertheless I try to avoid it as much as I can.
That said, Dan has a point here, but honestly both versions are somehow unsatisfying - for different reasons. It is not mpolednik's fault, it is just about that the flows are maybe only deceptively similar.
On one hand, revision 6 could be further refined like I suggested, but even though the code is nice on its own, I think it somehow fails to extract the common part: too many if()s -either explicit or hidden- in a supposedly shared code is a smell.
On the other hand, when I see version 7 I almost automatically think "nice, but something can be factored out" even though I *know* the background story.
Code is still "surprising" in its own little way.
The lesser evil seems version 7: complexity has a cost, the result is not brilliant (again: not mpolednik's fault in any way), that's why I'd go with it.
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()
--
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 <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