Change in vdsm[master]: hostdev: add support for hotplug
fromani at redhat.com
fromani at redhat.com
Thu Jul 16 11:52:21 UTC 2015
Francesco Romani has posted comments on this change.
Change subject: hostdev: add support for hotplug
......................................................................
Patch Set 3:
(4 comments)
very few initial comments and questions, source for discussion, not actual review (hence no score)
https://gerrit.ovirt.org/#/c/42661/3/vdsm/virt/vm.py
File vdsm/virt/vm.py:
Line 2026: return errCode['migInProgress']
Line 2027:
Line 2028: dev_specs = params['devices']
Line 2029: dev_objects = []
Line 2030: for dev_spec in dev_specs:
maybe wrap the body into a try/except block?
Well, let's put it differently: what happens if one device raises? should we go ahead with the others or abort the whole call?
Line 2031: dev_objects.append(vmdevices.hostdevice.HostDevice(
Line 2032: self.conf, self.log, **dev_spec
Line 2033: ))
Line 2034: dev_object = dev_objects[-1]
Line 2031: dev_objects.append(vmdevices.hostdevice.HostDevice(
Line 2032: self.conf, self.log, **dev_spec
Line 2033: ))
Line 2034: dev_object = dev_objects[-1]
Line 2035: dev_object.detach()
why not simply
dev_object = vmdevices.hostdevice.HostDevice(...)
dev_object.detach()
dev_objects.append(dev_object)
?
Line 2036:
Line 2037: try:
Line 2038: dev_xml = dev_object.getXML().toprettyxml(encoding='utf-8')
Line 2039: except vmdevices.core.SkipDevice:
Line 2049: self._dom.attachDevice(dev_xml)
Line 2050: except libvirt.libvirtError as e:
Line 2051: self.log.exception("Hotplug failed")
Line 2052: if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN:
Line 2053: return errCode['noVM']
while you're at it, you can use response.error() also here :)
Line 2054: return response.error('hotplugHostdev', e.message)
Line 2055: else:
Line 2056: # FIXME! We may have a problem here if vdsm dies right after
Line 2057: # we sent command to libvirt and before save conf. In this case
Line 2062: self.conf['devices'].append(dev_spec)
Line 2063: self.saveState()
Line 2064: self._getUnderlyingHostDeviceInfo()
Line 2065:
Line 2066: return {'status': doneCode, 'vmList': self.status()}
and probably here too
Line 2067:
Line 2068: def _lookupDeviceByAlias(self, devType, alias):
Line 2069: for dev in self._devices[devType][:]:
Line 2070: try:
--
To view, visit https://gerrit.ovirt.org/42661
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2465360664ef9b659c52dc610a95d2c2f1555c54
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Ido Barkan <ibarkan at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list