Skip to content

Commit

Permalink
rib: Fix ref-counting in RouteEntry class.
Browse files Browse the repository at this point in the history 
RouteEntry does ref-counting on the vif object, but it did not
implement copy constructors or operator=.  This means ref-counting
didn't work right.  Attempt to fix this by properly implementing
the copy constructors and operator= methods in RouteEntry and
child classes.

RIB is still a scary mess of pointers, but maybe this helps a bit.

Signed-off-by: Ben Greear <greearb@candelatech.com>
  • Loading branch information
@greearb
greearb committed Sep 18, 2012
1 parent 8c6d10c commit 313ca1e
Show file tree
Hide file tree
Showing 9 changed files with 105 additions and 34 deletions.
3 changes: 1 addition & 2 deletions xorp/libxorp/vif.cc
View file
Expand Up @@ -114,8 +114,7 @@ Vif::Vif(const string& vifname, const string& ifname)
//
// Vif copy constructor
//
Vif::Vif(const Vif& vif)
{
Vif::Vif(const Vif& vif) : BugCatcher(vif) {
_name = vif.name();
_ifname = vif.ifname();
set_pif_index(vif.pif_index());
Expand Down
3 changes: 2 additions & 1 deletion xorp/libxorp/vif.hh
View file
Expand Up @@ -29,6 +29,7 @@
#include "ipv6.hh"
#include "ipvx.hh"
#include "ipvxnet.hh"
#include "bug_catcher.hh"

/**
* @short Virtual interface address class.
Expand Down Expand Up @@ -211,7 +212,7 @@ public:
* entities such as the Discard or Unreachable interface, or a VLAN
* on a physical interface.
*/
class Vif {
class Vif : public BugCatcher {
public:
/**
* Constructor for a given virtual interface name.
Expand Down
2 changes: 1 addition & 1 deletion xorp/rib/redist_policy.hh
View file
Expand Up @@ -152,7 +152,7 @@ class IsOfProtocol : public RedistPolicy<A>
public:
IsOfProtocol(const Protocol& p) : _protocol(p) {}
bool accept(const IPRouteEntry<A>& ipr) const {
return ipr.protocol() == _protocol;
return *(ipr.protocol()) == _protocol;
}
private:
Protocol _protocol;
Expand Down
8 changes: 4 additions & 4 deletions xorp/rib/redist_xrl.cc
View file
Expand Up @@ -161,7 +161,7 @@ AddRoute<A>::AddRoute(RedistXrlOutput<A>* parent, const IPRouteEntry<A>& ipr)
_vifname(ipr.vif()->name()),
_metric(ipr.metric()),
_admin_distance(ipr.admin_distance()),
_protocol_origin(ipr.protocol().name())
_protocol_origin(ipr.protocol()->name())
{
}

Expand Down Expand Up @@ -247,7 +247,7 @@ DeleteRoute<A>::DeleteRoute(RedistXrlOutput<A>* parent,
_vifname(ipr.vif()->name()),
_metric(ipr.metric()),
_admin_distance(ipr.admin_distance()),
_protocol_origin(ipr.protocol().name())
_protocol_origin(ipr.protocol()->name())
{
}

Expand Down Expand Up @@ -1049,7 +1049,7 @@ RedistTransactionXrlOutput<A>::add_route(const IPRouteEntry<A>& ipr)
PROFILE(if (this->_profile.enabled(profile_route_rpc_in))
this->_profile.log(profile_route_rpc_in,
c_format("add %s %s %s %u",
ipr.protocol().name().c_str(),
ipr.protocol()->name().c_str(),
ipr.net().str().c_str(),
ipr.nexthop()->str().c_str(),
XORP_UINT_CAST(ipr.metric()))));
Expand Down Expand Up @@ -1080,7 +1080,7 @@ RedistTransactionXrlOutput<A>::delete_route(const IPRouteEntry<A>& ipr)
PROFILE(if (this->_profile.enabled(profile_route_rpc_in))
this->_profile.log(profile_route_rpc_in,
c_format("add %s %s",
ipr.protocol().name().c_str(),
ipr.protocol()->name().c_str(),
ipr.net().str().c_str())));

bool no_running_tasks = (this->_queued == 0);
Expand Down
4 changes: 2 additions & 2 deletions xorp/rib/rib.cc
View file
Expand Up @@ -908,7 +908,7 @@ RIB<A>::add_route(const string& tablename,
}

IPNextHop<A>* nexthop = find_or_create_peer_nexthop(nexthop_addr);
ot->add_route(new IPRouteEntry<A>(net, vif, nexthop, *protocol,
ot->add_route(new IPRouteEntry<A>(net, vif, nexthop, protocol,
metric, policytags));
flush();
return XORP_OK;
Expand Down Expand Up @@ -941,7 +941,7 @@ RIB<A>::add_route(const string& tablename,
//
// Add the route
//
ot->add_route(new IPRouteEntry<A>(net, vif, nexthop, *protocol, metric, policytags));
ot->add_route(new IPRouteEntry<A>(net, vif, nexthop, protocol, metric, policytags));

flush();
return XORP_OK;
Expand Down
11 changes: 6 additions & 5 deletions xorp/rib/rib.hh
View file
Expand Up @@ -845,7 +845,7 @@ public:
_usage_counter(0),
_is_deleted(false) { }

~RibVif() {}
virtual ~RibVif() {}

size_t copy_in(const Vif& from_vif) {
Vif* to_vif = this;
Expand All @@ -855,19 +855,20 @@ public:

void set_deleted(bool v) { _is_deleted = v; }

uint32_t usage_counter() const { return (_usage_counter); }
int32_t usage_counter() const { return (_usage_counter); }
void incr_usage_counter() { _usage_counter++; }
void decr_usage_counter() {
_usage_counter--;
assert(_usage_counter >= 0);
if (_is_deleted && (_usage_counter == 0)) {
if (_rib)
_rib->destroy_deleted_vif(this);
if (_rib)
_rib->destroy_deleted_vif(this);
}
}

private:
RIB<A>* _rib;
uint32_t _usage_counter;
int _usage_counter;
bool _is_deleted;
};

Expand Down
69 changes: 66 additions & 3 deletions xorp/rib/route.cc
View file
Expand Up @@ -31,7 +31,7 @@
#include "route.hh"

template<class A>
RouteEntry<A>::RouteEntry(RibVif<A>* vif, const Protocol& protocol,
RouteEntry<A>::RouteEntry(RibVif<A>* vif, Protocol* protocol,
uint32_t metric, const PolicyTags& policytags, const IPNet<A>& net)
: _vif(vif), _protocol(protocol),
_admin_distance(UNKNOWN_ADMIN_DISTANCE), _metric(metric),
Expand All @@ -42,7 +42,7 @@ RouteEntry<A>::RouteEntry(RibVif<A>* vif, const Protocol& protocol,
}

template<class A>
RouteEntry<A>::RouteEntry(RibVif<A>* vif, const Protocol& protocol,
RouteEntry<A>::RouteEntry(RibVif<A>* vif, Protocol* protocol,
uint32_t metric, const IPNet<A>& net)
: _vif(vif), _protocol(protocol),
_admin_distance(UNKNOWN_ADMIN_DISTANCE), _metric(metric), _net(net)
Expand All @@ -51,6 +51,35 @@ RouteEntry<A>::RouteEntry(RibVif<A>* vif, const Protocol& protocol,
_vif->incr_usage_counter();
}

template<class A>
RouteEntry<A>::RouteEntry(const RouteEntry& r) {
_vif = r._vif;
if (_vif)
_vif->incr_usage_counter();
_protocol = r._protocol;
_admin_distance = r._admin_distance;
_metric = r._metric;
_policytags = r._policytags;
_net = r._net;
}

template<class A>
RouteEntry<A>& RouteEntry<A>::operator=(const RouteEntry<A>& r) {
if (this == &r)
return *this;
if (_vif)
_vif->decr_usage_counter();
_vif = r._vif;
if (_vif)
_vif->incr_usage_counter();
_protocol = r._protocol;
_admin_distance = r._admin_distance;
_metric = r._metric;
_policytags = r._policytags;
_net = r._net;
return *this;
}

template<class A>
RouteEntry<A>::~RouteEntry()
{
Expand All @@ -70,10 +99,44 @@ IPRouteEntry<A>::str() const
return string("Dst: ") + dst + string(" Vif: ") + vif +
string(" NextHop: ") + _nexthop->str() +
string(" Metric: ") + c_format("%d", RouteEntry<A>::_metric) +
string(" Protocol: ") + RouteEntry<A>::_protocol.name() +
string(" Protocol: ") + RouteEntry<A>::_protocol->name() +
string(" PolicyTags: ") + RouteEntry<A>::_policytags.str();
}

template<class A>
IPRouteEntry<A>::IPRouteEntry(const IPRouteEntry<A>& r) : RouteEntry<A>(r) {
_nexthop = r._nexthop;
}

template<class A>
IPRouteEntry<A>& IPRouteEntry<A>::operator=(const IPRouteEntry<A>& r) {
if (this == &r)
return *this;
RouteEntry<A>::operator=(r);
_nexthop = r._nexthop;
return *this;
}


template<class A>
ResolvedIPRouteEntry<A>::ResolvedIPRouteEntry(const ResolvedIPRouteEntry<A>& r) : IPRouteEntry<A>(r) {
_igp_parent = r._igp_parent;
_egp_parent = r._egp_parent;
_backlink = r._backlink;
}

template<class A>
ResolvedIPRouteEntry<A>& ResolvedIPRouteEntry<A>::operator=(const ResolvedIPRouteEntry<A>& r) {
if (this == &r)
return *this;
IPRouteEntry<A>::operator=(r);
_igp_parent = r._igp_parent;
_egp_parent = r._egp_parent;
_backlink = r._backlink;
return *this;
}


template class IPRouteEntry<IPv4>;
template class IPRouteEntry<IPv6>;

37 changes: 22 additions & 15 deletions xorp/rib/route.hh
View file
Expand Up @@ -18,14 +18,11 @@
// XORP Inc, 2953 Bunker Hill Lane, Suite 204, Santa Clara, CA 95054, USA;
// http://xorp.net

// $XORP: xorp/rib/route.hh,v 1.29 2008/10/02 21:58:12 bms Exp $

#ifndef __RIB_ROUTE_HH__
#define __RIB_ROUTE_HH__




#include "libxorp/xorp.h"
#include "libxorp/ipv4net.hh"
#include "libxorp/ipv6net.hh"
Expand Down Expand Up @@ -58,12 +55,16 @@ public:
* @param protocol the routing protocol that originated this route.
* @param metric the routing protocol metric for this route.
*/
RouteEntry(RibVif<A>* vif, const Protocol& protocol,
RouteEntry(RibVif<A>* vif, Protocol* protocol,
uint32_t metric, const PolicyTags& policytags, const IPNet<A>& net);

RouteEntry(RibVif<A>* vif, const Protocol& protocol,
RouteEntry(RibVif<A>* vif, Protocol* protocol,
uint32_t metric, const IPNet<A>& net);

RouteEntry(const RouteEntry<A>& r);

RouteEntry& operator=(const RouteEntry<A>& r);

/**
* Destructor
*/
Expand Down Expand Up @@ -109,7 +110,7 @@ public:
* @return the routing protocol that originated this route.
* @see Protocol.
*/
const Protocol& protocol() const { return _protocol; }
Protocol* protocol() const { return _protocol; }

/**
* Display the route for debugging purposes.
Expand Down Expand Up @@ -145,14 +146,14 @@ public:
const PolicyTags& policytags() const { return _policytags; }

protected:
RibVif<A>* _vif;
RibVif<A>* _vif;

const Protocol& _protocol; // The routing protocol that instantiated this route
Protocol* _protocol; // The routing protocol that instantiated this route

uint16_t _admin_distance; // Lower is better
uint32_t _metric; // Lower is better
PolicyTags _policytags; // Tags used for policy route redistribution
const IPNet<A> _net; // The route entry's subnet address
uint16_t _admin_distance; // Lower is better
uint32_t _metric; // Lower is better
PolicyTags _policytags; // Tags used for policy route redistribution
IPNet<A> _net; // The route entry's subnet address
};

/**
Expand All @@ -177,7 +178,7 @@ public:
* @param metric the routing protocol metric for this route.
*/
IPRouteEntry(const IPNet<A>& net, RibVif<A>* vif, IPNextHop<A>* nexthop,
const Protocol& protocol, uint32_t metric)
Protocol* protocol, uint32_t metric)
: RouteEntry<A>(vif, protocol, metric, net), _nexthop(nexthop) {}

/**
Expand All @@ -193,10 +194,13 @@ public:
* @param policytags the policy-tags for this route.
*/
IPRouteEntry(const IPNet<A>& net, RibVif<A>* vif, IPNextHop<A>* nexthop,
const Protocol& protocol, uint32_t metric,
Protocol* protocol, uint32_t metric,
const PolicyTags& policytags)
: RouteEntry<A>(vif, protocol, metric, policytags, net), _nexthop(nexthop) {}

IPRouteEntry(const IPRouteEntry<A>& r);
IPRouteEntry& operator=(const IPRouteEntry<A>& r);

/**
* Destructor for Routing Table Entry
*/
Expand Down Expand Up @@ -274,13 +278,16 @@ public:
* @param egp_parent the orginal route entry with a non-local nexthop.
*/
ResolvedIPRouteEntry(const IPNet<A>& net, RibVif<A>* vif, IPNextHop<A>* nexthop,
const Protocol& protocol, uint32_t metric,
Protocol* protocol, uint32_t metric,
const IPRouteEntry<A>* igp_parent,
const IPRouteEntry<A>* egp_parent)
: IPRouteEntry<A>(net, vif, nexthop, protocol, metric, PolicyTags()),
_igp_parent(igp_parent),
_egp_parent(egp_parent) { }

ResolvedIPRouteEntry(const ResolvedIPRouteEntry<A>& r);
ResolvedIPRouteEntry& operator=(const ResolvedIPRouteEntry<A>& r);

/**
* Get the igp_parent.
*
Expand Down
2 changes: 1 addition & 1 deletion xorp/rib/rt_tab_register.cc
View file
Expand Up @@ -509,7 +509,7 @@ RegisterTable<A>::notify_route_changed(
} else {
uint32_t metric = changed_route.metric();
uint32_t admin_distance = changed_route.admin_distance();
const string& protocol_origin = changed_route.protocol().name();
const string& protocol_origin = changed_route.protocol()->name();
list<string>::const_iterator iter;
for (iter = module_names.begin(); iter != module_names.end(); ++iter) {
_register_server.send_route_changed(
Expand Down

0 comments on commit 313ca1e

Please sign in to comment.