Change in vdsm[master]: ip route: parse unreachable and broadcast routes
osvoboda at redhat.com
osvoboda at redhat.com
Tue Dec 23 11:37:23 UTC 2014
Ondřej Svoboda has posted comments on this change.
Change subject: ip route: parse unreachable and broadcast routes
......................................................................
Patch Set 4: Code-Review-1
(2 comments)
I think in the original code we were mixing flags (local) with "networks". I'll try to unify and split the handling of flags from the rest.
Even then I am not sure that the parser is/was correct from the viewpoint of 'ip route' grammar but we are not aiming for a complete parser anyway.
http://gerrit.ovirt.org/#/c/35971/4/lib/vdsm/ipwrapper.py
File lib/vdsm/ipwrapper.py:
Line 346: flags = {}
Line 347: for flag in ('broadcast', 'unreachable'):
Line 348: if route[0] == flag:
Line 349: flags[flag] = True
Line 350: route = route[1:]
As explained in the second comment, I think that you can move the parsing of "local" here as well. That way, the next if-else can be replaced with just
params = route[1:]
Further on the 'network' variable will no longer contain 'local' but '127.0.0.1'. However, we will have the 'local' flag. I think it's better for me to explain this in code, I am almost writing it here.
Line 351:
Line 352: network = route[0]
Line 353: if network == 'local':
Line 354: params = route[2:]
Line 350: route = route[1:]
Line 351:
Line 352: network = route[0]
Line 353: if network == 'local':
Line 354: params = route[2:]
Maybe we should a comment that this is to skip the loopback address as in
local 127.0.0.1 dev lo src 127.0.0.1
(so the "params" become 'dev': 'lo' and 'src': '127.0.0.1')
But I am not convinced that we need special handling for "local" as it is just another flag before an address that we skip in the "else" clause.
Line 355: else:
Line 356: params = route[1:]
Line 357:
Line 358: data = dict(params[i:i + 2] for i in range(0, len(params), 2))
--
To view, visit http://gerrit.ovirt.org/35971
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia0a882aa1b57dc0c7b26095205b884ae9eff0173
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Ido Barkan <ibarkan at redhat.com>
Gerrit-Reviewer: Ondřej Svoboda <osvoboda at redhat.com>
Gerrit-Reviewer: Petr Horáček <phoracek at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list