Change in vdsm[master]: iproute2 ip link wrapper
danken at redhat.com
danken at redhat.com
Sat Nov 9 23:58:20 UTC 2013
Dan Kenigsberg has posted comments on this change.
Change subject: iproute2 ip link wrapper
......................................................................
Patch Set 8: Code-Review-1
(5 comments)
yes, it would be great to have one patch introducing the ipwrapper changes, and another fixing the bug.
....................................................
File lib/vdsm/ipwrapper.py
Line 87: return '%s: %s(%s) %s' % (self.index, self.name, self.type,
Line 88: self.address)
Line 89:
Line 90: @staticmethod
Line 91: def parse(text):
I do not share this grand statement. classes and their staticmethod are useful as name spaces. In this case, I find ipwrapper.Link.parse(text) clearer than ipwrapper.parseLinkAttr(text).
But shouldn't it be _parse(), btw? I do not expect anyone outside the class to need it. (yeah, I know that other classes in this module has it named like that)
Line 92: """parses a line of output from "ip -o -d link" into a Link attribute
Line 93: dictionary and returns it."""
Line 94: attrs = {}
Line 95: attrs['index'], attrs['name'], data = [el.strip() for el in
Line 88: self.address)
Line 89:
Line 90: @staticmethod
Line 91: def parse(text):
Line 92: """parses a line of output from "ip -o -d link" into a Link attribute
pep257 says that this should be in the imperative tense, and fit into a single line.
"""The docstring is a phrase ending in a period. It prescribes the function or method's effect as a command ("Do this", "Return that"), not as a description; e.g. don't write "Returns the pathname ..."."""
I know that we've not been strict about it, but please take note.
Line 93: dictionary and returns it."""
Line 94: attrs = {}
Line 95: attrs['index'], attrs['name'], data = [el.strip() for el in
Line 96: text.split(':', 2)]
Line 100:
Line 101: baseData = [el for el in
Line 102: processedData[0].split('>')[1].strip().split(' ') if el and
Line 103: el != 'link/none']
Line 104: for i in range(0, len(baseData), 2):
pairwise() from http://docs.python.org/2/library/itertools.html#recipes may be cooler. for your consideration.
Line 105: key, value = baseData[i:i+2]
Line 106: if key.startswith('link/'):
Line 107: key = 'address'
Line 108: attrs[key] = value
Line 129: attrs['name'] = attrs['name'].split('@')[0]
Line 130: return cls(**attrs)
Line 131:
Line 132: @staticmethod
Line 133: def detectType(name):
Here, again, I think that it makes sense to keep this helper function aggregated within the class that uses it, and make its name _private.
Line 134: """Returns the LinkType for the specified device."""
Line 135: # TODO: Add support for virtual functions
Line 136: detectedType = None
Line 137: rc, out, _ = execCmd([_ETHTOOL_BINARY.cmd, '-i', name])
....................................................
File tests/functional/networkTests.py
Line 1504: Link class."""
Line 1505: BIG_MTU = '2000'
Line 1506: VLAN_NAME = '%s.%s' % (BONDING_NAME, VLAN_ID)
Line 1507: with dummyIf(2) as nics:
Line 1508: with self.vdsm_net.pinger():
I think we have more than enough tests using pinger(), each adding 1/2 a second to the already-long runtime of this module. Please use {'connectivityCheck': False} instead, and save an indentation level with it ;-)
Line 1509: status, msg = self.vdsm_net.setupNetworks(
Line 1510: {NETWORK_NAME:
Line 1511: {'bonding': BONDING_NAME, 'bridged': True,
Line 1512: 'vlan': VLAN_ID, 'mtu': BIG_MTU}},
--
To view, visit http://gerrit.ovirt.org/21054
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibeee70574536e838076704e76f86f2777d5db9b0
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list