diff --git a/include/maxscale/filter.h b/include/maxscale/filter.h index 607184c42..590f54f5a 100644 --- a/include/maxscale/filter.h +++ b/include/maxscale/filter.h @@ -225,16 +225,6 @@ typedef struct mxs_filter_def { } MXS_FILTER_DEF; -/** - * Lookup a filter definition using the unique section name in - * the configuration file. - * - * @param name The name of a filter. - * - * @return A filter definition or NULL if not found. - */ -MXS_FILTER_DEF *filter_def_find(const char *name); - /** * Get the name of a filter definition. This corresponds to * to a filter section in the configuration file. diff --git a/include/maxscale/service.h b/include/maxscale/service.h index 99d4e86cb..433c1a5d1 100644 --- a/include/maxscale/service.h +++ b/include/maxscale/service.h @@ -146,8 +146,6 @@ typedef struct service * when querying them from the server. MySQL Workbench seems * to escape at least the underscore character. */ SERVICE_REFRESH_RATE *rate_limits; /**< The refresh rate limits for users of each thread */ - MXS_FILTER_DEF **filters; /**< Ordered list of filters */ - int n_filters; /**< Number of filters */ int64_t conn_idle_timeout; /**< Session timeout in seconds */ char weightby[MAX_SERVICE_WEIGHTBY_LEN]; /**< Service weighting parameter name */ bool retry_start; /**< If starting of the service should be retried later */ diff --git a/server/core/config_runtime.cc b/server/core/config_runtime.cc index 20c3db08b..8db9d7c30 100644 --- a/server/core/config_runtime.cc +++ b/server/core/config_runtime.cc @@ -1035,9 +1035,9 @@ bool runtime_create_filter(const char *name, const char *module, MXS_CONFIG_PARA mxs::SpinLockGuard guard(crt_lock); bool rval = false; - if (filter_def_find(name) == NULL) + if (!filter_find(name)) { - FilterDef* filter = NULL; + SFilterDef filter; CONFIG_CONTEXT ctx{(char*)""}; ctx.parameters = load_defaults(module, MODULE_FILTER, CN_FILTER); @@ -1048,7 +1048,7 @@ bool runtime_create_filter(const char *name, const char *module, MXS_CONFIG_PARA config_replace_param(&ctx, p->name, p->value); } - if ((filter = filter_alloc(name, module, ctx.parameters)) == NULL) + if (!(filter = filter_alloc(name, module, ctx.parameters))) { runtime_error("Could not create filter '%s' with module '%s'", name, module); } @@ -1077,7 +1077,7 @@ bool runtime_create_filter(const char *name, const char *module, MXS_CONFIG_PARA return rval; } -bool runtime_destroy_filter(FilterDef* filter) +bool runtime_destroy_filter(const SFilterDef& filter) { ss_dassert(filter); bool rval = false; @@ -1091,7 +1091,7 @@ bool runtime_destroy_filter(FilterDef* filter) else { runtime_error("Filter '%s' cannot be destroyed: Remove it from all services " - "first", filter->name); + "first", filter->name.c_str()); } return rval; @@ -1438,7 +1438,7 @@ static bool filter_to_service_relation_is_valid(const std::string& type, const s static bool service_to_filter_relation_is_valid(const std::string& type, const std::string& value) { - return type == CN_FILTERS && filter_def_find(value.c_str()); + return type == CN_FILTERS && filter_find(value.c_str()); } static bool unlink_server_from_objects(SERVER* server, StringSet& relations) @@ -1898,9 +1898,9 @@ MXS_MONITOR* runtime_create_monitor_from_json(json_t* json) return rval; } -FilterDef* runtime_create_filter_from_json(json_t* json) +bool runtime_create_filter_from_json(json_t* json) { - FilterDef* rval = NULL; + bool rval = false; if (validate_object_json(json, {MXS_JSON_PTR_MODULE}, {filter_to_service})) { @@ -1908,11 +1908,7 @@ FilterDef* runtime_create_filter_from_json(json_t* json) const char* module = json_string_value(mxs_json_pointer(json, MXS_JSON_PTR_MODULE)); MXS_CONFIG_PARAMETER* params = extract_parameters_from_json(json); - if (runtime_create_filter(name, module, params)) - { - rval = filter_find(name); - ss_dassert(rval); - } + rval = runtime_create_filter(name, module, params); config_parameter_free(params); } diff --git a/server/core/filter.cc b/server/core/filter.cc index b6c24de19..eb839e82d 100644 --- a/server/core/filter.cc +++ b/server/core/filter.cc @@ -139,6 +139,7 @@ FilterDef::~FilterDef() */ void filter_free(const SFilterDef& filter) { + ss_dassert(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); @@ -162,11 +163,13 @@ SFilterDef filter_find(const char *name) bool filter_can_be_destroyed(const SFilterDef& filter) { + ss_dassert(filter); return !service_filter_in_use(filter); } void filter_destroy(const SFilterDef& filter) { + ss_dassert(filter); ss_dassert(filter_can_be_destroyed(filter)); ss_info_dassert(!true, "Not yet implemented"); } @@ -188,18 +191,21 @@ void filter_destroy_instances() const char* filter_def_get_name(const MXS_FILTER_DEF* filter_def) { const FilterDef* filter = static_cast(filter_def); + ss_dassert(filter); 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); + ss_dassert(filter); return filter->module.c_str(); } MXS_FILTER* filter_def_get_instance(const MXS_FILTER_DEF* filter_def) { const FilterDef* filter = static_cast(filter_def); + ss_dassert(filter); return filter->filter; } @@ -252,6 +258,7 @@ dprintAllFilters(DCB *dcb) */ void dprintFilter(DCB *dcb, const SFilterDef& filter) { + ss_dassert(filter); 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) @@ -299,6 +306,7 @@ dListFilters(DCB *dcb) void filter_add_parameter(SFilterDef& filter, const char *name, const char *value) { + ss_dassert(filter); CONFIG_CONTEXT ctx = {}; ctx.object = (char*)""; @@ -321,6 +329,7 @@ filter_add_parameter(SFilterDef& filter, const char *name, const char *value) */ MXS_DOWNSTREAM* filter_apply(const SFilterDef& filter, MXS_SESSION *session, MXS_DOWNSTREAM *downstream) { + ss_dassert(filter); MXS_DOWNSTREAM *me; if ((me = (MXS_DOWNSTREAM *)MXS_CALLOC(1, sizeof(MXS_DOWNSTREAM))) == NULL) @@ -355,6 +364,7 @@ MXS_DOWNSTREAM* filter_apply(const SFilterDef& filter, MXS_SESSION *session, MXS */ MXS_UPSTREAM* filter_upstream(const SFilterDef& filter, MXS_FILTER_SESSION *fsession, MXS_UPSTREAM *upstream) { + ss_dassert(filter); MXS_UPSTREAM *me = NULL; /* @@ -382,6 +392,7 @@ MXS_UPSTREAM* filter_upstream(const SFilterDef& filter, MXS_FILTER_SESSION *fses json_t* filter_parameters_to_json(const SFilterDef& filter) { + ss_dassert(filter); json_t* rval = json_object(); /** Add custom module parameters */ @@ -393,6 +404,7 @@ json_t* filter_parameters_to_json(const SFilterDef& filter) json_t* filter_json_data(const SFilterDef& filter, const char* host) { + ss_dassert(filter); json_t* rval = json_object(); json_object_set_new(rval, CN_ID, json_string(filter->name.c_str())); @@ -426,6 +438,7 @@ json_t* filter_json_data(const SFilterDef& filter, const char* host) json_t* filter_to_json(const SFilterDef& filter, const char* host) { + ss_dassert(filter); string self = MXS_JSON_API_FILTERS; self += filter->name; return mxs_json_resource(host, self.c_str(), filter_json_data(filter, host)); @@ -503,6 +516,7 @@ json_t* FilterSession::diagnostics_json() const static bool create_filter_config(const SFilterDef& filter, const char *filename) { + ss_dassert(filter); int file = open(filename, O_EXCL | O_CREAT | O_WRONLY, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH); if (file == -1) @@ -535,6 +549,7 @@ static bool create_filter_config(const SFilterDef& filter, const char *filename) bool filter_serialize(const SFilterDef& filter) { + ss_dassert(filter); bool rval = false; char filename[PATH_MAX]; snprintf(filename, sizeof(filename), "%s/%s.cnf.tmp", get_config_persistdir(), diff --git a/server/core/internal/service.hh b/server/core/internal/service.hh index 25880d216..d9dd5d5fb 100644 --- a/server/core/internal/service.hh +++ b/server/core/internal/service.hh @@ -15,6 +15,10 @@ #include #include +#include + +#include "filter.hh" + /** * @file service.h - MaxScale internal service functions */ @@ -25,11 +29,13 @@ * These functions should only be called by the MaxScale core. */ -// The internal service representation. Currently it only inherits the SERVICE struct. +// The internal service representation class Service: public SERVICE { public: ~Service(); + + std::vector filters; /**< Ordered list of filters */ }; /** @@ -161,7 +167,7 @@ bool service_server_in_use(const SERVER *server); * * @return True if at least one service uses the filter */ -bool service_filter_in_use(const MXS_FILTER_DEF *filter); +bool service_filter_in_use(const SFilterDef& filter); /** Update the server weights used by services */ void service_update_weights(); @@ -353,7 +359,7 @@ json_t* service_relations_to_server(const SERVER* server, const char* host); * * @return Array of service links */ -json_t* service_relations_to_filter(const MXS_FILTER_DEF* filter, const char* host); +json_t* service_relations_to_filter(const SFilterDef& filter, const char* host); std::unique_ptr serviceGetList(void); std::unique_ptr serviceGetListenerList(void); diff --git a/server/core/modulecmd.cc b/server/core/modulecmd.cc index feaaaaf80..0764e8520 100644 --- a/server/core/modulecmd.cc +++ b/server/core/modulecmd.cc @@ -340,8 +340,9 @@ static bool process_argument(const MODULECMD *cmd, modulecmd_arg_type_t *type, c break; case MODULECMD_ARG_FILTER: - if ((arg->value.filter = filter_def_find((char*)value))) + if (auto f = filter_find((char*)value)) { + arg->value.filter = f.get(); const char* orig_name = filter_def_get_module_name(arg->value.filter); const char* eff_name = mxs_module_get_effective_name(orig_name); if (MODULECMD_ALLOW_NAME_MISMATCH(type) || strcasecmp(cmd->domain, eff_name) == 0) diff --git a/server/core/resource.cc b/server/core/resource.cc index 1d7e98b6c..ab2adb20c 100644 --- a/server/core/resource.cc +++ b/server/core/resource.cc @@ -104,7 +104,7 @@ bool Resource::matching_variable_path(const string& path, const string& target) { if ((path == ":service" && service_find(target.c_str())) || (path == ":server" && server_find_by_unique_name(target.c_str())) || - (path == ":filter" && filter_def_find(target.c_str())) || + (path == ":filter" && filter_find(target.c_str())) || (path == ":monitor" && monitor_find(target.c_str())) || (path == ":module" && get_module(target.c_str(), NULL)) || (path == ":inetuser" && admin_inet_user_exists(target.c_str())) || @@ -486,7 +486,7 @@ HttpResponse cb_delete_service(const HttpRequest& request) HttpResponse cb_delete_filter(const HttpRequest& request) { - FilterDef* filter = filter_find(request.uri_part(1).c_str()); + auto filter = filter_find(request.uri_part(1).c_str()); ss_dassert(filter); if (runtime_destroy_filter(filter)) @@ -549,7 +549,7 @@ HttpResponse cb_all_filters(const HttpRequest& request) HttpResponse cb_get_filter(const HttpRequest& request) { - FilterDef* filter = filter_find(request.uri_part(1).c_str()); + auto filter = filter_find(request.uri_part(1).c_str()); ss_dassert(filter); return HttpResponse(MHD_HTTP_OK, filter_to_json(filter, request.host())); } diff --git a/server/core/service.cc b/server/core/service.cc index 45bcc0f0a..6ff45936e 100644 --- a/server/core/service.cc +++ b/server/core/service.cc @@ -118,8 +118,6 @@ Service* service_alloc(const char *name, const char *router, MXS_CONFIG_PARAMETE service->ports = NULL; service->dbref = NULL; service->n_dbref = 0; - service->filters = NULL; - service->n_filters = 0; service->weightby[0] = '\0'; spinlock_init(&service->spin); @@ -211,7 +209,6 @@ Service::~Service() config_parameter_free(svc_config_param); MXS_FREE(name); MXS_FREE(routerModule); - MXS_FREE(filters); MXS_FREE(rate_limits); } @@ -1234,18 +1231,18 @@ void service_set_retry_interval(Service *service, int value) bool service_set_filters(Service* service, const char* filters) { bool rval = true; - std::vector flist; + std::vector flist; uint64_t capabilities = 0; for (auto& f: mxs::strtok(filters, "|")) { fix_object_name(f); - if (FilterDef* def = filter_find(f.c_str())) + if (auto def = filter_find(f.c_str())) { flist.push_back(def); - const MXS_MODULE* module = get_module(def->module, MODULE_FILTER); + const MXS_MODULE* module = get_module(def->module.c_str(), MODULE_FILTER); ss_dassert(module); capabilities |= module->module_capabilities; @@ -1263,10 +1260,7 @@ bool service_set_filters(Service* service, const char* filters) if (rval) { - service->filters = (MXS_FILTER_DEF**)MXS_MALLOC((flist.size() + 1) * sizeof(MXS_FILTER_DEF*)); - std::copy(flist.begin(), flist.end(), service->filters); - service->n_filters = flist.size(); - service->filters[service->n_filters] = NULL; + service->filters = flist; service->capabilities |= capabilities; } @@ -1362,13 +1356,14 @@ void dprintService(DCB *dcb, SERVICE *svc) asctime_r(localtime_r(&service->stats.started, &result), timebuf)); dcb_printf(dcb, "\tRoot user access: %s\n", service->enable_root ? "Enabled" : "Disabled"); - if (service->n_filters) + if (!service->filters.empty()) { dcb_printf(dcb, "\tFilter chain: "); - for (i = 0; i < service->n_filters; i++) + const char* sep = ""; + for (const auto& f : service->filters) { - dcb_printf(dcb, "%s %s ", filter_def_get_name(service->filters[i]), - i + 1 < service->n_filters ? "|" : ""); + dcb_printf(dcb, "%s %s ", f->name.c_str(), sep); + sep = "|"; } dcb_printf(dcb, "\n"); } @@ -1930,23 +1925,21 @@ bool service_server_in_use(const SERVER *server) return rval; } -bool service_filter_in_use(const MXS_FILTER_DEF *filter) +bool service_filter_in_use(const SFilterDef& filter) { ss_dassert(filter); bool rval = false; spinlock_acquire(&service_spin); - for (Service *service: allServices) + for (Service *service : allServices) { spinlock_acquire(&service->spin); - for (int i = 0; i < service->n_filters; i++) + + if (std::find(service->filters.begin(), + service->filters.end(), filter) != service->filters.end()) { - if (service->filters[i] == filter) - { - rval = true; - break; - } + rval = true; } spinlock_release(&service->spin); @@ -2315,18 +2308,20 @@ json_t* service_attributes(const SERVICE* service) return attr; } -json_t* service_relationships(const SERVICE* service, const char* host) +json_t* service_relationships(const SERVICE* svc, const char* host) { + const Service* service = static_cast(svc); + /** Store relationships to other objects */ json_t* rel = json_object(); - if (service->n_filters) + if (!service->filters.empty()) { json_t* filters = mxs_json_relationship(host, MXS_JSON_API_FILTERS); - for (int i = 0; i < service->n_filters; i++) + for (const auto& f : service->filters) { - mxs_json_add_relation(filters, filter_def_get_name(service->filters[i]), CN_FILTERS); + mxs_json_add_relation(filters, f->name.c_str(), CN_FILTERS); } json_object_set_new(rel, CN_FILTERS, filters); @@ -2418,7 +2413,7 @@ json_t* service_list_to_json(const char* host) return mxs_json_resource(host, MXS_JSON_API_SERVICES, arr); } -json_t* service_relations_to_filter(const MXS_FILTER_DEF* filter, const char* host) +json_t* service_relations_to_filter(const SFilterDef& filter, const char* host) { json_t* rel = mxs_json_relationship(host, MXS_JSON_API_SERVICES); @@ -2428,9 +2423,9 @@ json_t* service_relations_to_filter(const MXS_FILTER_DEF* filter, const char* ho { spinlock_acquire(&service->spin); - for (int i = 0; i < service->n_filters; i++) + for (const auto& f : service->filters) { - if (service->filters[i] == filter) + if (f == filter) { mxs_json_add_relation(rel, service->name, CN_SERVICES); } diff --git a/server/core/session.cc b/server/core/session.cc index feb9b065e..76605d7a4 100644 --- a/server/core/session.cc +++ b/server/core/session.cc @@ -199,7 +199,6 @@ static MXS_SESSION* session_alloc_body(SERVICE* service, DCB* client_dcb, session->tail.clientReply = session_reply; if (SESSION_STATE_TO_BE_FREED != session->state - && service->n_filters > 0 && !session_setup_filters(session)) { session->state = SESSION_STATE_TO_BE_FREED; @@ -561,7 +560,7 @@ dprintSession(DCB *dcb, MXS_SESSION *print_session) for (i = 0; i < print_session->n_filters; i++) { FilterDef* filter = static_cast(print_session->filters[i].filter); - dcb_printf(dcb, "\tFilter: %s\n", filter->name); + dcb_printf(dcb, "\tFilter: %s\n", filter->name.c_str()); filter->obj->diagnostics(print_session->filters[i].instance, print_session->filters[i].session, dcb); } @@ -653,52 +652,52 @@ session_state(mxs_session_state_t state) static int session_setup_filters(MXS_SESSION *session) { - SERVICE *service = session->service; + Service* service = static_cast(session->service); MXS_DOWNSTREAM *head; MXS_UPSTREAM *tail; - int i; + int i = 0; - if ((session->filters = (SESSION_FILTER*)MXS_CALLOC(service->n_filters, + if (service->filters.empty()) + { + return 1; + } + + if ((session->filters = (SESSION_FILTER*)MXS_CALLOC(service->filters.size(), sizeof(SESSION_FILTER))) == NULL) { return 0; } - session->n_filters = service->n_filters; - for (i = service->n_filters - 1; i >= 0; i--) + + session->n_filters = service->filters.size(); + + for (auto r = service->filters.rbegin(); r != service->filters.rend(); r++) { - if (service->filters[i] == NULL) + if ((head = filter_apply(*r, session, &session->head)) == NULL) { - MXS_ERROR("Service '%s' contians an unresolved filter.", service->name); + MXS_ERROR("Failed to create filter '%s' for service '%s'.\n", + filter_def_get_name(r->get()), service->name); return 0; } - if ((head = filter_apply((FilterDef*)service->filters[i], session, - &session->head)) == NULL) - { - MXS_ERROR("Failed to create filter '%s' for " - "service '%s'.\n", - filter_def_get_name(service->filters[i]), - service->name); - return 0; - } - session->filters[i].filter = service->filters[i]; + session->filters[i].filter = r->get(); session->filters[i].session = head->session; session->filters[i].instance = head->instance; + i++; session->head = *head; MXS_FREE(head); } - for (i = 0; i < service->n_filters; i++) + for (auto r = service->filters.begin(); r != service->filters.end(); r++) { - if ((tail = filter_upstream((FilterDef*)service->filters[i], - session->filters[i].session, - &session->tail)) == NULL) + if ((tail = filter_upstream(*r, session->filters[i].session, &session->tail)) == NULL) { MXS_ERROR("Failed to create filter '%s' for service '%s'.", - filter_def_get_name(service->filters[i]), + filter_def_get_name(r->get()), service->name); return 0; } + i--; + /* * filter_upstream may simply return the 3 parameter if * the filter has no upstream entry point. So no need diff --git a/server/core/test/test_filter.cc b/server/core/test/test_filter.cc index 0fe6535f4..d18e6bc35 100644 --- a/server/core/test/test_filter.cc +++ b/server/core/test/test_filter.cc @@ -46,7 +46,7 @@ static int test1() { - FilterDef *f1, *f2; + SFilterDef f1, f2; if ((f1 = filter_alloc("test1", "qlafilter", NULL)) == NULL) { @@ -79,16 +79,13 @@ test1() static int test2() { - FilterDef *f1; + SFilterDef f1; if ((f1 = filter_alloc("test1", "qlafilter", NULL)) == NULL) { fprintf(stderr, "filter_alloc: test 1 failed.\n"); return 1; } - filter_add_parameter(f1, "name1", "value1"); - filter_add_parameter(f1, "name2", "value2"); - filter_add_parameter(f1, "name3", "value3"); filter_free(f1); return 0; @@ -102,7 +99,7 @@ test2() static int test3() { - FilterDef *f1; + SFilterDef f1; char name[40]; int i, n_filters = 1000; @@ -119,7 +116,7 @@ test3() for (i = 0; i < n_filters; i++) { sprintf(name, "filter%d", i); - if ((f1 = filter_find(name)) == NULL) + if (!(f1 = filter_find(name))) { fprintf(stderr, "filter_find: test 3 failed.\n"); return 1; @@ -128,13 +125,13 @@ test3() for (i = 0; i < n_filters; i++) { sprintf(name, "filter%d", i); - if ((f1 = filter_find(name)) == NULL) + if (!(f1 = filter_find(name))) { fprintf(stderr, "filter_find: test 3 failed.\n"); return 1; } filter_free(f1); - if ((f1 = filter_find(name)) != NULL) + if ((f1 = filter_find(name))) { fprintf(stderr, "filter_find: test 3 failed - found deleted filter.\n"); diff --git a/server/modules/filter/tee/teesession.cc b/server/modules/filter/tee/teesession.cc index e0fe708f4..ee97119e4 100644 --- a/server/modules/filter/tee/teesession.cc +++ b/server/modules/filter/tee/teesession.cc @@ -19,46 +19,6 @@ #include -/** - * Detect loops in the filter chain. - */ -bool recursive_tee_usage(std::set& services, SERVICE* service) -{ - if (!services.insert(service->name).second) - { - /** The service name was already in the set */ - return true; - } - - for (int i = 0; i < service->n_filters; i++) - { - const char* module = filter_def_get_module_name(service->filters[i]); - - if (strcmp(module, "tee") == 0) - { - /* - * Found a Tee filter, recurse down its path - * if the service name isn't already in the hashtable. - */ - Tee* inst = (Tee*)filter_def_get_instance(service->filters[i]); - - if (inst == NULL) - { - /** - * This tee instance hasn't been initialized yet and full - * resolution of recursion cannot be done now. - */ - } - else if (recursive_tee_usage(services, inst->get_service())) - { - return true; - } - } - } - - return false; -} - TeeSession::TeeSession(MXS_SESSION* session, LocalClient* client, pcre2_code* match, pcre2_match_data* md_match, pcre2_code* exclude, pcre2_match_data* md_exclude): @@ -73,15 +33,6 @@ TeeSession::TeeSession(MXS_SESSION* session, LocalClient* client, TeeSession* TeeSession::create(Tee* my_instance, MXS_SESSION* session) { - std::set services; - - if (recursive_tee_usage(services, my_instance->get_service())) - { - MXS_ERROR("%s: Recursive use of tee filter in service.", - session->service->name); - return NULL; - } - LocalClient* client = NULL; pcre2_code* match = NULL; pcre2_code* exclude = NULL; diff --git a/server/modules/routing/debugcli/debugcmd.cc b/server/modules/routing/debugcli/debugcmd.cc index 5ee8d87a5..124b6a96b 100644 --- a/server/modules/routing/debugcli/debugcmd.cc +++ b/server/modules/routing/debugcli/debugcmd.cc @@ -1850,9 +1850,13 @@ convert_arg(char *arg, int arg_type) break; case ARG_TYPE_FILTER: + { fix_object_name(arg); - rval = (unsigned long)filter_def_find(arg); + auto f = filter_find(arg); + // This will cause problems in the long run + rval = (unsigned long) (f ? f.get() : NULL); break; + } case ARG_TYPE_NUMERIC: