From 5ab7734e9db2d0a699f1f234614ba85519d21984 Mon Sep 17 00:00:00 2001 From: Esa Korhonen Date: Fri, 1 Feb 2019 18:34:27 +0200 Subject: [PATCH] MXS-2304 Add contains() to test if a parameter exists Replaces uses of config_get_param() in modules either with contains() or get_string(). The config_get_param() is moved to internal headers, as it allows seeing inside a config setting. --- include/maxscale/config.hh | 17 +++--- server/core/config.cc | 54 ++++++++++--------- server/core/internal/config.hh | 9 ++++ server/core/service.cc | 2 +- server/core/test/test_config.cc | 2 +- server/modules/filter/cache/cachefilter.cc | 7 ++- server/modules/filter/mqfilter/mqfilter.cc | 24 ++++----- server/modules/routing/avrorouter/avro.cc | 8 +-- .../routing/readwritesplit/readwritesplit.cc | 9 ++-- .../routing/schemarouter/schemarouter.cc | 5 +- .../schemarouter/schemarouterinstance.cc | 2 +- 11 files changed, 75 insertions(+), 64 deletions(-) diff --git a/include/maxscale/config.hh b/include/maxscale/config.hh index 82985f84d..20bd3e8c1 100644 --- a/include/maxscale/config.hh +++ b/include/maxscale/config.hh @@ -318,6 +318,14 @@ public: */ SERVER* get_server(const std::string& key) const; + /** + * Check if a key exists. + * + * @param key Parameter name + * @return True if key was found + */ + bool contains(const std::string& key) const; + char* name; /**< The name of the parameter */ char* value; /**< The value of the parameter */ MXS_CONFIG_PARAMETER* next; /**< Next pointer in the linked list */ @@ -396,15 +404,6 @@ struct MXS_CONFIG */ MXS_CONFIG* config_get_global_options(); -/** - * @brief Get a configuration parameter - * - * @param params List of parameters - * @param name Name of parameter to get - * @return The parameter or NULL if the parameter was not found - */ -MXS_CONFIG_PARAMETER* config_get_param(MXS_CONFIG_PARAMETER* params, const char* name); - /** * @brief Helper function for checking SSL parameters * diff --git a/server/core/config.cc b/server/core/config.cc index 7de982905..976d0cee8 100644 --- a/server/core/config.cc +++ b/server/core/config.cc @@ -760,7 +760,7 @@ static int ini_handler(void* userdata, const char* section, const char* name, co cntxt->next = ptr; } - if (config_get_param(ptr->parameters, name)) + if (ptr->parameters->contains(name)) { /** The values in the persisted configurations are updated versions of * the ones in the main configuration file. */ @@ -1283,27 +1283,18 @@ const char* get_missing_module_parameter_name(const CONFIG_CONTEXT* obj) { std::string type = obj->parameters->get_string(CN_TYPE); - if (type == CN_SERVICE && !config_get_param(obj->parameters, CN_ROUTER)) + if (type == CN_SERVICE && !obj->parameters->contains(CN_ROUTER)) { return CN_ROUTER; } - else if (type == CN_LISTENER && !config_get_param(obj->parameters, CN_PROTOCOL)) + else if ((type == CN_LISTENER || type == CN_SERVER) && !obj->parameters->contains(CN_PROTOCOL)) { return CN_PROTOCOL; } - else if (type == CN_SERVER && !config_get_param(obj->parameters, CN_PROTOCOL)) - { - return CN_PROTOCOL; - } - else if (type == CN_MONITOR && !config_get_param(obj->parameters, CN_MODULE)) + else if ((type == CN_MONITOR || type == CN_FILTER) && !obj->parameters->contains(CN_MODULE)) { return CN_MODULE; } - else if (type == CN_FILTER && !config_get_param(obj->parameters, CN_MODULE)) - { - return CN_MODULE; - } - return nullptr; } @@ -1709,7 +1700,7 @@ void do_passwd_deprecation(CONFIG_CONTEXT* obj) { if (auto p = config_get_param(obj->parameters, "passwd")) { - if (config_get_param(obj->parameters, CN_PASSWORD)) + if (obj->parameters->contains(CN_PASSWORD)) { MXS_WARNING("Both 'password' and 'passwd' specified. Using value of '%s'.", CN_PASSWORD); } @@ -1814,6 +1805,20 @@ SERVER* MXS_CONFIG_PARAMETER::get_server(const std::string& key) const return Server::find_by_unique_name(param_value.c_str()); } +bool MXS_CONFIG_PARAMETER::contains(const string& key) const +{ + auto params = this; + while (params) + { + if (params->name == key) + { + return true; + } + params = params->next; + } + return false; +} + std::vector config_get_server_list(const MXS_CONFIG_PARAMETER* params, const char* key, string* name_error_out) { @@ -1973,7 +1978,7 @@ void config_context_free(CONFIG_CONTEXT* context) bool config_add_param(CONFIG_CONTEXT* obj, const char* key, const char* value) { - mxb_assert(config_get_param(obj->parameters, key) == NULL); + mxb_assert(!obj->parameters->contains(key)); bool rval = false; char* my_key = MXS_STRDUP(key); char* my_value = MXS_STRDUP(value); @@ -2877,8 +2882,7 @@ static bool missing_required_parameters(const MXS_MODULE_PARAM* mod_params, { for (int i = 0; mod_params[i].name; i++) { - if ((mod_params[i].options & MXS_MODULE_OPT_REQUIRED) - && config_get_param(params, mod_params[i].name) == NULL) + if ((mod_params[i].options & MXS_MODULE_OPT_REQUIRED) && !params->contains(mod_params[i].name)) { MXS_ERROR("Mandatory parameter '%s' is not defined for '%s'.", mod_params[i].name, @@ -3571,8 +3575,7 @@ void config_add_defaults(CONFIG_CONTEXT* ctx, const MXS_MODULE_PARAM* params) { for (int i = 0; params[i].name; i++) { - if (params[i].default_value - && config_get_param(ctx->parameters, params[i].name) == NULL) + if (params[i].default_value && !ctx->parameters->contains(params[i].name)) { bool rv = config_add_param(ctx, params[i].name, params[i].default_value); MXS_ABORT_IF_FALSE(rv); @@ -3810,7 +3813,7 @@ int create_new_monitor(CONFIG_CONTEXT* obj, std::set& monitored_ser MXS_CONFIG_PARAMETER* params = obj->parameters; // The config loader has already checked that the server list is mostly ok. However, it cannot // check that the server names in the list actually ended up generated. - if (config_get_param(params, CN_SERVERS)) + if (params->contains(CN_SERVERS)) { string name_not_found; auto servers = config_get_server_list(params, CN_SERVERS, &name_not_found); @@ -3991,12 +3994,11 @@ int create_new_filter(CONFIG_CONTEXT* obj) bool config_have_required_ssl_params(CONFIG_CONTEXT* obj) { MXS_CONFIG_PARAMETER* param = obj->parameters; - - return config_get_param(param, CN_SSL) - && config_get_param(param, CN_SSL_KEY) - && config_get_param(param, CN_SSL_CERT) - && config_get_param(param, CN_SSL_CA_CERT) - && strcmp(config_get_value_string(param, CN_SSL), CN_REQUIRED) == 0; + return param->contains(CN_SSL) + && param->contains(CN_SSL_KEY) + && param->contains(CN_SSL_CERT) + && param->contains(CN_SSL_CA_CERT) + && (param->get_string(CN_SSL) == CN_REQUIRED); } bool config_is_ssl_parameter(const char* key) diff --git a/server/core/internal/config.hh b/server/core/internal/config.hh index 3ea4ef876..16fa46292 100644 --- a/server/core/internal/config.hh +++ b/server/core/internal/config.hh @@ -236,3 +236,12 @@ void dump_param_list(int file, * @return True if the parameter can be modified at runtime */ bool config_can_modify_at_runtime(const char* name); + +/** + * @brief Get a configuration parameter + * + * @param params List of parameters + * @param name Name of parameter to get + * @return The parameter or NULL if the parameter was not found + */ +MXS_CONFIG_PARAMETER* config_get_param(MXS_CONFIG_PARAMETER* params, const char* name); diff --git a/server/core/service.cc b/server/core/service.cc index 2d91c6125..1bc82a25e 100644 --- a/server/core/service.cc +++ b/server/core/service.cc @@ -197,7 +197,7 @@ Service::Service(const std::string& service_name, strip_db_esc = params->get_bool(CN_STRIP_DB_ESC); session_track_trx_state = params->get_bool(CN_SESSION_TRACK_TRX_STATE); - if (config_get_param(params, CN_RETAIN_LAST_STATEMENTS)) + if (params->contains(CN_RETAIN_LAST_STATEMENTS)) { retain_last_statements = params->get_integer(CN_RETAIN_LAST_STATEMENTS); } diff --git a/server/core/test/test_config.cc b/server/core/test/test_config.cc index f4fa97408..bb89adea1 100644 --- a/server/core/test/test_config.cc +++ b/server/core/test/test_config.cc @@ -179,7 +179,7 @@ int test_add_parameter() TEST(ctx.parameters->get_integer("p1") == -321); TEST(ctx.parameters->get_integer("p2") == 321); - TEST(config_get_param(ctx.parameters, "p3") && ctx.parameters->get_bool("p3") == false); + TEST(ctx.parameters->contains("p3") && ctx.parameters->get_bool("p3") == false); TEST(ctx.parameters->get_string("p4") == "strange"); int val = ctx.parameters->get_enum("p5", enum_values); TEST(val == 5); diff --git a/server/modules/filter/cache/cachefilter.cc b/server/modules/filter/cache/cachefilter.cc index 81ab07469..41a1a47ac 100644 --- a/server/modules/filter/cache/cachefilter.cc +++ b/server/modules/filter/cache/cachefilter.cc @@ -377,11 +377,10 @@ bool CacheFilter::process_params(MXS_CONFIG_PARAMETER* ppParams, CACHE_CONFIG& c config.rules = ppParams->get_c_str_copy("rules"); - const MXS_CONFIG_PARAMETER* pParam = config_get_param(ppParams, "storage_options"); - - if (pParam) + string storage_options = ppParams->get_string("storage_options"); + if (!storage_options.empty()) { - config.storage_options = MXS_STRDUP(pParam->value); + config.storage_options = MXS_STRDUP(storage_options.c_str()); if (config.storage_options) { diff --git a/server/modules/filter/mqfilter/mqfilter.cc b/server/modules/filter/mqfilter/mqfilter.cc index 95a5432f7..fab0771d6 100644 --- a/server/modules/filter/mqfilter/mqfilter.cc +++ b/server/modules/filter/mqfilter/mqfilter.cc @@ -618,32 +618,32 @@ static MXS_FILTER* createInstance(const char* name, MXS_CONFIG_PARAMETER* params MXS_ABORT_IF_NULL(my_instance->obj_trg); } - MXS_CONFIG_PARAMETER* p = config_get_param(params, "logging_source_user"); + std::string value = params->get_string("logging_source_user"); - if (p && my_instance->src_trg) + if (!value.empty() && my_instance->src_trg) { - my_instance->src_trg->user = parse_optstr(p->value, ",", &my_instance->src_trg->usize); + my_instance->src_trg->user = parse_optstr(value.c_str(), ",", &my_instance->src_trg->usize); } - p = config_get_param(params, "logging_source_host"); + value = params->get_string("logging_source_host"); - if (p && my_instance->src_trg) + if (!value.empty() && my_instance->src_trg) { - my_instance->src_trg->host = parse_optstr(p->value, ",", &my_instance->src_trg->hsize); + my_instance->src_trg->host = parse_optstr(value.c_str(), ",", &my_instance->src_trg->hsize); } - p = config_get_param(params, "logging_schema"); + value = params->get_string("logging_schema"); - if (p && my_instance->shm_trg) + if (!value.empty() && my_instance->shm_trg) { - my_instance->shm_trg->objects = parse_optstr(p->value, ",", &my_instance->shm_trg->size); + my_instance->shm_trg->objects = parse_optstr(value.c_str(), ",", &my_instance->shm_trg->size); } - p = config_get_param(params, "logging_object"); + value = params->get_string("logging_object"); - if (p && my_instance->obj_trg) + if (!value.empty() && my_instance->obj_trg) { - my_instance->obj_trg->objects = parse_optstr(p->value, ",", &my_instance->obj_trg->size); + my_instance->obj_trg->objects = parse_optstr(value.c_str(), ",", &my_instance->obj_trg->size); } my_instance->use_ssl = my_instance->ssl_client_cert diff --git a/server/modules/routing/avrorouter/avro.cc b/server/modules/routing/avrorouter/avro.cc index 2fef92fef..299f990d1 100644 --- a/server/modules/routing/avrorouter/avro.cc +++ b/server/modules/routing/avrorouter/avro.cc @@ -94,11 +94,11 @@ void Avro::read_source_service_options(SERVICE* source) Avro* Avro::create(SERVICE* service, SRowEventHandler handler) { SERVICE* source_service = NULL; - MXS_CONFIG_PARAMETER* param = config_get_param(service->svc_config_param, "source"); + std::string source_name = service->svc_config_param->get_string("source"); - if (param) + if (!source_name.empty()) { - SERVICE* source = service_find(param->value); + SERVICE* source = service_find(source_name.c_str()); mxb_assert(source); if (source) @@ -119,7 +119,7 @@ Avro* Avro::create(SERVICE* service, SRowEventHandler handler) } else { - MXS_ERROR("Service '%s' not found.", param->value); + MXS_ERROR("Service '%s' not found.", source_name.c_str()); return NULL; } } diff --git a/server/modules/routing/readwritesplit/readwritesplit.cc b/server/modules/routing/readwritesplit/readwritesplit.cc index ebd168cf5..8366a9e0c 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.cc +++ b/server/modules/routing/readwritesplit/readwritesplit.cc @@ -191,11 +191,11 @@ bool RWSplit::have_enough_servers() const return succp; } -static void log_router_options_not_supported(SERVICE* service, MXS_CONFIG_PARAMETER* p) +static void log_router_options_not_supported(SERVICE* service, std::string router_opts) { std::stringstream ss; - for (const auto& a : mxs::strtok(p->value, ", \t")) + for (const auto& a : mxs::strtok(router_opts, ", \t")) { ss << a << "\n"; } @@ -214,9 +214,10 @@ static void log_router_options_not_supported(SERVICE* service, MXS_CONFIG_PARAME RWSplit* RWSplit::create(SERVICE* service, MXS_CONFIG_PARAMETER* params) { - if (MXS_CONFIG_PARAMETER* p = config_get_param(params, CN_ROUTER_OPTIONS)) + + if (params->contains(CN_ROUTER_OPTIONS)) { - log_router_options_not_supported(service, p); + log_router_options_not_supported(service, params->get_string(CN_ROUTER_OPTIONS)); return NULL; } diff --git a/server/modules/routing/schemarouter/schemarouter.cc b/server/modules/routing/schemarouter/schemarouter.cc index 1552be6dd..7967359d6 100644 --- a/server/modules/routing/schemarouter/schemarouter.cc +++ b/server/modules/routing/schemarouter/schemarouter.cc @@ -31,9 +31,10 @@ Config::Config(MXS_CONFIG_PARAMETER* conf) ignored_dbs.insert("performance_schema"); // TODO: Don't process this in the router - if (MXS_CONFIG_PARAMETER* p = config_get_param(conf, "ignore_databases")) + std::string ignored_dbs_str = conf->get_string("ignore_databases"); + if (!ignored_dbs_str.empty()) { - for (const auto& a : mxs::strtok(p->value, ", \t")) + for (const auto& a : mxs::strtok(ignored_dbs_str, ", \t")) { ignored_dbs.insert(a); } diff --git a/server/modules/routing/schemarouter/schemarouterinstance.cc b/server/modules/routing/schemarouter/schemarouterinstance.cc index 635c064e0..62d5c8b17 100644 --- a/server/modules/routing/schemarouter/schemarouterinstance.cc +++ b/server/modules/routing/schemarouter/schemarouterinstance.cc @@ -53,7 +53,7 @@ SchemaRouter::~SchemaRouter() SchemaRouter* SchemaRouter::create(SERVICE* pService, MXS_CONFIG_PARAMETER* params) { - if ((config_get_param(params, "auth_all_servers")) == NULL) + if (!params->contains("auth_all_servers")) { MXS_NOTICE("Authentication data is fetched from all servers. To disable this " "add 'auth_all_servers=0' to the service.");