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 <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Ido Barkan <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
