From 35ab911d5c54b866a3c1a7bc4ddd57e9b2c6d69f Mon Sep 17 00:00:00 2001 From: Esa Korhonen Date: Tue, 5 Feb 2019 18:06:30 +0200 Subject: [PATCH] MXS-2304 Use configuration class methods instead of separate implementations Replaces parameter add/set/remove/free. --- include/maxscale/config.hh | 2 + server/core/config.cc | 95 ++++--------------- server/core/config_runtime.cc | 12 +-- server/core/filter.cc | 20 +--- server/core/internal/config.hh | 2 - server/core/internal/filter.hh | 1 - server/core/internal/monitor.hh | 1 - server/core/monitor.cc | 30 +----- server/core/service.cc | 54 +---------- server/core/test/test_config.cc | 14 +-- server/modules/routing/avrorouter/avro.cc | 15 +-- .../routing/binlogrouter/test/testbinlog.cc | 2 +- 12 files changed, 44 insertions(+), 204 deletions(-) diff --git a/include/maxscale/config.hh b/include/maxscale/config.hh index 5cd07fce3..e657d09c4 100644 --- a/include/maxscale/config.hh +++ b/include/maxscale/config.hh @@ -345,6 +345,8 @@ public: */ static void remove(MXS_CONFIG_PARAMETER** ppParams, const std::string& key); + static void free_all(MXS_CONFIG_PARAMETER** ppParams); + char* name {nullptr}; /**< The name of the parameter */ char* value {nullptr}; /**< The value of the parameter */ MXS_CONFIG_PARAMETER* next {nullptr}; /**< Next pointer in the linked list */ diff --git a/server/core/config.cc b/server/core/config.cc index a13df28ca..fe9086192 100644 --- a/server/core/config.cc +++ b/server/core/config.cc @@ -1908,16 +1908,6 @@ int64_t MXS_CONFIG_PARAMETER::get_integer(const std::string& key) const return value.empty() ? 0 : strtoll(value.c_str(), NULL, 10); } -MXS_CONFIG_PARAMETER* config_clone_param(const MXS_CONFIG_PARAMETER* param) -{ - auto p2 = new MXS_CONFIG_PARAMETER; - p2->name = MXS_STRDUP_A(param->name); - p2->value = MXS_STRDUP_A(param->value); - p2->next = nullptr; - - return p2; -} - void config_free_one_param(MXS_CONFIG_PARAMETER* p1) { if (p1) @@ -1926,20 +1916,6 @@ void config_free_one_param(MXS_CONFIG_PARAMETER* p1) } } -/** - * Free a configuration parameter - * @param p1 Parameter to free - */ -void config_parameter_free(MXS_CONFIG_PARAMETER* p1) -{ - while (p1) - { - MXS_CONFIG_PARAMETER* tmp = p1; - p1 = p1->next; - config_free_one_param(tmp); - } -} - void config_context_free(CONFIG_CONTEXT* context) { CONFIG_CONTEXT* obj; @@ -1947,7 +1923,7 @@ void config_context_free(CONFIG_CONTEXT* context) while (context) { obj = context->next; - config_parameter_free(context->parameters); + MXS_CONFIG_PARAMETER::free_all(&context->parameters); MXS_FREE(context->object); MXS_FREE(context); context = obj; @@ -1957,27 +1933,8 @@ void config_context_free(CONFIG_CONTEXT* context) bool config_add_param(CONFIG_CONTEXT* obj, const char* key, const char* value) { mxb_assert(!obj->parameters->contains(key)); - bool rval = false; - char* my_key = MXS_STRDUP(key); - char* my_value = MXS_STRDUP(value); - auto param = new MXS_CONFIG_PARAMETER; - - if (my_key && my_value) - { - param->name = my_key; - param->value = my_value; - param->next = obj->parameters; - obj->parameters = param; - rval = true; - } - else - { - MXS_FREE(my_key); - MXS_FREE(my_value); - delete param; - } - - return rval; + MXS_CONFIG_PARAMETER::set(&obj->parameters, key, value); + return true; } bool config_append_param(CONFIG_CONTEXT* obj, const char* key, const char* value) @@ -2085,6 +2042,19 @@ void MXS_CONFIG_PARAMETER::remove(MXS_CONFIG_PARAMETER** ppParams, const string& } } +void MXS_CONFIG_PARAMETER::free_all(MXS_CONFIG_PARAMETER** ppParams) +{ + mxb_assert(ppParams); + auto pParam = *ppParams; + while (pParam) + { + auto temp = pParam; + pParam = pParam->next; + delete temp; + } + *ppParams = nullptr; +} + MXS_CONFIG_PARAMETER* MXS_CONFIG_PARAMETER::find(const std::string& key) { auto param = this; @@ -2101,40 +2071,13 @@ MXS_CONFIG_PARAMETER* MXS_CONFIG_PARAMETER::find(const std::string& key) bool config_replace_param(CONFIG_CONTEXT* obj, const char* key, const char* value) { - if (MXS_CONFIG_PARAMETER* param = config_get_param(obj->parameters, key)) - { - MXS_FREE(param->value); - param->value = MXS_STRDUP(value); - } - else - { - config_add_param(obj, key, value); - } - + MXS_CONFIG_PARAMETER::set(&obj->parameters, key, value); return true; } void config_remove_param(CONFIG_CONTEXT* obj, const char* name) { - if (obj->parameters && strcmp(obj->parameters->name, name) == 0) - { - MXS_CONFIG_PARAMETER* tmp = obj->parameters; - obj->parameters = obj->parameters->next; - config_free_one_param(tmp); - } - else - { - for (MXS_CONFIG_PARAMETER* p = obj->parameters; p && p->next; p = p->next) - { - if (strcmp(p->next->name, name) == 0) - { - MXS_CONFIG_PARAMETER* tmp = p->next; - p->next = tmp->next; - config_free_one_param(tmp); - break; - } - } - } + MXS_CONFIG_PARAMETER::remove(&obj->parameters, name); } /** @@ -4931,7 +4874,7 @@ ParamList::ParamList(std::initializer_list> ParamList::~ParamList() { - config_parameter_free(m_ctx.parameters); + MXS_CONFIG_PARAMETER::free_all(&m_ctx.parameters); } MXS_CONFIG_PARAMETER* ParamList::params() diff --git a/server/core/config_runtime.cc b/server/core/config_runtime.cc index 789cfaf88..442c7b388 100644 --- a/server/core/config_runtime.cc +++ b/server/core/config_runtime.cc @@ -262,7 +262,7 @@ bool runtime_create_server(const char* name, name); } - config_parameter_free(ctx.parameters); + MXS_CONFIG_PARAMETER::free_all(&ctx.parameters); } else { @@ -1201,7 +1201,7 @@ bool runtime_create_monitor(const char* name, const char* module) config_runtime_error("Could not create monitor '%s' with module '%s'", name, module); } - config_parameter_free(params); + MXS_CONFIG_PARAMETER::free_all(¶ms); } } else @@ -1258,7 +1258,7 @@ bool runtime_create_filter(const char* name, const char* module, MXS_CONFIG_PARA config_runtime_error("Could not create filter '%s' with module '%s'", name, module); } - config_parameter_free(ctx.parameters); + MXS_CONFIG_PARAMETER::free_all(&ctx.parameters); } else { @@ -1335,7 +1335,7 @@ static bool runtime_create_service(const char* name, const char* router, MXS_CON config_runtime_error("Could not create service '%s' with module '%s'", name, router); } - config_parameter_free(ctx.parameters); + MXS_CONFIG_PARAMETER::free_all(&ctx.parameters); } else { @@ -2264,7 +2264,7 @@ bool runtime_create_filter_from_json(json_t* json) rval = runtime_create_filter(name, module, params); - config_parameter_free(params); + MXS_CONFIG_PARAMETER::free_all(¶ms); } return rval; @@ -2298,7 +2298,7 @@ Service* runtime_create_service_from_json(json_t* json) } } - config_parameter_free(params); + MXS_CONFIG_PARAMETER::free_all(¶ms); } return rval; diff --git a/server/core/filter.cc b/server/core/filter.cc index 4cabc175d..32f83e657 100644 --- a/server/core/filter.cc +++ b/server/core/filter.cc @@ -59,7 +59,7 @@ static struct */ static void filter_free_parameters(FilterDef* filter) { - config_parameter_free(filter->parameters); + MXS_CONFIG_PARAMETER::free_all(&filter->parameters); } /** @@ -298,24 +298,6 @@ void dListFilters(DCB* dcb) } } -/** - * Add a router parameter to a service - * - * @param filter The filter to add the parameter to - * @param name The parameter name - * @param value The parameter value - */ -void filter_add_parameter(SFilterDef& filter, const char* name, const char* value) -{ - mxb_assert(filter); - CONFIG_CONTEXT ctx = {}; - ctx.object = (char*)""; - - config_add_param(&ctx, name, value); - ctx.parameters->next = filter->parameters; - filter->parameters = ctx.parameters; -} - /** * Connect the downstream filter chain for a filter. * diff --git a/server/core/internal/config.hh b/server/core/internal/config.hh index 16fa46292..f744492c9 100644 --- a/server/core/internal/config.hh +++ b/server/core/internal/config.hh @@ -75,10 +75,8 @@ void config_set_global_defaults(); void config_add_defaults(CONFIG_CONTEXT* ctx, const MXS_MODULE_PARAM* params); char* config_clean_string_list(const char* str); -MXS_CONFIG_PARAMETER* config_clone_param(const MXS_CONFIG_PARAMETER* param); bool config_load(const char*); bool config_load_global(const char* filename); -void config_parameter_free(MXS_CONFIG_PARAMETER* p1); /** * @brief Creates an empty configuration context diff --git a/server/core/internal/filter.hh b/server/core/internal/filter.hh index 043b2f7d5..d313d9e41 100644 --- a/server/core/internal/filter.hh +++ b/server/core/internal/filter.hh @@ -46,7 +46,6 @@ struct FilterDef : public MXS_FILTER_DEF 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); diff --git a/server/core/internal/monitor.hh b/server/core/internal/monitor.hh index 81ce9a744..950e34410 100644 --- a/server/core/internal/monitor.hh +++ b/server/core/internal/monitor.hh @@ -136,7 +136,6 @@ void monitor_list(DCB*); bool monitor_add_server(Monitor* mon, SERVER* server); void monitor_remove_server(Monitor* mon, SERVER* server); -void monitor_add_parameters(Monitor* monitor, const MXS_CONFIG_PARAMETER* params); void monitor_set_journal_max_age(Monitor* mon, time_t value); diff --git a/server/core/monitor.cc b/server/core/monitor.cc index 71378c918..324b5e72b 100644 --- a/server/core/monitor.cc +++ b/server/core/monitor.cc @@ -231,7 +231,7 @@ bool Monitor::configure_base(const MXS_CONFIG_PARAMETER* params) Monitor::~Monitor() { - config_parameter_free(parameters); + MXS_CONFIG_PARAMETER::free_all(¶meters); monitor_server_free_all(m_servers); MXS_FREE((const_cast(m_name))); } @@ -741,34 +741,6 @@ bool Monitor::test_permissions(const string& query) return rval; } -/** - * Add parameters to the monitor - * @param monitor Monitor - * @param params Config parameters - */ -void monitor_add_parameters(Monitor* monitor, const MXS_CONFIG_PARAMETER* params) -{ - Guard guard(monitor->m_lock); - while (params) - { - MXS_CONFIG_PARAMETER* old = config_get_param(monitor->parameters, params->name); - - if (old) - { - MXS_FREE(old->value); - old->value = MXS_STRDUP_A(params->value); - } - else - { - MXS_CONFIG_PARAMETER* clone = config_clone_param(params); - clone->next = monitor->parameters; - monitor->parameters = clone; - } - - params = params->next; - } -} - void monitor_stash_current_status(MXS_MONITORED_SERVER* ptr) { ptr->mon_prev_status = ptr->server->status; diff --git a/server/core/service.cc b/server/core/service.cc index fd0fdfc30..1c04aed65 100644 --- a/server/core/service.cc +++ b/server/core/service.cc @@ -246,7 +246,7 @@ Service::~Service() MXS_FREE(tmp); } - config_parameter_free(svc_config_param); + MXS_CONFIG_PARAMETER::free_all(&svc_config_param); } void service_free(Service* service) @@ -1122,13 +1122,7 @@ int service_refresh_users(SERVICE* svc) void service_add_parameters(Service* service, const MXS_CONFIG_PARAMETER* param) { - while (param) - { - MXS_CONFIG_PARAMETER* new_param = config_clone_param(param); - new_param->next = service->svc_config_param; - service->svc_config_param = new_param; - param = param->next; - } + MXS_CONFIG_PARAMETER::set_multiple(&service->svc_config_param, param); } void service_add_parameter(Service* service, const char* key, const char* value) @@ -1141,52 +1135,12 @@ void service_add_parameter(Service* service, const char* key, const char* value) void service_remove_parameter(Service* service, const char* key) { - if (MXS_CONFIG_PARAMETER* params = service->svc_config_param) - { - MXS_CONFIG_PARAMETER* to_free = NULL; - - if (strcasecmp(params->name, key) == 0) - { - service->svc_config_param = params->next; - to_free = params; - } - else - { - while (MXS_CONFIG_PARAMETER* p = params->next) - { - if (strcasecmp(p->name, key) == 0) - { - params->next = p->next; - to_free = p; - break; - } - - params = p; - } - } - - if (to_free) - { - // Set next pointer to null to prevent freeing of other parameters - to_free->next = NULL; - config_parameter_free(to_free); - } - } + MXS_CONFIG_PARAMETER::remove(&service->svc_config_param, key); } void service_replace_parameter(Service* service, const char* key, const char* value) { - for (MXS_CONFIG_PARAMETER* p = service->svc_config_param; p; p = p->next) - { - if (strcasecmp(p->name, key) == 0) - { - MXS_FREE(p->value); - p->value = MXS_STRDUP_A(value); - return; - } - } - - service_add_parameter(service, key, value); + MXS_CONFIG_PARAMETER::set(&service->svc_config_param, key, value); } /** diff --git a/server/core/test/test_config.cc b/server/core/test/test_config.cc index bb89adea1..6ea25b904 100644 --- a/server/core/test/test_config.cc +++ b/server/core/test/test_config.cc @@ -116,7 +116,7 @@ int test_validity() TEST(!config_param_is_valid(params, "p8", "d", &ctx)); TEST(!config_param_is_valid(params, "p8", "a,d", &ctx)); TEST(!config_param_is_valid(params, "p8", "a,b,c,d", &ctx)); - config_parameter_free(svc.parameters); + MXS_CONFIG_PARAMETER::free_all(&svc.parameters); return 0; } @@ -163,7 +163,7 @@ int test_add_parameter() TEST(ctx.parameters->get_string("p6") == "/tmp"); TEST(ctx.parameters->get_string("p7") == "my-service"); - config_parameter_free(ctx.parameters); + MXS_CONFIG_PARAMETER::free_all(&ctx.parameters); ctx.parameters = NULL; /** Test custom parameters overriding default values */ @@ -185,9 +185,9 @@ int test_add_parameter() TEST(val == 5); TEST(ctx.parameters->get_string("p6") == "/dev/null"); TEST(ctx.parameters->get_string("p7") == "some-service"); - config_parameter_free(ctx.parameters); - config_parameter_free(svc1.parameters); - config_parameter_free(svc2.parameters); + MXS_CONFIG_PARAMETER::free_all(&ctx.parameters); + MXS_CONFIG_PARAMETER::free_all(&svc1.parameters); + MXS_CONFIG_PARAMETER::free_all(&svc2.parameters); return 0; } @@ -208,14 +208,14 @@ int test_required_parameters() config_add_defaults(&ctx, params); TEST(!missing_required_parameters(params, ctx.parameters, "test")); - config_parameter_free(ctx.parameters); + MXS_CONFIG_PARAMETER::free_all(&ctx.parameters); ctx.parameters = NULL; config_add_param(&ctx, "p1", "1"); config_add_param(&ctx, "p2", "1"); config_add_param(&ctx, "p3", "1"); TEST(!missing_required_parameters(params, ctx.parameters, "test")); - config_parameter_free(ctx.parameters); + MXS_CONFIG_PARAMETER::free_all(&ctx.parameters); return 0; } diff --git a/server/modules/routing/avrorouter/avro.cc b/server/modules/routing/avrorouter/avro.cc index 299f990d1..694e86cc9 100644 --- a/server/modules/routing/avrorouter/avro.cc +++ b/server/modules/routing/avrorouter/avro.cc @@ -62,18 +62,9 @@ using namespace maxscale; void Avro::read_source_service_options(SERVICE* source) { MXS_CONFIG_PARAMETER* params = source->svc_config_param; - - for (MXS_CONFIG_PARAMETER* p = params; p; p = p->next) - { - if (strcmp(p->name, "binlogdir") == 0) - { - binlogdir = p->value; - } - else if (strcmp(p->name, "filestem") == 0) - { - filestem = p->value; - } - } + binlogdir = params->get_string("binlogdir"); + filestem = params->get_string("filestem"); + mxb_assert(!binlogdir.empty() && !filestem.empty()); for (const auto& opt : mxs::strtok(params->get_string("router_options"), ", \t")) { diff --git a/server/modules/routing/binlogrouter/test/testbinlog.cc b/server/modules/routing/binlogrouter/test/testbinlog.cc index c46c52687..a67e2170d 100644 --- a/server/modules/routing/binlogrouter/test/testbinlog.cc +++ b/server/modules/routing/binlogrouter/test/testbinlog.cc @@ -133,7 +133,7 @@ int main(int argc, char** argv) return 1; } - config_parameter_free(ctx.parameters); + MXS_CONFIG_PARAMETER::free_all(&ctx.parameters); // Declared in config.cc and needs to be removed if/when blr is refactored extern const MXS_MODULE_PARAM config_server_params[];