Change in vdsm[master]: Change getMaxMtu to use the info provided by sysfs

wudxw at linux.vnet.ibm.com wudxw at linux.vnet.ibm.com
Mon Feb 4 06:23:40 UTC 2013


Mark Wu has posted comments on this change.

Change subject: Change getMaxMtu to use the info provided by sysfs
......................................................................


Patch Set 1: (2 inline comments)

....................................................
File vdsm/configNetwork.py
Line 905:         configWriter = ConfigWriter()
Line 906: 
Line 907:     prevmtu = None
Line 908:     if mtu:
Line 909:         prevmtu = netinfo.getMaxMtu(nics)
Yes, in the old implementation, getMaxMtu returns the maximum of all configured mtu and new desired mtu. And the variable name 'prevmtu' is not very accurate. It could make more sense to call it newMaxMtu.  However, we used prevmtu as it was the maximum mtu of existing configurations. 


    # First we need to prepare all conf files
    if bonding:
        configWriter.addBonding(bonding, bridge=bridgeForNic,
                                bondingOptions=bondingOptions,
                                mtu=max(prevmtu, mtu), **options)
                                              ^^^^^^^^^^^^^^^^^
                                              compare again.

    for nic in nics:
        configWriter.addNic(nic, bonding=bonding,
                            bridge=bridgeForNic if not bonding else None,
                            mtu=max(prevmtu, mtu), **options)
                                    ^^^^^^^^^^^^^^^^^^^^^
                                    the same here.

So in the new implementation,  I removed the new  mtu parameter from the function getMaxMtu(). Now the semantics of getMaxMtu is just the maximum mtu existing configurations.

After reviewing the code again,  I get a new question,  why do need check all nics to get the max mtu? My understanding multiple nics co-existing in the same network only happens to a bonding enabled network. And if they're all slaves of the same bonding, they should have the same MTU, right. So probably we could just check the mtu of one nic. Is it correct?
Line 910: 
Line 911:     nic = nics[0] if nics else None
Line 912:     iface = bonding or nic
Line 913:     if bridged:


....................................................
File vdsm/netinfo.py
Line 123: 
Line 124: 
Line 125: def getMaxMtu(nics):
Line 126:     maxMtu = int(getMtu(nics[0]))
Line 127:     for nic in nics[1:]:
Cool! I like it too. Thanks for pointing out that.  Just a small correction, the 'key' argument must be provided in the keyword form. Otherwise, the function object will be taken as an item to be compared.
Line 128:         mtu = int(getMtu(nics[0]))
Line 129:         if mtu > maxMtu:
Line 130:             maxMtu = mtu
Line 131:     return maxMtu


--
To view, visit http://gerrit.ovirt.org/11357
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I89d007eb81c2c64e642ed80af26f882f7632dc1c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw at linux.vnet.ibm.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: Mark Wu <wudxw at linux.vnet.ibm.com>


More information about the vdsm-patches mailing list