Change in vdsm[master]: vdsm: add functionality to detach host devices

mpolednik at redhat.com mpolednik at redhat.com
Mon Oct 27 09:56:01 UTC 2014


Martin Polednik has posted comments on this change.

Change subject: vdsm: add functionality to detach host devices
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.ovirt.org/#/c/32389/3/vdsm/hostdev.py
File vdsm/hostdev.py:

Line 117: 
Line 118: def detach(device_name):
Line 119:     libvirt_device, device_params = _get_device_ref_and_params(device_name)
Line 120: 
Line 121:     if device_params['capability'] in _UNMANAGED_DEVICE_CAPS:
> Do you expect that many users would need the semantics of "detach the devic
Hopefully I'm starting to understand - current detach() function actually does two things: detaches_if_detachable() and returns the parsed XML to vmdevice space.

I'm ok with splitting it to detach_if_detachable (maybe phrased differently, but keeping the same logic) and something like get_parsed_params (possibly nicer name) - the functionality would essentially be the same.

The second option for me is to revert back to the name of acquire - where the acquire composite function is meant to process the request for device and execute routines necessary to prepare it for the VM, returning parameters of the prepared device. (that is what this function does, therefore this would only mean renaming it back to acquire)
Line 122:         libvirt_device.dettach()
Line 123: 


Line 118: def detach(device_name):
Line 119:     libvirt_device, device_params = _get_device_ref_and_params(device_name)
Line 120: 
Line 121:     if device_params['capability'] in _UNMANAGED_DEVICE_CAPS:
Line 122:         libvirt_device.dettach()
> the spelling mistake of dettach/detach is a sore in my eye. Could you use d
I understand your suffering :P At least on my f20 test box, detachFlags have signature detachFlags(self, driverName, flags=0) - I'll have a look if I can get driverName to use hypervisor default...
Line 123: 


-- 
To view, visit http://gerrit.ovirt.org/32389
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I530fed56831314286942b8afa62900208812bfb5
Gerrit-PatchSet: 3
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: Michal Skrivanek <mskrivan 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