Antoni Segura Puimedon has posted comments on this change.
Change subject: iproute2 binary wrapper
......................................................................
Patch Set 4: (8 inline comments)
Thanks for doing the wrapper. A few comments to improve the modeling and the
robustness of the parsing.
....................................................
File vdsm/netconf/iproute2.py
Line 25:
Line 26:
Line 27: class Route(object):
Line 28: def __init__(self, device=None, network=None, ipaddr=None,
table=None,
Line 29: gateway=None, text=None):
What about to take text out of here and have a classmethod that takes just a
textual representation and does:
@classmethod
def routeFromText(cls, text):
device, network, ipaddr, table = cls._parse(text)
return cls(device, network, ipaddr, table)
or even:
@classmethod
def routeFromText(cls, text):
return cls(*cls._parse(text))
Line 30: '''
Line 31: * Two allowed possibilities:
Line 32: 1) default via <gateway> ...
Line 33: 2) <network> via <ip> ...
Line 45: (network is None or ipaddr is None):
Line 46: raise ValueError("network and ip are required for
non-default "
Line 47: "routing entry")
Line 48:
Line 49: if text and (device or network or ipaddr or table or gateway):
With the proposal of line 29 this would be unnecessary.
Line 50: raise ValueError("Must supply either a text in textual
format "
Line 51: "or parameters, not both")
Line 52:
Line 53: if not text:
Line 59: else:
Line 60: (self.device, self.network, self.ipaddr, self.table,
Line 61: self.gateway) = self._parse(text)
Line 62:
Line 63: def _parse(self, route):
It doesn't use anything from the instance. It should probably be a staticmethod
or at most a classmethod.
Line 64: route = route.split()
Line 65:
Line 66: if len(route) not in (5, 7):
Line 67: raise ValueError("Route parse error")
Line 62:
Line 63: def _parse(self, route):
Line 64: route = route.split()
Line 65:
Line 66: if len(route) not in (5, 7):
where do these textual representations come from? Is it the output of ip route
show or just the serialization done by us in multiple gateways? If it is the
former, different versions of iproute have different amount of returning
parameters, I'm afraid.
iproute-2.6.32:
default via 10.34.63.254 dev rhevm
iproute-3.9.0-1.fc19.x86_64
default via 10.34.131.254 dev em1 proto static
Thus, handling this parsing will have to be a bit more complicated, I guess.
network, data = route[0], route[1:]
network here will be either default or a value like 10.0.0.0/20. I'd substitute
default for '0.0.0.0/0'. Then we see that the rest of data is tag-value, so we
generate a dict:
data = dict(data[i:i+2] for i in range(0, len(data), 2))
ipaddr = data['via']
device = data['dev']
We do the convenient netaddr checks. Then:
table = data.get('table')
return device, network, ipaddr, table
Line 67: raise ValueError("Route parse error")
Line 68:
Line 69: if route[0] == 'default':
Line 70: gateway = route[2]
Line 87: "while parsing route")
Line 88:
Line 89: device = route[4]
Line 90: table = route[6] if len(route) == 7 else None
Line 91: return (device, network, ipaddr, table, gateway)
As per the above comments, I'd drop gateway.
Line 92:
Line 93: def __str__(self):
Line 94: if self.network:
Line 95: str = "%s via %s" % (self.network, self.ipaddr)
Line 92:
Line 93: def __str__(self):
Line 94: if self.network:
Line 95: str = "%s via %s" % (self.network, self.ipaddr)
Line 96: else: # default route
since we did away with gateway and replaced 'default' with '0.0.0.0/0' here we
don't need the else at all.
ip route add 0.0.0.0/0 via 10.0.0.254
is equivalent to
ip route add default via 10.0.0.254
Line 97: str = "default via %s" % self.gateway
Line 98:
Line 99: str += " dev %s" % self.device
Line 100:
Line 108: yield word
Line 109:
Line 110:
Line 111: class Rule(object):
Line 112: def __init__(self, source=None, destination=None, table=None,
text=None):
I'd apply my comments for "text" for Route here.
Line 113: if not text and table is None:
Line 114: raise ValueError("table is a required parameter")
Line 115:
Line 116: if text and (source or destination or table):
Line 184: return output
Line 185:
Line 186:
Line 187: def ipRoute():
Line 188: command = [ipBinary, "route"]
In general for this and the following methods, I'd just do away with the
command variable and I'd put the command straight into return _execCmd(..)
Line 189: return _execCmd(command)
Line 190:
Line 191:
Line 192: def ipRouteShowTable(table):
--
To view, visit http://gerrit.ovirt.org/15335
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d315c3294fd7f058cdc840dea329d91a658a304
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Assaf Muller <[email protected]>
Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]>
Gerrit-Reviewer: Assaf Muller <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Livnat Peer <[email protected]>
Gerrit-Reviewer: Mark Wu <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
