Dan Kenigsberg has posted comments on this change.
Change subject: sourcerouting: fix _getRoute not to include local routes
......................................................................
Patch Set 1: Code-Review-1
(3 comments)
http://gerrit.ovirt.org/#/c/27262/1//COMMIT_MSG
Commit Message:
Line 5: CommitDate: 2014-04-30 16:38:34 +0200
Line 6:
Line 7: sourcerouting: fix _getRoute not to include local routes
Line 8:
Line 9: _getRoute, a method used to find the routes of a vdsm created table
s/vdsm created/vdsm-created/
Line 10: in Dynamic Source Routing was listing the routes of the table and
Line 11: matching by device, the problem is that such matching would wrongly
Line 12: include local scope routes. Having such route would generate an
Line 13: IPRoute2 exception when removing routes and rules would never be
Line 7: sourcerouting: fix _getRoute not to include local routes
Line 8:
Line 9: _getRoute, a method used to find the routes of a vdsm created table
Line 10: in Dynamic Source Routing was listing the routes of the table and
Line 11: matching by device, the problem is that such matching would wrongly
s/device,/device./
Line 12: include local scope routes. Having such route would generate an
Line 13: IPRoute2 exception when removing routes and rules would never be
Line 14: removed, thus leaving behind trash in the rule list.
Line 15:
Line 10: in Dynamic Source Routing was listing the routes of the table and
Line 11: matching by device, the problem is that such matching would wrongly
Line 12: include local scope routes. Having such route would generate an
Line 13: IPRoute2 exception when removing routes and rules would never be
Line 14: removed, thus leaving behind trash in the rule list.
Since we are missing tests for sourcerouting, I need much more detailed commit message to
understand the problem and its solution. Could you supply and example routing table that
would cause matching a local-scope routes?
Please explain how the changed code solves the problem. Adding a test with a sample output
of routeShowTable('all') may do the trick for me.
Line 15:
Line 16: Change-Id: I5b3d43c8a2077e40b8b4314f02ea17bc3968c42b
--
To view, visit
http://gerrit.ovirt.org/27262
To unsubscribe, visit
http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b3d43c8a2077e40b8b4314f02ea17bc3968c42b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes