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