MXS-1929: Store filters in smart pointers

The FilterDef structs are now stored in a vector<std::shared_ptr>. This
should make it easier to use filters even if they are deleted before the
session using them closes.

All internal functions now take a smart point as a parameter. One
problematic case will be debugcmd.cc which moves information around as
pointers cast into unsigned longs.

server/core/filter.cc

@@ -23,19 +23,20 @@
 #include <errno.h>
 #include <sys/stat.h>
 #include <fcntl.h>
-#include <string>
+
+#include <algorithm>
+#include <memory>
 #include <set>
+#include <string>
 #include <vector>
 
 #include <maxscale/alloc.h>
 #include <maxscale/log_manager.h>
 #include <maxscale/paths.h>
 #include <maxscale/session.h>
-#include <maxscale/spinlock.hh>
 #include <maxscale/service.h>
 #include <maxscale/filter.hh>
 #include <maxscale/json_api.h>
-#include <algorithm>
 
 #include "internal/config.hh"
 #include "internal/modules.h"
@@ -43,11 +44,12 @@
 
 using std::string;
 using std::set;
+using Guard = std::lock_guard<std::mutex>;
 
 using namespace maxscale;
 
-static SpinLock filter_spin;                /**< Protects the list of all filters */
+static std::mutex filter_lock; /**< Protects the list of all filters */
-static std::vector<FilterDef*>  allFilters; /**< The list of all filters */
+static std::vector<SFilterDef> all_filters; /**< The list of all filters */
 
 /**
  * Free filter parameters
@@ -67,7 +69,7 @@ static void filter_free_parameters(FilterDef* filter)
  *
  * @return The newly created filter or NULL if an error occurred
  */
-FilterDef* filter_alloc(const char *name, const char *module, MXS_CONFIG_PARAMETER* params)
+SFilterDef filter_alloc(const char *name, const char *module, MXS_CONFIG_PARAMETER* params)
 {
     MXS_FILTER_OBJECT* object = (MXS_FILTER_OBJECT*)load_module(module, MODULE_FILTER);
 
@@ -77,44 +79,57 @@ FilterDef* filter_alloc(const char *name, const char *module, MXS_CONFIG_PARAMET
         return NULL;
     }
 
-    char* my_name = MXS_STRDUP(name);
+    MXS_FILTER* instance = object->createInstance(name, params);
-    char* my_module = MXS_STRDUP(module);
 
-    FilterDef* filter = new (std::nothrow) FilterDef;
+    if (instance == NULL)
-
-    if (!my_name || !my_module || !filter)
     {
-        MXS_FREE(my_name);
+        MXS_ERROR("Failed to create filter '%s' instance.", name);
-        MXS_FREE(my_module);
-        delete filter;
         return NULL;
     }
-    filter->name = my_name;
+
-    filter->module = my_module;
+    SFilterDef filter(new (std::nothrow) FilterDef(name, module, object, instance, params));
-    filter->obj = object;
+
-    filter->parameters = NULL;
+    if (filter)
-    spinlock_init(&filter->spin);
+    {
+        Guard guard(filter_lock);
+        all_filters.push_back(filter);
+    }
+    else
+    {
+        object->destroyInstance(instance);
+    }
+
+    return filter;
+}
+
+FilterDef::FilterDef(std::string name, std::string module, MXS_FILTER_OBJECT* object,
+                     MXS_FILTER* instance, MXS_CONFIG_PARAMETER* params):
+    name(name),
+    module(module),
+    parameters(NULL),
+    filter(instance),
+    obj(object)
+{
+    // TODO: Add config_clone_param_chain
+    CONFIG_CONTEXT ctx = {};
+    ctx.object = (char*)"";
 
     for (MXS_CONFIG_PARAMETER* p = params; p; p = p->next)
     {
-        filter_add_parameter(filter, p->name, p->value);
+        config_add_param(&ctx, p->name, p->value);
     }
 
-    if ((filter->filter = object->createInstance(name, params)) == NULL)
+    parameters = ctx.parameters;
+}
+
+FilterDef::~FilterDef()
+{
+    if (obj->destroyInstance && filter)
     {
-        MXS_ERROR("Failed to create filter '%s' instance.", name);
+        obj->destroyInstance(filter);
-        filter_free_parameters(filter);
-        MXS_FREE(my_name);
-        MXS_FREE(my_module);
-        delete filter;
-        return NULL;
     }
 
-    filter_spin.acquire();
+    filter_free_parameters(this);
-    allFilters.push_back(filter);
-    filter_spin.release();
-
-    return filter;
 }
 
 /**
@@ -122,56 +137,35 @@ FilterDef* filter_alloc(const char *name, const char *module, MXS_CONFIG_PARAMET
  *
  * @param filter        The filter to free
  */
-void filter_free(FilterDef* filter)
+void filter_free(const SFilterDef& filter)
 {
-    if (filter)
+    // Removing the filter from the list will trigger deletion once it's no longer in use
-    {
+    Guard guard(filter_lock);
-        /* First of all remove from the linked list */
+    auto it = std::remove(all_filters.begin(), all_filters.end(), filter);
-
+    all_filters.erase(it);
-        filter_spin.acquire();
-        auto it = std::remove(allFilters.begin(), allFilters.end(), filter);
-        allFilters.erase(it);
-        filter_spin.release();
-
-        /* Clean up session and free the memory */
-        MXS_FREE(filter->name);
-        MXS_FREE(filter->module);
-
-        filter_free_parameters(filter);
-
-        delete filter;
-    }
 }
 
-FilterDef* filter_find(const char *name)
+SFilterDef filter_find(const char *name)
 {
-    FilterDef* rval = NULL;
+    Guard guard(filter_lock);
-    filter_spin.acquire();
 
-    for (FilterDef* filter: allFilters)
+    for (const auto& filter : all_filters)
     {
-        if (strcmp(filter->name, name) == 0)
+        if (filter->name == name)
         {
-            rval = filter;
+            return filter;
-            break;
         }
     }
 
-    filter_spin.release();
+    return SFilterDef();
-    return rval;
 }
 
-MXS_FILTER_DEF* filter_def_find(const char *name)
+bool filter_can_be_destroyed(const SFilterDef& filter)
-{
-    return filter_find(name);
-}
-
-bool filter_can_be_destroyed(MXS_FILTER_DEF *filter)
 {
     return !service_filter_in_use(filter);
 }
 
-void filter_destroy(MXS_FILTER_DEF *filter)
+void filter_destroy(const SFilterDef& filter)
 {
     ss_dassert(filter_can_be_destroyed(filter));
     ss_info_dassert(!true, "Not yet implemented");
@@ -179,9 +173,9 @@ void filter_destroy(MXS_FILTER_DEF *filter)
 
 void filter_destroy_instances()
 {
-    filter_spin.acquire();
+    Guard guard(filter_lock);
 
-    for (FilterDef* filter: allFilters)
+    for (const auto& filter : all_filters)
     {
         // NOTE: replace this with filter_destroy
         if (filter->obj->destroyInstance)
@@ -189,20 +183,18 @@ void filter_destroy_instances()
             filter->obj->destroyInstance(filter->filter);
         }
     }
-
-    filter_spin.release();
 }
 
 const char* filter_def_get_name(const MXS_FILTER_DEF* filter_def)
 {
     const FilterDef* filter = static_cast<const FilterDef*>(filter_def);
-    return filter->name;
+    return filter->name.c_str();
 }
 
 const char* filter_def_get_module_name(const MXS_FILTER_DEF* filter_def)
 {
     const FilterDef* filter = static_cast<const FilterDef*>(filter_def);
-    return filter->module;
+    return filter->module.c_str();
 }
 
 MXS_FILTER* filter_def_get_instance(const MXS_FILTER_DEF* filter_def)
@@ -235,12 +227,12 @@ filter_standard_parameter(const char *name)
 void
 dprintAllFilters(DCB *dcb)
 {
-    filter_spin.acquire();
+    Guard guard(filter_lock);
 
-    for (FilterDef* ptr: allFilters)
+    for (const auto& ptr : all_filters)
     {
-        dcb_printf(dcb, "FilterDef %p (%s)\n", ptr, ptr->name);
+        dcb_printf(dcb, "FilterDef %p (%s)\n", ptr.get(), ptr->name.c_str());
-        dcb_printf(dcb, "\tModule:      %s\n", ptr->module);
+        dcb_printf(dcb, "\tModule:      %s\n", ptr->module.c_str());
         if (ptr->obj && ptr->filter)
         {
             ptr->obj->diagnostics(ptr->filter, NULL, dcb);
@@ -250,8 +242,6 @@ dprintAllFilters(DCB *dcb)
             dcb_printf(dcb, "\tModule not loaded.\n");
         }
     }
-
-    filter_spin.release();
 }
 
 /**
@@ -260,10 +250,10 @@ dprintAllFilters(DCB *dcb)
  * Designed to be called within a debug CLI in order
  * to display all active filters in MaxScale
  */
-void dprintFilter(DCB *dcb, const FilterDef *filter)
+void dprintFilter(DCB *dcb, const SFilterDef& filter)
 {
-    dcb_printf(dcb, "FilterDef %p (%s)\n", filter, filter->name);
+    dcb_printf(dcb, "FilterDef %p (%s)\n", filter.get(), filter->name.c_str());
-    dcb_printf(dcb, "\tModule:      %s\n", filter->module);
+    dcb_printf(dcb, "\tModule:      %s\n", filter->module.c_str());
     if (filter->obj && filter->filter)
     {
         filter->obj->diagnostics(filter->filter, NULL, dcb);
@@ -277,29 +267,26 @@ void dprintFilter(DCB *dcb, const FilterDef *filter)
 void
 dListFilters(DCB *dcb)
 {
-    filter_spin.acquire();
+    Guard guard(filter_lock);
 
-    if (!allFilters.empty())
+    if (!all_filters.empty())
     {
         dcb_printf(dcb, "FilterDefs\n");
         dcb_printf(dcb, "--------------------+-----------------+----------------------------------------\n");
         dcb_printf(dcb, "%-19s | %-15s | Options\n",
                    "FilterDef", "Module");
         dcb_printf(dcb, "--------------------+-----------------+----------------------------------------\n");
-    }
 
-    for (FilterDef* ptr: allFilters)
+        for (const auto& ptr : all_filters)
-    {
+        {
-        dcb_printf(dcb, "%-19s | %-15s | ",
+            dcb_printf(dcb, "%-19s | %-15s | ",
-                   ptr->name, ptr->module);
+                       ptr->name.c_str(), ptr->module.c_str());
-        dcb_printf(dcb, "\n");
+            dcb_printf(dcb, "\n");
-    }
+        }
-    if (!allFilters.empty())
+
-    {
         dcb_printf(dcb,
                    "--------------------+-----------------+----------------------------------------\n\n");
     }
-    filter_spin.release();
 }
 
 /**
@@ -310,7 +297,7 @@ dListFilters(DCB *dcb)
  * @param value         The parameter value
  */
 void
-filter_add_parameter(FilterDef *filter, const char *name, const char *value)
+filter_add_parameter(SFilterDef& filter, const char *name, const char *value)
 {
     CONFIG_CONTEXT ctx = {};
     ctx.object = (char*)"";
@@ -332,7 +319,7 @@ filter_add_parameter(FilterDef *filter, const char *name, const char *value)
  * @return              The downstream component for the next filter or NULL
  *                      if the filter could not be created
  */
-MXS_DOWNSTREAM* filter_apply(FilterDef* filter, MXS_SESSION *session, MXS_DOWNSTREAM *downstream)
+MXS_DOWNSTREAM* filter_apply(const SFilterDef& filter, MXS_SESSION *session, MXS_DOWNSTREAM *downstream)
 {
     MXS_DOWNSTREAM *me;
 
@@ -366,7 +353,7 @@ MXS_DOWNSTREAM* filter_apply(FilterDef* filter, MXS_SESSION *session, MXS_DOWNST
  * @param upstream      The filter that should be upstream of this filter
  * @return              The upstream component for the next filter
  */
-MXS_UPSTREAM* filter_upstream(FilterDef* filter, MXS_FILTER_SESSION *fsession, MXS_UPSTREAM *upstream)
+MXS_UPSTREAM* filter_upstream(const SFilterDef& filter, MXS_FILTER_SESSION *fsession, MXS_UPSTREAM *upstream)
 {
     MXS_UPSTREAM *me = NULL;
 
@@ -392,27 +379,28 @@ MXS_UPSTREAM* filter_upstream(FilterDef* filter, MXS_FILTER_SESSION *fsession, M
     }
     return me;
 }
-json_t* filter_parameters_to_json(const FilterDef* filter)
+
+json_t* filter_parameters_to_json(const SFilterDef&  filter)
 {
     json_t* rval = json_object();
 
     /** Add custom module parameters */
-    const MXS_MODULE* mod = get_module(filter->module, MODULE_FILTER);
+    const MXS_MODULE* mod = get_module(filter->module.c_str(), MODULE_FILTER);
     config_add_module_params_json(mod, filter->parameters, config_filter_params, rval);
 
     return rval;
 }
 
-json_t* filter_json_data(const FilterDef* filter, const char* host)
+json_t* filter_json_data(const SFilterDef&  filter, const char* host)
 {
     json_t* rval = json_object();
 
-    json_object_set_new(rval, CN_ID, json_string(filter->name));
+    json_object_set_new(rval, CN_ID, json_string(filter->name.c_str()));
     json_object_set_new(rval, CN_TYPE, json_string(CN_FILTERS));
 
     json_t* attr = json_object();
 
-    json_object_set_new(attr, CN_MODULE, json_string(filter->module));
+    json_object_set_new(attr, CN_MODULE, json_string(filter->module.c_str()));
     json_object_set_new(attr, CN_PARAMETERS, filter_parameters_to_json(filter));
 
     if (filter->obj && filter->filter && filter->obj->diagnostics_json)
@@ -431,12 +419,12 @@ json_t* filter_json_data(const FilterDef* filter, const char* host)
 
     json_object_set_new(rval, CN_RELATIONSHIPS, rel);
     json_object_set_new(rval, CN_ATTRIBUTES, attr);
-    json_object_set_new(rval, CN_LINKS, mxs_json_self_link(host, CN_FILTERS, filter->name));
+    json_object_set_new(rval, CN_LINKS, mxs_json_self_link(host, CN_FILTERS, filter->name.c_str()));
 
     return rval;
 }
 
-json_t* filter_to_json(const FilterDef* filter, const char* host)
+json_t* filter_to_json(const SFilterDef&  filter, const char* host)
 {
     string self = MXS_JSON_API_FILTERS;
     self += filter->name;
@@ -447,9 +435,9 @@ json_t* filter_list_to_json(const char* host)
 {
     json_t* rval = json_array();
 
-    filter_spin.acquire();
+    Guard guard(filter_lock);
 
-    for (FilterDef* f: allFilters)
+    for (const auto& f : all_filters)
     {
         json_t* json = filter_json_data(f, host);
 
@@ -459,8 +447,6 @@ json_t* filter_list_to_json(const char* host)
         }
     }
 
-    filter_spin.release();
-
     return mxs_json_resource(host, MXS_JSON_API_FILTERS, rval);
 }
 
@@ -515,22 +501,22 @@ json_t* FilterSession::diagnostics_json() const
 
 }
 
-static bool create_filter_config(const FilterDef *filter, const char *filename)
+static bool create_filter_config(const SFilterDef& filter, const char *filename)
 {
     int file = open(filename, O_EXCL | O_CREAT | O_WRONLY, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
 
     if (file == -1)
     {
         MXS_ERROR("Failed to open file '%s' when serializing filter '%s': %d, %s",
-                  filename, filter->name, errno, mxs_strerror(errno));
+                  filename, filter->name.c_str(), errno, mxs_strerror(errno));
         return false;
     }
 
-    mxs::SpinLockGuard guard(filter->spin);
+    Guard guard(filter->lock);
 
-    dprintf(file, "[%s]\n", filter->name);
+    dprintf(file, "[%s]\n", filter->name.c_str());
     dprintf(file, "%s=%s\n", CN_TYPE, CN_FILTER);
-    dprintf(file, "%s=%s\n", CN_MODULE, filter->module);
+    dprintf(file, "%s=%s\n", CN_MODULE, filter->module.c_str());
 
     std::set<std::string> param_set{CN_TYPE, CN_MODULE};
 
@@ -547,12 +533,12 @@ static bool create_filter_config(const FilterDef *filter, const char *filename)
     return true;
 }
 
-bool filter_serialize(const FilterDef *filter)
+bool filter_serialize(const SFilterDef& filter)
 {
     bool rval = false;
     char filename[PATH_MAX];
     snprintf(filename, sizeof(filename), "%s/%s.cnf.tmp", get_config_persistdir(),
-             filter->name);
+             filter->name.c_str());
 
     if (unlink(filename) == -1 && errno != ENOENT)
     {

server/core/internal/config_runtime.h

@@ -215,7 +215,7 @@ bool runtime_create_filter(const char *name, const char *module, MXS_CONFIG_PARA
  *
  * @return True if filter was destroyed
  */
-bool runtime_destroy_filter(FilterDef* filter);
+bool runtime_destroy_filter(const SFilterDef& filter);
 
 /**
  * @brief Destroy a monitor
@@ -283,9 +283,9 @@ MXS_MONITOR* runtime_create_monitor_from_json(json_t* json);
  *
  * @param json JSON defining the filter
  *
- * @return Created filter or NULL on error
+ * @return True if filter was created, false on error
  */
-FilterDef* runtime_create_filter_from_json(json_t* json);
+bool runtime_create_filter_from_json(json_t* json);
 
 /**
  * @brief Create a new service from JSON

server/core/internal/filter.hh

@@ -18,33 +18,42 @@
 
 #include <maxscale/filter.h>
 
+#include <memory>
+#include <mutex>
+
 /**
  * The definition of a filter from the configuration file.
  * This is basically the link between a plugin to load and the
  * options to pass to that plugin.
  */
+// TODO: Make this a class
 struct FilterDef: public MXS_FILTER_DEF
 {
-    char *name;                   /**< The Filter name */
+    FilterDef(std::string name, std::string module, MXS_FILTER_OBJECT* object,
-    char *module;                 /**< The module to load */
+              MXS_FILTER* instance, MXS_CONFIG_PARAMETER* params);
+    ~FilterDef();
+
+    std::string name; /**< The Filter name */
+    std::string module; /**< The module to load */
     MXS_CONFIG_PARAMETER *parameters; /**< The filter parameters */
-    MXS_FILTER* filter;           /**< The runtime filter */
+    MXS_FILTER* filter; /**< The runtime filter */
-    MXS_FILTER_OBJECT *obj;       /**< The "MODULE_OBJECT" for the filter */
+    MXS_FILTER_OBJECT *obj; /**< The "MODULE_OBJECT" for the filter */
-    SPINLOCK spin;                /**< Spinlock to protect the filter definition */
+    mutable std::mutex lock;
-    struct FilterDef *next;  /**< Next filter in the chain of all filters */
 };
 
-void filter_add_parameter(FilterDef *filter_def, const char *name, const char *value);
+typedef std::shared_ptr<FilterDef> SFilterDef;
-FilterDef* filter_alloc(const char *name, const char *module, MXS_CONFIG_PARAMETER* params);
+
-MXS_DOWNSTREAM *filter_apply(FilterDef* filter_def, MXS_SESSION *session, MXS_DOWNSTREAM *downstream);
+void filter_add_parameter(SFilterDef& filter_def, const char *name, const char *value);
-void filter_free(FilterDef *filter);
+SFilterDef filter_alloc(const char *name, const char *module, MXS_CONFIG_PARAMETER* params);
+MXS_DOWNSTREAM* filter_apply(const SFilterDef& filter_def, MXS_SESSION *session, MXS_DOWNSTREAM *downstream);
+void filter_free(const SFilterDef& filter);
 int filter_standard_parameter(const char *name);
-MXS_UPSTREAM *filter_upstream(FilterDef* filter_def,
+MXS_UPSTREAM *filter_upstream(const SFilterDef& filter_def,
                               MXS_FILTER_SESSION *fsession,
                               MXS_UPSTREAM *upstream);
 
 // Find the internal filter representation
-FilterDef* filter_find(const char *name);
+SFilterDef filter_find(const char *name);
 
 /**
  * Check if filter can be destroyed
@@ -55,14 +64,14 @@ FilterDef* filter_find(const char *name);
  *
  * @return True if filter can be destroyed
  */
-bool filter_can_be_destroyed(MXS_FILTER_DEF *filter);
+bool filter_can_be_destroyed(const SFilterDef& filter);
 
 /**
  * Destroy a filter
  *
  * @param filter Filter to destroy
  */
-void filter_destroy(MXS_FILTER_DEF *filter);
+void filter_destroy(const SFilterDef& filter);
 
 /**
  * Destroy all filters
@@ -78,10 +87,10 @@ void filter_destroy_instances();
  *
  * @return True if serialization was successful
  */
-bool filter_serialize(const FilterDef *filter);
+bool filter_serialize(const SFilterDef& filter);
 
 void dprintAllFilters(DCB *);
-void dprintFilter(DCB *, const FilterDef *);
+void dprintFilter(DCB *, const SFilterDef&);
 void dListFilters(DCB *);
 
 /**
@@ -92,7 +101,7 @@ void dListFilters(DCB *);
  *
  * @return Filter converted to JSON format
  */
-json_t* filter_to_json(const FilterDef* filter, const char* host);
+json_t* filter_to_json(const SFilterDef& filter, const char* host);
 
 /**
  * @brief Convert all filters into JSON