Change in vdsm[master]: iproute2 ip link wrapper

nsoffer at redhat.com nsoffer at redhat.com
Fri Nov 8 21:59:47 UTC 2013


Nir Soffer has posted comments on this change.

Change subject: iproute2 ip link wrapper
......................................................................


Patch Set 8:

(5 comments)

Partial review regarding Python and object design.

Also, I guess this is not the minimal fix for that bug. Can we separate the fix from the new fancy parser?

....................................................
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):
staticmethod in Python is pointless - use module level function.
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 117:                          range(0, len(tokens)-1, 2))
Line 118:         return attrs
Line 119: 
Line 120:     @classmethod
Line 121:     def fromText(cls, text, detection=True):
This could be also a regular function.
Line 122:         """Creates a Link object from the textual representation from
Line 123:         iproute2's ip link show command."""
Line 124:         attrs = cls.parse(text)
Line 125:         if 'linkType' not in attrs:


Line 129:             attrs['name'] = attrs['name'].split('@')[0]
Line 130:         return cls(**attrs)
Line 131: 
Line 132:     @staticmethod
Line 133:     def detectType(name):
Why this belong to the Link class?
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 lib/vdsm/netinfo.py
Line 78: 
Line 79:     fakeNics = config.get('vars', 'fake_nics').split(',')
Line 80:     for device in deviceLinks:
Line 81:         if device.type == LinkType.NIC or (device.type == LinkType.DUMMY and
Line 82:            _match_name(device.name, fakeNics)):
Can we use smarter object?

    if device.isNIC() or (device.isDUMMY() and device.isFake()):
        yield device.name
Line 83:             yield device.name
Line 84: 
Line 85: 
Line 86: def nics():


Line 144: 
Line 145: def vlans():
Line 146:     hidden_vlans = config.get('vars', 'hidden_vlans').split(',')
Line 147:     return [link.name for link in getLinks() if link.type == LinkType.VLAN and
Line 148:             not _match_name(link.name, hidden_vlans)]
If we have a link object, maybe we can clean up this code by making the object little smarter?

    return [link.name for link in getLinks() 
            if link.isVLAN() and not link.isHidden()]
Line 149: 
Line 150: 
Line 151: def bridges():
Line 152:     return [b.split('/')[-2] for b in iglob('/sys/class/net/*/bridge')


-- 
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