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