Change in vdsm[master]: [WIP] netwiring: [4/4] Add API definitions.
danken at redhat.com
danken at redhat.com
Fri Dec 7 07:46:55 UTC 2012
Dan Kenigsberg has posted comments on this change.
Change subject: [WIP] netwiring: [4/4] Add API definitions.
......................................................................
Patch Set 22: I would prefer that you didn't submit this
(7 inline comments)
I like this version - and understand it - much much better!
....................................................
File vdsm/dummybr.py
Line 24:
Line 25: from vdsm import libvirtconnection, utils, constants
Line 26:
Line 27:
Line 28: DUMMY_BRIDGE = ';;;;;;;;'
I'm pretty sure I gave the colon ":" as an example. the semicolon is not in ILLEGAL_BRIDGE_CHARS.
how about
DUMMY_BRIDGE = ':dummybridge:'
so it's clearer for the casuasal brctl shower?
Line 29:
Line 30:
Line 31: def createEphemeralBridge(bridgeName):
Line 32: rc, out, err = utils.execCmd([constants.EXT_BRCTL, 'addbr', bridgeName])
Line 36:
Line 37:
Line 38: def addBridgeToLibvirt(bridgeName):
Line 39: conn = libvirtconnection.get()
Line 40: if bridgeName not in conn.listNetworks():
wouldn't it be nicer to "ask for forgiveness"?
do you intentionally not follow the LIBVIRT_NET_PREFIX convention? It think that it would be clearer to show that vdsm owns this dummy network.
Line 41: conn.networkCreateXML(
Line 42: '''<network><name>%s</name><forward mode='bridge'/><bridge '''
Line 43: '''name='%s'/></network>''' % (bridgeName, bridgeName))
Line 44:
....................................................
File vdsm/libvirtvm.py
Line 54: DRIVE_NOT_FOUND = "Drive Not Found"
Line 55: BASE_NOT_FOUND = "Base Not Found"
Line 56:
Line 57:
Line 58: class setLinkAndNetworkError(Exception):
classes should begin with a Capital letter.
Line 59: pass
Line 60:
Line 61:
Line 62: class updatePortMirroringError(Exception):
Line 1590: except Exception as e:
Line 1591: # Rollback link and network.
Line 1592: self._dom.updateDeviceFlags(vnicXML.toxml(encoding='utf-8'),
Line 1593: libvirt.VIR_DOMAIN_AFFECT_LIVE)
Line 1594: if isinstance(e, updatePortMirroringError):
this is not vey pretty, as it assumes a very specific order of usage (update mirror within linkAndNetwork).
how about raising SetLinkAndNetworkError in a try-block BEFORE the yield of line 1589?
Line 1595: raise
Line 1596: else:
Line 1597: self.log.debug('Request to update the the vnic %s failed. '
Line 1598: 'Made with the following xml: %s',
Line 1593: libvirt.VIR_DOMAIN_AFFECT_LIVE)
Line 1594: if isinstance(e, updatePortMirroringError):
Line 1595: raise
Line 1596: else:
Line 1597: self.log.debug('Request to update the the vnic %s failed. '
I prefer shorter English in log files. Remember that the function name is already logged.
Line 1598: 'Made with the following xml: %s',
Line 1599: dev.alias,
Line 1600: vnicXML.toprettyxml(encoding='utf-8'),
Line 1601: exc_info=True)
Line 1643: def updateDevice(self, params):
Line 1644: if params.get('deviceType') == vm.NIC_DEVICES:
Line 1645: return self._updateInterfaceDevice(params)
Line 1646: else:
Line 1647: return errCode['noimpl']
I suppose that the very similar condition in API.py is a bit redundant if you have this here?
Line 1648:
Line 1649: def hotunplugNic(self, params):
Line 1650: if self.isMigrating():
Line 1651: return errCode['migInProgress']
....................................................
File vdsm/netinfo.py
Line 76: return [b.split('/')[-2] for b in glob.glob('/sys/class/net/*/bridge')
Line 77: if b.split('/')[-2] != DUMMY_BRIDGE]
Line 78:
Line 79:
Line 80: def networks():
I think we should do the same filtering for networks - we do not want to report the dummy network.
Line 81: """
Line 82: Get dict of networks from libvirt
Line 83:
Line 84: :returns: dict of networkname={properties}
--
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: 22
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Alona Kaplan <alkaplan at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky at redhat.com>
Gerrit-Reviewer: Livnat Peer <lpeer at redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: ShaoHe Feng <shaohef at linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
More information about the vdsm-patches
mailing list