Igor Lvovsky has posted comments on this change.
Change subject: [WIP] netwiring: [4/4] Add API definitions.
......................................................................
Patch Set 11: I would prefer that you didn't submit this
(8 inline comments)
....................................................
File vdsm/API.py
Line 357: if 'deviceType' not in params:
Line 358: self.log.error('Missing a required parameters:
deviceType')
Line 359: return {'status': {'code':
errCode['MissParam']['status']['code'],
Line 360: 'message': 'Missing one of required '
Line 361: 'parameters: type'}}
s/type/deviceType
Line 362: v = self._cif.vmContainer.get(self._UUID)
Line 363: if not v:
Line 364: return errCode['noVM']
Line 365: return v.updateDevice(params)
Line 360: 'message': 'Missing one of required '
Line 361: 'parameters: type'}}
Line 362: v = self._cif.vmContainer.get(self._UUID)
Line 363: if not v:
Line 364: return errCode['noVM']
I prefer try...except as we do in all other calls (look at hotplugNic)
Line 365: return v.updateDevice(params)
Line 366:
Line 367: def hotplugNic(self, params):
Line 368: try:
....................................................
File vdsm/libvirtvm.py
Line 1521: if 'alias' not in params:
Line 1522: self.log.error('Missing the required alias parameters.')
Line 1523: return {'status': {'code':
errCode['MissParam']['status']['code'],
Line 1524: 'message': 'Missing the required
alias '
Line 1525: 'parameter'}}
why you didn't check this in API.py as you did with deviceType?
Line 1526:
Line 1527: netDev = None
Line 1528: for dev in self.conf['devices']:
Line 1529: if dev['type'] == vm.NIC_DEVICES and \
Line 1540:
Line 1541: network = netDev['network']
Line 1542:
Line 1543: # Prepare the updateDevice xml
Line 1544: netelem = xml.dom.minidom.Element(params['type'])
should it be params['deviceType']?
Line 1545: netelem.setAttribute('type', 'bridge')
Line 1546: mac = xml.dom.minidom.Element('mac')
Line 1547: mac.setAttribute('address', netDev['macAddr'])
Line 1548: netelem.appendChild(mac)
Line 1549: model = xml.dom.minidom.Element('model')
Line 1550: model.setAttribute('type', netDev['nicModel'])
Line 1551: netelem.appendChild(model)
Line 1552:
Line 1553: if 'network' not in params:
What happen if we got network=None ?
Is it legal?
Line 1554: # If no network is specified we take the vnic to the
dummy bridge
Line 1555: # and set the link 'down' always.
Line 1556: source = xml.dom.minidom.Element('source')
Line 1557: source.setAttribute('bridge', DUMMY_BRIDGE)
Line 1575: self._dom.updateDeviceFlags(
Line 1576: netelem.toxml(),
Line 1577: libvirt.libvirt.VIR_DOMAIN_AFFECT_LIVE)
Line 1578: except:
Line 1579: self.log.debug("updateInterfaceDevice failed",
Maybe we need more verbose explanation (e.g. same as in exception).
In additional consider to add xml itself to the log.
The function name anyway will be printed
Line 1580: exc_info=True)
Line 1581: return {'status':
Line 1582: {'code':
errCode['updateDevice']['status']['code'],
Line 1583: 'message': 'Failed to take the link
down.'}}
Line 1593: self._dom.updateDeviceFlags(
Line 1594: netelem.toxml(),
Line 1595: libvirt.libvirt.VIR_DOMAIN_AFFECT_LIVE)
Line 1596: except:
Line 1597: self.log.debug("updateInterfaceDeviceFlags failed",
exc_info=True)
Same as above
Line 1598: return {'status':
Line 1599: {'code':
errCode['updateDevice']['status']['code'],
Line 1600: 'message': 'Failed to take the link %s' %
Line 1601: link.getAttribute('state')}}
....................................................
File vdsm/vdsmd.init.in
Line 505: try:
Line 506: conn.networkCreateXML('''<network><name>$DUMMY_BR</name><forward
mode='bridge'/><bridge name='$DUMMY_BR'/></network>''')
Line 507: except:
Line 508: sys.exit(1)
Line 509: sys.exit(0)
Hate this even more :).
Any chance to put it in external script?
Line 510: EOF
Line 511: ret_val=$?
Line 512: if [ $ret_val -ne 0 ]
Line 513: then
--
To view, visit http://gerrit.ovirt.org/9562
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b9b4f49f80466a83e3e13f1042ac2a8866c6bcd
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <[email protected]>
Gerrit-Reviewer: Alona Kaplan <[email protected]>
Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Igor Lvovsky <[email protected]>
Gerrit-Reviewer: Livnat Peer <[email protected]>
Gerrit-Reviewer: Mark Wu <[email protected]>
Gerrit-Reviewer: ShaoHe Feng <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
