[Xorp-cvs] SF.net SVN: xorp:[11694] trunk/xorp/libxipc
bms_fbsd at users.sourceforge.net
bms_fbsd at users.sourceforge.net
Fri Apr 16 16:36:02 PDT 2010
Revision: 11694
http://xorp.svn.sourceforge.net/xorp/?rev=11694&view=rev
Author: bms_fbsd
Date: 2010-04-16 23:36:01 +0000 (Fri, 16 Apr 2010)
Log Message:
-----------
Fix a critical race condition in XRL method lookup.
Introduce the use of Boost's weak_ptr<T>/shared_ptr<T> for managing
the lifecycle of XrlPFSender objects, which represent open XRL transports.
class Xrl now holds a boost::weak_ptr<XrlPFSender> for its cached, resolved
sender object. This fixes the issue where there was no way of
cleanly invalidating an existing XrlPFSender* transport pointer.
The method in XrlRouter which tries to find an existing RPC endpoint for
outgoing XRLs, has been renamed lookup_sender(), so as not to clash with
the hooks for batch XRL support (which are not currently used).
As most of the XORP code base is not yet thread or Boost aware, we
continue to use XrlPFSender* in most situations.
This change is cumulative with the auto_ptr<T> fix used in the XIF
client-side RPC stubs themselves; this is necessary to ensure the
weak_ptr<T> is private to each XIF client stub instantiation.
This change passes 'scons check' on FreeBSD 8-STABLE/amd64.
Reported by: Ben Greear, Li Zhao
Modified Paths:
--------------
trunk/xorp/libxipc/xrl.cc
trunk/xorp/libxipc/xrl.hh
trunk/xorp/libxipc/xrl_pf.cc
trunk/xorp/libxipc/xrl_pf.hh
trunk/xorp/libxipc/xrl_pf_factory.cc
trunk/xorp/libxipc/xrl_pf_factory.hh
trunk/xorp/libxipc/xrl_pf_stcp.cc
trunk/xorp/libxipc/xrl_router.cc
trunk/xorp/libxipc/xrl_router.hh
Modified: trunk/xorp/libxipc/xrl.cc
===================================================================
--- trunk/xorp/libxipc/xrl.cc 2010-03-25 13:12:31 UTC (rev 11693)
+++ trunk/xorp/libxipc/xrl.cc 2010-04-16 23:36:01 UTC (rev 11694)
@@ -76,7 +76,7 @@
const XrlArgs& args)
: _protocol(protocol), _target(protocol_target), _command(command),
_args(args), _sna_atom(NULL), _packed_bytes(0), _argp(&_args),
- _to_finder(-1), _resolved(false), _resolved_sender(NULL)
+ _to_finder(-1), _resolved(false)
{
}
@@ -85,7 +85,7 @@
const XrlArgs& args)
: _protocol(_finder_protocol), _target(target), _command(command),
_args(args), _sna_atom(NULL), _packed_bytes(0), _argp(&_args),
- _to_finder(-1), _resolved(false), _resolved_sender(NULL)
+ _to_finder(-1), _resolved(false)
{
}
@@ -94,7 +94,7 @@
const string& command)
: _protocol(protocol), _target(protocol_target), _command(command),
_sna_atom(NULL), _packed_bytes(0), _argp(&_args), _to_finder(-1),
- _resolved(false), _resolved_sender(NULL)
+ _resolved(false)
{
}
@@ -102,7 +102,7 @@
const string& command)
: _protocol(_finder_protocol), _target(target), _command(command),
_sna_atom(NULL), _packed_bytes(0), _argp(&_args), _to_finder(-1),
- _resolved(false), _resolved_sender(NULL)
+ _resolved(false)
{
}
@@ -110,13 +110,13 @@
const char* command)
: _protocol(_finder_protocol), _target(target), _command(command),
_sna_atom(NULL), _packed_bytes(0), _argp(&_args), _to_finder(-1),
- _resolved(false), _resolved_sender(NULL)
+ _resolved(false)
{
}
Xrl::Xrl(const char* c_str) throw (InvalidString)
: _sna_atom(NULL), _packed_bytes(0), _argp(&_args),
- _to_finder(-1), _resolved(false), _resolved_sender(NULL)
+ _to_finder(-1), _resolved(false)
{
if (0 == c_str)
xorp_throw0(InvalidString);
@@ -136,7 +136,7 @@
Xrl::Xrl()
: _sna_atom(0), _packed_bytes(0), _argp(&_args), _to_finder(-1),
- _resolved(false), _resolved_sender(NULL)
+ _resolved(false)
{
}
@@ -335,8 +335,9 @@
_packed_bytes = 0;
_to_finder = -1;
_resolved = false;
- _resolved_sender = NULL;
+ _resolved_sender.reset();
+
delete _sna_atom;
_sna_atom = NULL;
}
Modified: trunk/xorp/libxipc/xrl.hh
===================================================================
--- trunk/xorp/libxipc/xrl.hh 2010-03-25 13:12:31 UTC (rev 11693)
+++ trunk/xorp/libxipc/xrl.hh 2010-04-16 23:36:01 UTC (rev 11694)
@@ -26,11 +26,15 @@
#include <string>
+#include <boost/weak_ptr.hpp>
+
#include "libxorp/exceptions.hh"
#include "xrl_atom.hh"
#include "xrl_args.hh"
#include "xrl_tokens.hh"
+using boost::weak_ptr;
+
class XrlPFSender;
/**
@@ -169,9 +173,14 @@
bool resolved() const { return _resolved; }
void set_resolved(bool r) const { _resolved = r; }
- XrlPFSender *resolved_sender() const { return _resolved_sender; }
- void set_resolved_sender(XrlPFSender *s) const { _resolved_sender = s; }
+ weak_ptr<XrlPFSender> resolved_sender() const {
+ return _resolved_sender;
+ }
+ void set_resolved_sender(weak_ptr<XrlPFSender> s) const {
+ _resolved_sender = s;
+ }
+
void set_target(const char* target);
private:
@@ -194,7 +203,7 @@
mutable XrlArgs* _argp; // XXX shouldn't be mutable
mutable int _to_finder;
mutable bool _resolved; // XXX ditto
- mutable XrlPFSender* _resolved_sender; // XXX ditto
+ mutable weak_ptr<XrlPFSender> _resolved_sender; // XXX ditto
static const string _finder_protocol;
};
Modified: trunk/xorp/libxipc/xrl_pf.cc
===================================================================
--- trunk/xorp/libxipc/xrl_pf.cc 2010-03-25 13:12:31 UTC (rev 11693)
+++ trunk/xorp/libxipc/xrl_pf.cc 2010-04-16 23:36:01 UTC (rev 11694)
@@ -55,4 +55,5 @@
XrlPFSender::~XrlPFSender()
{
+ // XXX put a debug_msg() here; we are now deleted through shared_ptr.
}
Modified: trunk/xorp/libxipc/xrl_pf.hh
===================================================================
--- trunk/xorp/libxipc/xrl_pf.hh 2010-03-25 13:12:31 UTC (rev 11693)
+++ trunk/xorp/libxipc/xrl_pf.hh 2010-04-16 23:36:01 UTC (rev 11694)
@@ -105,7 +105,15 @@
virtual bool sends_pending() const = 0;
virtual const char* protocol() const = 0;
+
+ /**
+ * Determine if the underlying transport is still open.
+ *
+ * @return true if the transport is alive.
+ */
virtual bool alive() const = 0;
+
+ // XXX Unfinished support for XRL batching.
virtual void batch_start() {}
virtual void batch_stop() {}
Modified: trunk/xorp/libxipc/xrl_pf_factory.cc
===================================================================
--- trunk/xorp/libxipc/xrl_pf_factory.cc 2010-03-25 13:12:31 UTC (rev 11693)
+++ trunk/xorp/libxipc/xrl_pf_factory.cc 2010-04-16 23:36:01 UTC (rev 11694)
@@ -33,12 +33,14 @@
#include "xrl_pf_stcp.hh"
#include "xrl_pf_unix.hh"
+//#include <boost/make_shared.hpp>
+//using boost::make_shared;
// STCP senders are a special case. Constructing an STCP sender has
// real cost, unlike InProc and SUDP, so we maintain a cache of
// STCP senders with one per sender destination address.
-XrlPFSender*
+shared_ptr<XrlPFSender>
XrlPFSenderFactory::create_sender(EventLoop& eventloop,
const char* protocol,
const char* address)
@@ -47,18 +49,22 @@
protocol, address);
try {
if (strcmp(XrlPFSTCPSender::protocol_name(), protocol) == 0) {
- return new XrlPFSTCPSender(eventloop, address);
+ // XXX boost::make_shared() candidate.
+ return shared_ptr<XrlPFSender>(
+ new XrlPFSTCPSender(eventloop, address));
}
if (strcmp(XrlPFUNIXSender::protocol_name(), protocol) == 0) {
- return new XrlPFUNIXSender(eventloop, address);
+ // XXX boost::make_shared() candidate.
+ return shared_ptr<XrlPFSender>(
+ new XrlPFUNIXSender(eventloop, address));
}
} catch (XorpException& e) {
XLOG_ERROR("XrlPFSenderFactory::create failed: %s\n", e.str().c_str());
}
- return 0;
+ return shared_ptr<XrlPFSender>();
}
-XrlPFSender*
+shared_ptr<XrlPFSender>
XrlPFSenderFactory::create_sender(EventLoop& eventloop,
const char* protocol_colon_address)
{
@@ -66,7 +72,7 @@
if (colon == 0) {
debug_msg("No colon in supposedly colon separated <protocol><address>"
"combination\n\t\"%s\".\n", protocol_colon_address);
- return 0;
+ return shared_ptr<XrlPFSender>();
}
string protocol(protocol_colon_address, colon - protocol_colon_address);
@@ -74,15 +80,6 @@
}
void
-XrlPFSenderFactory::destroy_sender(XrlPFSender* s)
-{
- debug_msg("destroying sender pf = \"%s\", addr = \"%s\"\n",
- s->protocol(), s->address().c_str());
-
- delete s;
-}
-
-void
XrlPFSenderFactory::startup()
{
}
Modified: trunk/xorp/libxipc/xrl_pf_factory.hh
===================================================================
--- trunk/xorp/libxipc/xrl_pf_factory.hh 2010-03-25 13:12:31 UTC (rev 11693)
+++ trunk/xorp/libxipc/xrl_pf_factory.hh 2010-04-16 23:36:01 UTC (rev 11694)
@@ -27,19 +27,20 @@
#include "xrl_error.hh"
#include "xrl_pf.hh"
+#include <boost/shared_ptr.hpp>
+
+using boost::shared_ptr;
+
class XrlPFSenderFactory {
public:
static void startup();
static void shutdown();
- static XrlPFSender* create_sender(EventLoop& eventloop,
- const char* proto_colon_addr);
+ static shared_ptr<XrlPFSender> create_sender(
+ EventLoop& eventloop, const char* proto_colon_addr);
- static XrlPFSender* create_sender(EventLoop& e,
- const char* protocol,
- const char* address);
-
- static void destroy_sender(XrlPFSender* s);
+ static shared_ptr<XrlPFSender> create_sender(EventLoop& e,
+ const char* protocol, const char* address);
};
#endif // __LIBXIPC_XRL_PF_FACTORY_HH__
Modified: trunk/xorp/libxipc/xrl_pf_stcp.cc
===================================================================
--- trunk/xorp/libxipc/xrl_pf_stcp.cc 2010-03-25 13:12:31 UTC (rev 11693)
+++ trunk/xorp/libxipc/xrl_pf_stcp.cc 2010-04-16 23:36:01 UTC (rev 11694)
@@ -1083,12 +1083,17 @@
void
XrlPFSTCPSender::batch_start()
{
+#if 0
_batching = true;
+#else
+ XLOG_UNREACHABLE();
+#endif
}
void
XrlPFSTCPSender::batch_stop()
{
+#if 0
_batching = false;
// If we aint got no requests, we may not be able to signal to the receiver
@@ -1100,4 +1105,7 @@
if (_writer->running() == false)
_writer->start();
+#else
+ XLOG_UNREACHABLE();
+#endif
}
Modified: trunk/xorp/libxipc/xrl_router.cc
===================================================================
--- trunk/xorp/libxipc/xrl_router.cc 2010-03-25 13:12:31 UTC (rev 11693)
+++ trunk/xorp/libxipc/xrl_router.cc 2010-04-16 23:36:01 UTC (rev 11694)
@@ -38,6 +38,10 @@
#include "sockutil.hh"
+// XXX should be included nested above.
+//#include <boost/shared_ptr.hpp>
+//#include <boost/weak_ptr.hpp>
+
//
// Enable this macro to enable Xrl callback checker that checks each Xrl
// callback is dispatched just once.
@@ -273,10 +277,8 @@
_fc->detach_observer(this);
_fac->set_enabled(false);
- while (_senders.empty() == false) {
- XrlPFSenderFactory::destroy_sender(_senders.front());
+ while (_senders.empty() == false)
_senders.pop_front();
- }
while (_dsl.empty() == false) {
delete _dsl.front();
@@ -389,7 +391,8 @@
bool direct_call)
{
try {
- XrlPFSender* s = get_sender(xrl, const_cast<FinderDBEntry*>(dbe));
+ shared_ptr<XrlPFSender> s =
+ lookup_sender(xrl, const_cast<FinderDBEntry*>(dbe));
if (s == 0) {
// Notify Finder client that result was bad.
_fc->uncache_result(dbe);
@@ -400,10 +403,11 @@
const Xrl& x = dbe->xrls().front();
x.set_args(xrl);
- if (s) {
+ if (s != 0) {
trace_xrl("Sending ", x);
return s->send(x, direct_call,
- callback(this, &XrlRouter::send_callback, s, cb));
+ callback(this, &XrlRouter::send_callback,
+ s.get(), cb));
}
cb->dispatch(XrlError(SEND_FAILED, "sender not instantiated"), 0);
} catch (const InvalidString&) {
@@ -412,31 +416,47 @@
return false;
}
-XrlPFSender*
-XrlRouter::get_sender(const Xrl& xrl, FinderDBEntry* dbe)
+// XXX Return uninitialized shared_ptr, or locked shared_ptr,
+// on desired XrlPFSender.
+shared_ptr<XrlPFSender>
+XrlRouter::lookup_sender(const Xrl& xrl, FinderDBEntry* dbe)
{
const Xrl& x = dbe->xrls().front();
- XrlPFSender* s = NULL;
+ shared_ptr<XrlPFSender> s;
- // Use the cache pointer to the sender.
+ // Try to use the cached pointer to the sender.
if (xrl.resolved()) {
- s = xrl.resolved_sender();
+ weak_ptr<XrlPFSender> w = xrl.resolved_sender();
- if (s->alive())
+ // Check if cached weak_ptr to sender is still valid; we may
+ // have lost a race on the XRLDB.
+ // Check if transport layer is still up for this sender.
+ // If good to go, return shared pointer for which we
+ // still hold the lock.
+ s = w.lock();
+ if (s.get() != 0 && s->alive())
return s;
+ // We lost the race.
+
+ // XXX Sanity check that this sender's protocol and endpoint
+ // addresses are the same as they were when the Xrl was
+ // instantiated.
XLOG_ASSERT(s->protocol() == x.protocol());
XLOG_ASSERT(s->address() == x.target());
xrl.set_resolved(false);
- xrl.set_resolved_sender(NULL);
+ xrl.set_resolved_sender(weak_ptr<XrlPFSender>());
}
// Find a new sender.
- for (list<XrlPFSender*>::iterator i = _senders.begin();
+ for (list< shared_ptr<XrlPFSender> >::iterator i = _senders.begin();
i != _senders.end(); ++i) {
s = *i;
+ // This XrlRouter holds the reference, so the pointer should be valid.
+ XLOG_ASSERT(s.get() != 0);
+
if (s->protocol() != x.protocol() || s->address() != x.target())
continue;
@@ -448,13 +468,13 @@
XLOG_INFO("Sender died (protocol = \"%s\", address = \"%s\")",
s->protocol(), s->address().c_str());
- XrlPFSenderFactory::destroy_sender(s);
+
_senders.erase(i);
_senders2.erase(xrl.target());
break;
}
- s = NULL;
+ s.reset();
// create sender
while (dbe->xrls().size()) {
@@ -462,7 +482,7 @@
s = XrlPFSenderFactory::create_sender(_e, x.protocol().c_str(),
x.target().c_str());
- if (s)
+ if (s.get() != 0)
break;
XLOG_ERROR("Could not create XrlPFSender for protocol = \"%s\" "
@@ -471,15 +491,18 @@
dbe->pop_front();
}
- if (!s)
- return NULL;
+ // Unable to instantiate.
+ if (s == 0)
+ return shared_ptr<XrlPFSender>();
+
+ // New sender instantiated, take lock and record state.
const Xrl& front = dbe->xrls().front();
XLOG_ASSERT(s->protocol() == front.protocol());
XLOG_ASSERT(s->address() == front.target());
_senders.push_back(s);
- _senders2[xrl.target()] = s;
+ _senders2[xrl.target()] = s.get();
// Don't do this here as it is set in the Xrl that is stored with
// the finder client, so is not used. But if the connection needs
@@ -506,7 +529,7 @@
if (e == XrlError::OKAY()) {
const Xrl& xrl = ds->xrl();
xrl.set_resolved(false);
- xrl.set_resolved_sender(NULL);
+ xrl.set_resolved_sender(weak_ptr<XrlPFSender>());
if (send_resolved(xrl, dbe, ds->cb(), false) == false) {
// We tried to force sender to send xrl and it declined the
// opportunity. This should only happen when it's out of buffer
@@ -710,25 +733,44 @@
debug_msg("Finder target ready event: \"%s\"\n", tgt_name.c_str());
}
+//
+// XXX This is unfinished code related to a batch XRL implementation.
+// It is unfortunately incompatible, straight off, with using refcounts to
+// manage XrlPFSender's lifecycle, so it needs to be revisited later by
+// an interested party.
+// Also, the overloading of the get_sender() method name is potentially
+// confusing. --bms
+
void
XrlRouter::batch_start(const string& target)
{
+#if 0
XrlPFSender& sender = get_sender(target);
sender.batch_start();
+#else
+ XLOG_UNREACHABLE();
+ UNUSED(target);
+#endif
}
void
XrlRouter::batch_stop(const string& target)
{
+#if 0
XrlPFSender& sender = get_sender(target);
sender.batch_stop();
+#else
+ XLOG_UNREACHABLE();
+ UNUSED(target);
+#endif
}
XrlPFSender&
XrlRouter::get_sender(const string& target)
{
+#if 0
XrlPFSender* s = 0;
SENDERS::iterator i = _senders2.find(target);
@@ -737,6 +779,12 @@
s = i->second;
return *s;
+#else
+ XLOG_UNREACHABLE();
+ XrlPFSender* s = 0;
+ return *s;
+ UNUSED(target);
+#endif
}
Modified: trunk/xorp/libxipc/xrl_router.hh
===================================================================
--- trunk/xorp/libxipc/xrl_router.hh 2010-03-25 13:12:31 UTC (rev 11693)
+++ trunk/xorp/libxipc/xrl_router.hh 2010-04-16 23:36:01 UTC (rev 11694)
@@ -34,7 +34,10 @@
#include "finder_constants.hh"
#include "finder_client_observer.hh"
+#include <boost/shared_ptr.hpp>
+using boost::shared_ptr;
+
class DispatchState;
class FinderClient;
@@ -190,6 +193,7 @@
/**
* Send callback (fast path).
+ * FIXME: use smart ptr.
*/
void send_callback(const XrlError& e,
XrlArgs* reply,
@@ -211,8 +215,9 @@
uint16_t finder_port);
private:
+ // XXX
XrlPFSender& get_sender(const string& target);
- XrlPFSender* get_sender(const Xrl& xrl, FinderDBEntry *dbe);
+ shared_ptr<XrlPFSender> lookup_sender(const Xrl& xrl, FinderDBEntry *dbe);
protected:
EventLoop& _e;
@@ -224,10 +229,12 @@
list<XrlPFListener*> _listeners; // listeners
list<XrlRouterDispatchState*> _dsl; // dispatch state
- list<XrlPFSender*> _senders; // active senders
+ list< shared_ptr<XrlPFSender> > _senders; // active senders
static uint32_t _icnt; // instance count
+ // XXX the following are mostly only used by the incomplete
+ // batch support. Stick to using unchecked pointers for now --bms
private:
typedef map<string, XI*> XIM;
typedef map<string, XrlPFSender*> SENDERS;
This was sent by the SourceForge.net collaborative development platform, the world's largest Open Source development site.
More information about the Xorp-cvs
mailing list