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