From 00ab890b19ed9539929ab288c716a02519b8505e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Wed, 1 Aug 2018 19:06:54 +0300 Subject: [PATCH] MXS-1929: Store filters in smart pointers The FilterDef structs are now stored in a vector. 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 | 214 ++++++++++++-------------- server/core/internal/config_runtime.h | 6 +- server/core/internal/filter.hh | 43 ++++-- 3 files changed, 129 insertions(+), 134 deletions(-) diff --git a/server/core/filter.cc b/server/core/filter.cc index 665a9a61a..b6c24de19 100644 --- a/server/core/filter.cc +++ b/server/core/filter.cc @@ -23,19 +23,20 @@ #include #include #include -#include + +#include +#include #include +#include #include #include #include #include #include -#include #include #include #include -#include #include "internal/config.hh" #include "internal/modules.h" @@ -43,11 +44,12 @@ using std::string; using std::set; +using Guard = std::lock_guard; using namespace maxscale; -static SpinLock filter_spin; /**< Protects the list of all filters */ -static std::vector allFilters; /**< The list of all filters */ +static std::mutex filter_lock; /**< Protects the list of all filters */ +static std::vector 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); - char* my_module = MXS_STRDUP(module); + MXS_FILTER* instance = object->createInstance(name, params); - FilterDef* filter = new (std::nothrow) FilterDef; - - if (!my_name || !my_module || !filter) + if (instance == NULL) { - MXS_FREE(my_name); - MXS_FREE(my_module); - delete filter; + MXS_ERROR("Failed to create filter '%s' instance.", name); return NULL; } - filter->name = my_name; - filter->module = my_module; - filter->obj = object; - filter->parameters = NULL; - spinlock_init(&filter->spin); + + SFilterDef filter(new (std::nothrow) FilterDef(name, module, object, instance, params)); + + if (filter) + { + 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); - filter_free_parameters(filter); - MXS_FREE(my_name); - MXS_FREE(my_module); - delete filter; - return NULL; + obj->destroyInstance(filter); } - filter_spin.acquire(); - allFilters.push_back(filter); - filter_spin.release(); - - return filter; + filter_free_parameters(this); } /** @@ -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) - { - /* First of all remove from the linked list */ - - 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; - } + // Removing the filter from the list will trigger deletion once it's no longer in use + Guard guard(filter_lock); + auto it = std::remove(all_filters.begin(), all_filters.end(), filter); + all_filters.erase(it); } -FilterDef* filter_find(const char *name) +SFilterDef filter_find(const char *name) { - FilterDef* rval = NULL; - filter_spin.acquire(); + Guard guard(filter_lock); - for (FilterDef* filter: allFilters) + for (const auto& filter : all_filters) { - if (strcmp(filter->name, name) == 0) + if (filter->name == name) { - rval = filter; - break; + return filter; } } - filter_spin.release(); - return rval; + return SFilterDef(); } -MXS_FILTER_DEF* filter_def_find(const char *name) -{ - return filter_find(name); -} - -bool filter_can_be_destroyed(MXS_FILTER_DEF *filter) +bool filter_can_be_destroyed(const SFilterDef& 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(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(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, "\tModule: %s\n", ptr->module); + dcb_printf(dcb, "FilterDef %p (%s)\n", ptr.get(), ptr->name.c_str()); + 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, "\tModule: %s\n", filter->module); + dcb_printf(dcb, "FilterDef %p (%s)\n", filter.get(), filter->name.c_str()); + 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) - { - dcb_printf(dcb, "%-19s | %-15s | ", - ptr->name, ptr->module); - dcb_printf(dcb, "\n"); - } - if (!allFilters.empty()) - { + for (const auto& ptr : all_filters) + { + dcb_printf(dcb, "%-19s | %-15s | ", + ptr->name.c_str(), ptr->module.c_str()); + dcb_printf(dcb, "\n"); + } + 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 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) { diff --git a/server/core/internal/config_runtime.h b/server/core/internal/config_runtime.h index df9348a61..1314b909e 100644 --- a/server/core/internal/config_runtime.h +++ b/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 diff --git a/server/core/internal/filter.hh b/server/core/internal/filter.hh index cb4657fdd..e32c817cc 100644 --- a/server/core/internal/filter.hh +++ b/server/core/internal/filter.hh @@ -18,33 +18,42 @@ #include +#include +#include + /** * 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 */ - char *module; /**< The module to load */ + FilterDef(std::string name, std::string module, MXS_FILTER_OBJECT* object, + 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_OBJECT *obj; /**< The "MODULE_OBJECT" for the filter */ - SPINLOCK spin; /**< Spinlock to protect the filter definition */ - struct FilterDef *next; /**< Next filter in the chain of all filters */ + MXS_FILTER* filter; /**< The runtime filter */ + MXS_FILTER_OBJECT *obj; /**< The "MODULE_OBJECT" for the filter */ + mutable std::mutex lock; }; -void filter_add_parameter(FilterDef *filter_def, const char *name, const char *value); -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_free(FilterDef *filter); +typedef std::shared_ptr SFilterDef; + +void filter_add_parameter(SFilterDef& filter_def, const char *name, const char *value); +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