Martin Polednik has posted comments on this change. Change subject: vdsm: add functionality to acquire host devices ......................................................................
Patch Set 1: (3 comments) http://gerrit.ovirt.org/#/c/32389/1/vdsm/hostdev.py File vdsm/hostdev.py: Line 23: Line 24: from vdsm import libvirtconnection Line 25: Line 26: Line 27: DETACHABLE_DEVICES = ('usb', 'pci') > these are capabilities, not devices, right? if so, please rename. agreed Line 28: Line 29: Line 30: def _parse_device_params(device_xml): Line 31: params = {} Line 120: Line 121: return devices Line 122: Line 123: Line 124: def acquire(device_name): > What about this name? Libvirt calls the action "detach". Why should not we? This function is to be called inside device's constructor, the difference is that the libvirt sees that stack from below: [host [device1 device2 device3]] -> libvirt really does dettach the device, but from VDSM's pov: [libvirt [device1 device2 device3]] -> [vm1 vm2 vm3] here, we're acquiring the device from libvirt and giving it to one of our VMs Line 125: device = libvirtconnection.get().nodeDeviceLookupByName(device_name) Line 126: device_params = _parse_device_params(device) Line 127: Line 128: if device_params['capability'] in DETACHABLE_DEVICES: Line 124: def acquire(device_name): Line 125: device = libvirtconnection.get().nodeDeviceLookupByName(device_name) Line 126: device_params = _parse_device_params(device) Line 127: Line 128: if device_params['capability'] in DETACHABLE_DEVICES: > I'm a bit surprised: it is quite dangerous to silently ignore an action, ju Acquire will be called by any device - even non-detachable (at least to get device_params) and we therefore have to limit devices for which the real detach is called Line 129: device.dettach() Line 130: -- 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: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <mskri...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
