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