Revert un-necessary Plugin complications. Better solution for plugin extension points coming up...

git-svn-id: https://svn.apache.org/repos/asf/incubator/qpid/trunk/qpid@674915 13f79535-47bb-0310-9956-ffa450edef68

9 files changed, 116 insertions, 173 deletions

diff --git a/cpp/src/qpid/Plugin.cpp b/cpp/src/qpid/Plugin.cpp
index 449eaf937d..733d134334 100644
--- a/cpp/src/qpid/Plugin.cpp
+++ b/cpp/src/qpid/Plugin.cpp
@@ -19,53 +19,36 @@
  */
 
 #include "Plugin.h"
-#include <qpid/shared_ptr.h>
-#include <qpid/Options.h>
-#include <qpid/sys/Mutex.h>
+#include "qpid/Options.h"
 
 namespace qpid {
 
-Plugin::Target::~Target() {}
-
-void Plugin::Target::createPlugins() {
-    typedef std::vector<Plugin::Factory*>::const_iterator Iter; 
-    for (Iter i = Factory::getList().begin(); i != Factory::getList().end(); ++i) {
-        boost::shared_ptr<Plugin> plugin = (**i).create(*this);
-        if (plugin)
-            plugins.push_back(plugin);
-    }
+namespace {
+Plugin::Plugins& thePlugins() {
+    // This is a single threaded singleton implementation so
+    // it is important to be sure that the first use of this
+    // singleton is when the program is still single threaded
+    static Plugin::Plugins plugins;
+    return plugins;
 }
-
-void Plugin::Target::initializePlugins() {
-    typedef std::vector<boost::shared_ptr<Plugin> >::iterator Iter; 
-    for (Iter i = plugins.begin(); i != plugins.end(); ++i) 
-        (**i).initialize(*this);
 }
 
-void Plugin::Target::releasePlugins() { plugins.clear(); }
-
-Plugin::Factory::~Factory() {}
-
-Plugin::Factory::Factory() {
-    const_cast<std::vector<Plugin::Factory*>& >(getList()).push_back(this);
+Plugin::Plugin() {
+    // Register myself.
+    thePlugins().push_back(this);
 }
 
-static sys::PODMutex PluginFactoryGetListLock;
+Plugin::~Plugin() {}
 
-const std::vector<Plugin::Factory*>& Plugin::Factory::getList() {
-    sys::PODMutex::ScopedLock l(PluginFactoryGetListLock);
-    static std::vector<Plugin::Factory*> list;
-    return list;
-}
+Options*  Plugin::getOptions() { return 0; }
+
+const Plugin::Plugins& Plugin::getPlugins() { return thePlugins(); }
 
-void Plugin::Factory::addOptions(Options& opts) {
-    typedef std::vector<Plugin::Factory*>::const_iterator Iter; 
-    for (Iter i = Factory::getList().begin(); i != Factory::getList().end(); ++i) {
-        Options* opt=(**i).getOptions();
-        if (opt) opts.add(*opt);
+void Plugin::addOptions(Options& opts) {
+    for (Plugins::const_iterator i = getPlugins().begin(); i != getPlugins().end(); ++i) {
+        if ((*i)->getOptions())
+            opts.add(*(*i)->getOptions());
     }
 }
 
-Plugin::~Plugin() {}
-
 } // namespace qpid
diff --git a/cpp/src/qpid/Plugin.h b/cpp/src/qpid/Plugin.h
index 5b0bb0c2c1..3ead770129 100644
--- a/cpp/src/qpid/Plugin.h
+++ b/cpp/src/qpid/Plugin.h
@@ -21,123 +21,78 @@
  *
  */
 
-#include <boost/shared_ptr.hpp>
+#include "qpid/shared_ptr.h"
+#include <boost/noncopyable.hpp>
 #include <vector>
+#include <boost/function.hpp>
+
 
 /**@file Generic plug-in framework. */
 
 namespace qpid {
-
 class Options;
 
 /**
  * Plug-in base class.
- *
- * A Plugin is created by a Plugin::Factory and attached to a Plugin::Target.
  */
-class Plugin {
+class Plugin : boost::noncopyable
+{
   public:
     /**
-     * Base class for target objects that receive plug-ins.
+     * Base interface for targets that receive plug-ins.
+     *
+     * The Broker is a plug-in target, there might be others
+     * in future.
      */
-    class Target {
-      public:
-        virtual ~Target();
-
-      protected:
-        /** For each Factory create a plugin attached to this */
-        void createPlugins();
-
-        /** Initialize all attached plugins */
-        void initializePlugins();
-        
-        /** Release the attached plugins. Done automatically in destructor. */
-        void releasePlugins();
-
-      private:
-        std::vector<boost::shared_ptr<Plugin> > plugins;
-    };
-
-    /** Base class for a factory to create Plugins. */
-    class Factory {
-      public:
-        /**
-         * Constructor registers the factory so it will be included in getList().
-         *
-         * Derive your Plugin Factory class from Factory and create a
-         * single global instance in your plug-in library. Loading the
-         * library will automatically register your factory.
-         */
-        Factory();
-        
-        virtual ~Factory();
-
-        /** Get the list of Factories */
-        static const std::vector<Factory*>& getList();
-
-        /** For each Factory in Factory::getList() add options to opts. */
-        static void addOptions(Options& opts);
-
-        /**
-         * Configuration options for this factory.
-         * Then will be updated during option parsing by the host program.
-         * 
-         * @return An options group or 0 for no options.
-         */
-        virtual Options* getOptions() = 0;
-        
-        /**
-         * Create a Plugin for target.
-         * Uses option values set by getOptions().
-         * Target may not be fully initialized at this point.
-         * Actions that require a fully-initialized target should be
-         * done in initialize().
-         * 
-         * @returns 0 if the target type is not compatible with this Factory.
-         */
-        virtual boost::shared_ptr<Plugin> create(Target& target) = 0;
-    };
+    struct Target { virtual ~Target() {} };
 
+    typedef std::vector<Plugin*> Plugins;
+    
     /**
-     * Template factory implementation, checks target type is correct. 
+     * Construct registers the plug-in to appear in getPlugins().
+     * 
+     * A concrete Plugin is instantiated as a global or static
+     * member variable in a library so it is registered during static
+     * initialization when the library is loaded.
      */
-    template <class TargetType> class FactoryT : public Factory {
-        Options* getOptions() { return 0; }
-
-        virtual boost::shared_ptr<Plugin> createT(TargetType& target) = 0;
+    Plugin();
+    
+    virtual ~Plugin();
 
-        boost::shared_ptr<Plugin> create(Target& target) {
-            TargetType* tt = dynamic_cast<TargetType*>(&target);
-            if (tt)
-                return createT(*tt);
-            else
-                return boost::shared_ptr<Plugin>();
-        }
-    };
+    /**
+     * Configuration options for the plugin.
+     * Then will be updated during option parsing by the host program.
+     * 
+     * @return An options group or 0 for no options. Default returns 0.
+     * Plugin retains ownership of return value.
+     */
+    virtual Options* getOptions();
 
-    // Plugin functions.
+    /**
+     * Initialize Plugin functionality on a Target.
+     * Plugins should ignore targets they don't recognize.
+     *
+     * Called before the target itself is initialized.
+     */
+    virtual void earlyInitialize(Target&) = 0;
 
-    virtual ~Plugin();
-    
     /**
-     * Initialize the Plugin. 
+     * Initialize Plugin functionality on a Target.
+     * Plugins should ignore targets they don't recognize.
+     *
      * Called after the target is fully initialized.
      */
     virtual void initialize(Target&) = 0;
-};
 
-/** Template plugin factory */
-template <class TargetType> class PluginT : public Plugin {
-
-    virtual void initializeT(TargetType&) = 0;
+    /** List of registered Plugin objects.
+     * Caller must not delete plugin pointers.
+     */
+    static const Plugins& getPlugins();
 
-    void initialize(Plugin::Target& target) {
-        TargetType* tt = dynamic_cast<TargetType*>(&target);
-        assert(tt);
-        initializeT(*tt);
-    }
+    /** For each registered plugin, add plugin.getOptions() to opts. */
+    static void addOptions(Options& opts);
 };
-
+ 
 } // namespace qpid
 
 #endif  /*!QPID_PLUGIN_H*/
diff --git a/cpp/src/qpid/broker/Broker.cpp b/cpp/src/qpid/broker/Broker.cpp
index e7b8460f62..c391d25788 100644
--- a/cpp/src/qpid/broker/Broker.cpp
+++ b/cpp/src/qpid/broker/Broker.cpp
@@ -166,7 +166,13 @@ Broker::Broker(const Broker::Options& conf) :
         links.setParent     (vhost);
     }
 
-    createPlugins();
+    // Early-Initialize plugins
+    const Plugin::Plugins& plugins=Plugin::getPlugins();
+    for (Plugin::Plugins::const_iterator i = plugins.begin();
+         i != plugins.end();
+         i++)
+        (*i)->earlyInitialize(*this);
+
     // If no plugin store module registered itself, set up the null store.
     if (store == 0)
         setStore (new NullMessageStore (false));
@@ -217,7 +223,11 @@ Broker::Broker(const Broker::Options& conf) :
 #endif
     }
 
-    initializePlugins();
+    // Initialize plugins
+    for (Plugin::Plugins::const_iterator i = plugins.begin();
+         i != plugins.end();
+         i++)
+        (*i)->initialize(*this);
 }
 
 void Broker::declareStandardExchange(const std::string& name, const std::string& type)
diff --git a/cpp/src/qpid/cluster/ClusterPlugin.cpp b/cpp/src/qpid/cluster/ClusterPlugin.cpp
index 10695496bc..6d3dca84be 100644
--- a/cpp/src/qpid/cluster/ClusterPlugin.cpp
+++ b/cpp/src/qpid/cluster/ClusterPlugin.cpp
@@ -33,56 +33,48 @@ namespace qpid {
 namespace cluster {
 
 using namespace std;
-using broker::Broker;
 
-
-// Note we update the values in a separate struct.
-// This is to work around boost::program_options differences,
-// older versions took a reference to the options, newer
-// ones take a copy (or require a shared_ptr)
-//
 struct ClusterOptions : public Options {
-    std::string name;
-    std::string url;
+    string name;
+    string url;
 
     ClusterOptions() : Options("Cluster Options") {
         addOptions()
-            ("cluster-name", optValue(name,""), "Cluster identifier")
+            ("cluster-name", optValue(name, "NAME"), "Name of cluster to join")
             ("cluster-url", optValue(url,"URL"),
              "URL of this broker, advertized to the cluster.\n"
              "Defaults to a URL listing all the local IP addresses\n")
             ;
     }
-};
 
-struct ClusterPlugin : public PluginT<Broker> {
-    ClusterOptions options;
-    boost::optional<Cluster> cluster;
-
-    ClusterPlugin(const ClusterOptions& opts) : options(opts) {}
-    
-    void initializeT(Broker& broker) { // FIXME aconway 2008-07-01: drop T suffix.
-        Url url = options.url.empty() ? Url::getIpAddressesUrl(broker.getPort()) : Url(options.url);
-        cluster = boost::in_place(options.name, url, boost::ref(broker));
-        broker.getConnectionManager().add(cluster->getObserver());	// FIXME aconway 2008-07-01: to Cluster ctor
+    Url getUrl(uint16_t port) const {
+        if (url.empty()) return Url::getIpAddressesUrl(port);
+        return Url(url);
     }
 };
 
-struct PluginFactory : public Plugin::FactoryT<Broker> {
+struct ClusterPlugin : public Plugin {
 
     ClusterOptions options;
+    boost::optional<Cluster> cluster;
 
     Options* getOptions() { return &options; }
 
-    boost::shared_ptr<Plugin> createT(Broker&) {
-        if (options.name.empty()) { // No cluster name, don't initialize cluster.
-            return boost::shared_ptr<Plugin>();
+    void earlyInitialize(Plugin::Target&) {}
+
+    void initialize(Plugin::Target& target) {
+        broker::Broker* broker = dynamic_cast<broker::Broker*>(&target);
+        // Only provide to a Broker, and only if the --cluster config is set.
+        if (broker && !options.name.empty()) {
+            assert(!cluster); // A process can only belong to one cluster.
+            cluster = boost::in_place(options.name,
+                                      options.getUrl(broker->getPort()),
+                                      boost::ref(*broker));
+            broker->getConnectionManager().add(cluster->getObserver());	
         }
-        else
-            return make_shared_ptr(new ClusterPlugin(options));
     }
 };
 
-static PluginFactory instance; // Static initialization.
+static ClusterPlugin instance; // Static initialization.
     
 }} // namespace qpid::cluster
diff --git a/cpp/src/qpid/sys/TCPIOPlugin.cpp b/cpp/src/qpid/sys/TCPIOPlugin.cpp
index 9d272ee69c..e82a6a9102 100644
--- a/cpp/src/qpid/sys/TCPIOPlugin.cpp
+++ b/cpp/src/qpid/sys/TCPIOPlugin.cpp
@@ -53,20 +53,22 @@ class AsynchIOProtocolFactory : public ProtocolFactory {
                      bool isClient);
 };
 
-struct TCPIOPlugin : public PluginT<broker::Broker> {
-    void initializeT(broker::Broker& broker) {
-        const broker::Broker::Options& opts = broker.getOptions();
-        ProtocolFactory::shared_ptr protocol(new AsynchIOProtocolFactory(opts.port, opts.connectionBacklog));
-        QPID_LOG(info, "Listening on TCP port " << protocol->getPort());
-        broker.registerProtocolFactory(protocol);
+// Static instance to initialise plugin
+static class TCPIOPlugin : public Plugin {
+    void earlyInitialize(Target&) {
     }
-};
-
-static struct TCPIOPluginFactory : public Plugin::FactoryT<broker::Broker> {
-    boost::shared_ptr<Plugin> createT(broker::Broker&) {
-        return make_shared_ptr(new TCPIOPlugin());
+    
+    void initialize(Target& target) {
+        broker::Broker* broker = dynamic_cast<broker::Broker*>(&target);
+        // Only provide to a Broker
+        if (broker) {
+            const broker::Broker::Options& opts = broker->getOptions();
+            ProtocolFactory::shared_ptr protocol(new AsynchIOProtocolFactory(opts.port, opts.connectionBacklog));
+            QPID_LOG(info, "Listening on TCP port " << protocol->getPort());
+            broker->registerProtocolFactory(protocol);
+        }
     }
-} theTCPIOPluginFactory;   // Static plugin factory instance.
+} tcpPlugin;
 
 AsynchIOProtocolFactory::AsynchIOProtocolFactory(int16_t port, int backlog) :
     listeningPort(listener.listen(port, backlog))
diff --git a/cpp/src/qpidd.cpp b/cpp/src/qpidd.cpp
index 6b6d37c933..3d4b25e1a8 100644
--- a/cpp/src/qpidd.cpp
+++ b/cpp/src/qpidd.cpp
@@ -103,7 +103,7 @@ struct QpiddOptions : public qpid::Options {
         add(broker);
         add(daemon);
         add(log);
-        Plugin::Factory::addOptions(*this);
+        Plugin::addOptions(*this);
     }
 
     void usage() const {
diff --git a/cpp/src/tests/Makefile.am b/cpp/src/tests/Makefile.am
index f72264d69a..0fadc39c8b 100644
--- a/cpp/src/tests/Makefile.am
+++ b/cpp/src/tests/Makefile.am
@@ -135,7 +135,8 @@ EXTRA_DIST +=								\
   MessageUtils.h							\
   TestMessageStore.h							\
   MockConnectionInputHandler.h						\
-  TxMocks.h
+  TxMocks.h								\
+  qpid_test_plugin.h
 
 check_LTLIBRARIES += libdlclose_noop.la
 libdlclose_noop_la_LDFLAGS = -module -rpath $(abs_builddir)
diff --git a/cpp/src/tests/cluster.mk b/cpp/src/tests/cluster.mk
index 8c6c69db21..da5b4d6e90 100644
--- a/cpp/src/tests/cluster.mk
+++ b/cpp/src/tests/cluster.mk
@@ -17,6 +17,6 @@ EXTRA_DIST+=ais_check
 
 check_PROGRAMS+=cluster_test
 cluster_test_SOURCES=unit_test.cpp cluster_test.cpp
-cluster_test_LDADD=$(lib_client) $(lib_cluster) $(lib_broker) -lboost_unit_test_framework
+cluster_test_LDADD=$(lib_client) $(lib_cluster) -lboost_unit_test_framework
 
 endif
diff --git a/cpp/src/tests/cluster_test.cpp b/cpp/src/tests/cluster_test.cpp
index 82d18aceff..65aa4d5a28 100644
--- a/cpp/src/tests/cluster_test.cpp
+++ b/cpp/src/tests/cluster_test.cpp
@@ -60,7 +60,7 @@ struct ClusterFixture : public ptr_vector<ForkedBroker> {
 
 void ClusterFixture::add() {
     broker::Broker::Options opts;
-    Plugin::Factory::addOptions(opts); // For cluster options.
+    Plugin::addOptions(opts); // For cluster options.
     const char* argv[] = {
         "", "--cluster-name", name.c_str(), "--auth=no", "--no-data-dir"
     };