From: Igor Maravic <[email protected]> Also, don't check if metric is larger then RIP_INFINITY, before the route passes through the policy filters. EXPORT and IMPORT filter, could posibly change metric and make it smaller then RIP_INFINITY.
After the route passes through the EXPORT filter, or if it doesn't pass IMPORT or EXPORT_SOURCEMATCH filters, check the route cost. If it's larger then RIP_INFINITY set it to ROUTE_INFINITY. Don't run EXPORT filter when the outgoing packet is sent, because route is pushed through the EXPORT filter when it comes to RIP. Signed-off-by: Igor Maravic <[email protected]> --- xorp/rip/output.hh | 29 --------------------- xorp/rip/output_table.cc | 17 ++++++------- xorp/rip/output_updates.cc | 11 ++++---- xorp/rip/route_db.cc | 60 +++++++++++++++++++++++++++++--------------- xorp/rip/route_db.hh | 2 +- 5 files changed, 55 insertions(+), 64 deletions(-) diff --git a/xorp/rip/output.hh b/xorp/rip/output.hh index 24fed44..13d04e5 100644 --- a/xorp/rip/output.hh +++ b/xorp/rip/output.hh @@ -129,14 +129,6 @@ protected: void incr_packets_sent() { _pkts_out++; } - /** - * Policy filters the route. - * - * @param r route to filter. - * @return true if the route was accepted, false otherwise. - */ - bool do_filtering(RouteEntry<A>* r); - protected: EventLoop& _e; Port<A>& _port; // Port associated with output @@ -190,25 +182,4 @@ OutputBase<A>::interpacket_gap_ms() const return _port.constants().interpacket_delay_ms(); } - -template <typename A> -bool -OutputBase<A>::do_filtering(RouteEntry<A>* route) -{ - try { - RIPVarRW<A> varrw(*route); - - debug_msg("[RIP] Running export filter on route: %s\n", - route->net().str().c_str()); - - bool accepted = _policy_filters.run_filter(filter::EXPORT, - varrw); - - return accepted; - } catch(const PolicyException& e) { - XLOG_FATAL("PolicyException: %s", e.str().c_str()); - XLOG_UNFINISHED(); - } -} - #endif // __RIP_OUTPUT_HH__ diff --git a/xorp/rip/output_table.cc b/xorp/rip/output_table.cc index dc2237e..b3d470f 100644 --- a/xorp/rip/output_table.cc +++ b/xorp/rip/output_table.cc @@ -50,15 +50,13 @@ OutputTable<A>::output_packet() // or set cost to infinity... // or depending on poison-reverse / horizon settings // - if (r->filtered()) { + if (r->filtered()) continue; - } pair<A,uint16_t> p = this->_port.route_policy(*r); - if (p.second > RIP_INFINITY) { + if (p.second > RIP_INFINITY) continue; - } RouteEntryOrigin<A>* origin = NULL; // XXX string ifname, vifname; // XXX: not set, because not needed @@ -68,11 +66,12 @@ OutputTable<A>::output_packet() origin, r->tag(), r->policytags()); - bool accepted = this->do_filtering(copy); - if (!accepted) { - delete copy; - continue; - } + // Policy EXPORT filtering was done here. + // It's moved to RouteDB<A>::do_filtering. + // This was done because EXPORT filter could possibly change route metric, + // and thus make it lower then RIP_INFINITY. + // Routes with cost > RIP_INFINTY would never come here, because they would be filtered out in RouteDB<A>::update_route + // - IMAR rpa.packet_add_route(copy->net(), copy->nexthop(), copy->cost(), copy->tag()); diff --git a/xorp/rip/output_updates.cc b/xorp/rip/output_updates.cc index aaf7148..92f8415 100644 --- a/xorp/rip/output_updates.cc +++ b/xorp/rip/output_updates.cc @@ -78,11 +78,12 @@ OutputUpdates<A>::output_packet() origin, r->tag(), r->policytags()); - bool accepted = this->do_filtering(copy); - if (!accepted) { - delete copy; - continue; - } + // Policy EXPORT filtering was done here. + // It's moved to RouteDB<A>::do_filtering. + // This was done because EXPORT filter could possibly change route metric, + // and thus make it lower then RIP_INFINITY + // Routes with cost > RIP_INFINTY would never come here, because they would be filtered out in RouteDB<A>::update_route + // - IMAR rpa.packet_add_route(copy->net(), copy->nexthop(), copy->cost(), r->tag()); added_routes.insert(r); diff --git a/xorp/rip/route_db.cc b/xorp/rip/route_db.cc index 7b116b4..9c4fd0a 100644 --- a/xorp/rip/route_db.cc +++ b/xorp/rip/route_db.cc @@ -185,7 +185,7 @@ RouteDB<A>::set_expiry_timer(Route* r) template <typename A> bool -RouteDB<A>::do_filtering(Route* r) +RouteDB<A>::do_filtering(Route* r, uint32_t& cost) { try { RIPVarRW<A> varrw(*r); @@ -199,19 +199,44 @@ RouteDB<A>::do_filtering(Route* r) bool accepted = _policy_filters.run_filter(filter::IMPORT, varrw); if (!accepted) - return false; + goto exit; - RIPVarRW<A> varrw2(*r); + do { + RIPVarRW<A> varrw2(*r); - debug_msg("[RIP] Running source match filter on route %s\n", + debug_msg("[RIP] Running source match filter on route %s\n", r->net().str().c_str()); - XLOG_TRACE(trace()._routes, + XLOG_TRACE(trace()._routes, "Running source match filter on route %s\n", r->net().str().c_str()); - _policy_filters.run_filter(filter::EXPORT_SOURCEMATCH, varrw2); + accepted = _policy_filters.run_filter(filter::EXPORT_SOURCEMATCH, varrw2); + } while(0); - return true; + if (!accepted) + goto exit; + + do { + RIPVarRW<A> varrw3(*r); + + debug_msg("[RIP] Running export filter on route: %s\n", + r->net().str().c_str()); + XLOG_TRACE(trace()._routes, + "Running export filter on route %s\n", + r->net().str().c_str()); + + accepted = _policy_filters.run_filter(filter::EXPORT, varrw3); + } while(0); + +exit: + cost = r->cost(); + if (r->cost() > RIP_INFINITY) { + r->set_cost(RIP_INFINITY); + cost = r->cost(); + accepted = false; + } + + return accepted; } catch(const PolicyException& e) { XLOG_FATAL("PolicyException: %s", e.str().c_str()); XLOG_UNFINISHED(); @@ -237,10 +262,6 @@ RouteDB<A>::update_route(const Net& net, return false; } - if (cost > RIP_INFINITY) { - cost = RIP_INFINITY; - } - // // Update steps, based on RFC2453 pp. 26-28 // @@ -252,11 +273,7 @@ RouteDB<A>::update_route(const Net& net, // Route does not appear in table so it needs to be // created if peer does not have an entry for it or - // resurrected if it does. But first this... - if (cost == RIP_INFINITY) { - // Don't bother adding a route for unreachable net - return false; - } + // resurrected if it does. // Create route if necessary r = o->find_route(net); @@ -270,10 +287,10 @@ RouteDB<A>::update_route(const Net& net, XLOG_ASSERT(ok); - bool accepted = do_filtering(r); + bool accepted = do_filtering(r, cost); r->set_filtered(!accepted); - if (!accepted) + if (!accepted || cost == RIP_INFINITY) return false; _uq->push_back(r); @@ -287,9 +304,12 @@ RouteDB<A>::update_route(const Net& net, XLOG_ASSERT(ok); // XXX: this is wrong - bool accepted = do_filtering(r); + bool accepted = do_filtering(r, cost); r->set_filtered(!accepted); + if (cost == RIP_INFINITY) + return false; + if (accepted) updated = true; } else { @@ -302,7 +322,7 @@ RouteDB<A>::update_route(const Net& net, cost, no_origin, tag, policytags); // XXX: lost origin - bool accepted = do_filtering(new_route); + bool accepted = do_filtering(new_route, cost); // XXX: this whole section of code is too entangled. if (r->origin() == o) { diff --git a/xorp/rip/route_db.hh b/xorp/rip/route_db.hh index 0c795dc..0686920 100644 --- a/xorp/rip/route_db.hh +++ b/xorp/rip/route_db.hh @@ -213,7 +213,7 @@ public: * @param r route to filter. * @return true if route was accepted, false otherwise. */ - bool do_filtering(Route* r); + bool do_filtering(Route* r, uint32_t& cost); Trace& trace() { return _trace; } -- 1.7.9.5 _______________________________________________ Xorp-hackers mailing list [email protected] http://mailman.ICSI.Berkeley.EDU/mailman/listinfo/xorp-hackers
