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