diff --git a/include/maxscale/config.hh b/include/maxscale/config.hh index 7faedbd1f..3a937b71b 100644 --- a/include/maxscale/config.hh +++ b/include/maxscale/config.hh @@ -451,6 +451,7 @@ public: void remove(const std::string& key); void clear(); + bool empty() const; ContainerType::const_iterator begin() const; ContainerType::const_iterator end() const; diff --git a/server/core/config.cc b/server/core/config.cc index 092a94323..a22a3fd8a 100644 --- a/server/core/config.cc +++ b/server/core/config.cc @@ -2212,6 +2212,11 @@ void MXS_CONFIG_PARAMETER::clear() m_contents.clear(); } +bool MXS_CONFIG_PARAMETER::empty() const +{ + return m_contents.empty(); +} + MXS_CONFIG_PARAMETER::ContainerType::const_iterator MXS_CONFIG_PARAMETER::begin() const { return m_contents.begin(); @@ -3817,34 +3822,24 @@ void config_add_module_params_json(const MXS_CONFIG_PARAMETER* parameters, const MXS_MODULE_PARAM* module_params, json_t* output) { - // Create a map of the config values to ease their extraction - std::unordered_map params; - - for (const auto& elem : *parameters) - { - params[elem.first] = elem.second; - } - for (const auto* param_info : {basic_params, module_params}) { for (int i = 0; param_info[i].name; i++) { - if (ignored_params.count(param_info[i].name) == 0 - && !json_object_get(output, param_info[i].name)) + const string param_name = param_info[i].name; + if (ignored_params.count(param_name) == 0 && !json_object_get(output, param_name.c_str())) { - if (params.count(param_info[i].name) > 0) + if (parameters->contains(param_name)) { - const string name = param_info[i].name; - const string value = params[name]; - json_object_set_new(output, name.c_str(), - param_value_to_json(¶m_info[i], name, - value)); + const string value = parameters->get_string(param_name); + json_object_set_new(output, param_name.c_str(), + param_value_to_json(¶m_info[i], param_name, value)); } else { // The parameter was not set in config and does not have a default value. // Print a null value. - json_object_set_new(output, param_info[i].name, json_null()); + json_object_set_new(output, param_name.c_str(), json_null()); } } } diff --git a/server/core/config_runtime.cc b/server/core/config_runtime.cc index b14c3e92b..08862403c 100644 --- a/server/core/config_runtime.cc +++ b/server/core/config_runtime.cc @@ -576,92 +576,111 @@ bool runtime_alter_server(Server* server, const char* key, const char* value) return false; } - const MXS_MODULE* mod = get_module(server->protocol().c_str(), MODULE_PROTOCOL); + bool is_normal_parameter = !server->is_custom_parameter(key); - // As servers allow unknown parameters, we must only validate known parameters - if (param_is_known(config_server_params, mod->parameters, key) - && !param_is_valid(config_server_params, mod->parameters, key, value)) + // Only validate known parameters as custom parameters cannot be validated. + if (is_normal_parameter) { - config_runtime_error("Invalid value for parameter '%s': %s", key, value); - return false; - } - - if (strcmp(key, CN_DISK_SPACE_THRESHOLD) == 0) - { - // This cannot be safely modified during runtime since the monitor thread could be reading the - // value. TODO: To enable this, the threshold value needs to be moved to the private Server-class - // so that locking can be enforced on any access. - config_runtime_error("The server parameter '%s' cannot be modified during runtime.", - CN_DISK_SPACE_THRESHOLD); - return false; + const MXS_MODULE* mod = get_module(server->protocol().c_str(), MODULE_PROTOCOL); + if (!param_is_valid(config_server_params, mod->parameters, key, value)) + { + config_runtime_error("Invalid value for parameter '%s': %s", key, value); + return false; + } } std::lock_guard guard(crt_lock); - server->set_parameter(key, value); - if (strcmp(key, CN_ADDRESS) == 0 || strcmp(key, CN_SOCKET) == 0) + bool setting_changed = false; + if (is_normal_parameter) { - server->server_update_address(value); - } - else if (strcmp(key, CN_PORT) == 0) - { - if (int ival = get_positive_int(value)) + // Only some normal parameters can be changed runtime. The key/value-combination has already + // been checked to be valid. + if (strcmp(key, CN_ADDRESS) == 0 || strcmp(key, CN_SOCKET) == 0) { - server->update_port(ival); + server->server_update_address(value); + setting_changed = true; } - } - else if (strcmp(key, CN_EXTRA_PORT) == 0) - { - server->update_extra_port(atoi(value)); - } - else if (strcmp(key, CN_MONITORUSER) == 0) - { - server->set_monitor_user(value); - } - else if (strcmp(key, CN_MONITORPW) == 0) - { - server->set_monitor_password(value); - } - else if (strcmp(key, CN_PERSISTPOOLMAX) == 0) - { - if (is_valid_integer(value)) + else if (strcmp(key, CN_PORT) == 0) { - server->set_persistpoolmax(atoi(value)); + if (int ival = get_positive_int(value)) + { + server->update_port(ival); + setting_changed = true; + } } - } - else if (strcmp(key, CN_PERSISTMAXTIME) == 0) - { - if (is_valid_integer(value)) + else if (strcmp(key, CN_EXTRA_PORT) == 0) { - server->set_persistmaxtime(atoi(value)); + server->update_extra_port(atoi(value)); + setting_changed = true; } - } - if (strcmp(key, CN_RANK) == 0) - { - auto v = config_enum_to_value(value, rank_values); - - if (v != MXS_UNKNOWN_ENUM_VALUE) + else if (strcmp(key, CN_MONITORUSER) == 0) { - server->set_rank(v); + server->set_monitor_user(value); + setting_changed = true; + } + else if (strcmp(key, CN_MONITORPW) == 0) + { + server->set_monitor_password(value); + setting_changed = true; + } + else if (strcmp(key, CN_PERSISTPOOLMAX) == 0) + { + if (is_valid_integer(value)) + { + server->set_persistpoolmax(atoi(value)); + setting_changed = true; + } + } + else if (strcmp(key, CN_PERSISTMAXTIME) == 0) + { + if (is_valid_integer(value)) + { + server->set_persistmaxtime(atoi(value)); + setting_changed = true; + } + } + else if (strcmp(key, CN_RANK) == 0) + { + auto v = config_enum_to_value(value, rank_values); + if (v != MXS_UNKNOWN_ENUM_VALUE) + { + server->set_rank(v); + setting_changed = true; + } + else + { + config_runtime_error("Invalid value for '%s': %s", CN_RANK, value); + } } else { - config_runtime_error("Invalid value for '%s': %s", CN_RANK, value); + // Was a recognized parameter but runtime modification is not supported. + config_runtime_error("Server parameter '%s' cannot be modified during runtime. A similar " + "effect can be produced by destroying the server and recreating it " + "with the new settings.", key); + } + + if (setting_changed) + { + // Successful modification of a normal parameter, write to text storage. + server->set_normal_parameter(key, value); } } else { - /** - * It's likely that this parameter is used as a weighting parameter. - * We need to update the weights of services that use this. - */ + // This is a custom parameter and may be used for weighting. Update the weights of services. + server->set_custom_parameter(key, value); service_update_weights(); + setting_changed = true; } - server->serialize(); - MXS_NOTICE("Updated server '%s': %s=%s", server->name(), key, value); - - return true; + if (setting_changed) + { + server->serialize(); + MXS_NOTICE("Updated server '%s': %s=%s", server->name(), key, value); + } + return setting_changed; } static bool undefined_mandatory_parameter(const MXS_MODULE_PARAM* mod_params, diff --git a/server/core/internal/server.hh b/server/core/internal/server.hh index b3fb183cf..3a193bfda 100644 --- a/server/core/internal/server.hh +++ b/server/core/internal/server.hh @@ -36,12 +36,6 @@ public: m_settings.authenticator = authenticator; } - struct ConfigParameter - { - std::string name; - std::string value; - }; - long persistpoolmax() const { return m_settings.persistpoolmax; @@ -186,10 +180,10 @@ public: static Server* find_by_unique_name(const std::string& name); /** - * Test if name is a normal server setting name. + * Test if name is not a normal server setting name. * * @param name Name to check - * @return True if name is a standard parameter + * @return True if name is not a standard parameter */ bool is_custom_parameter(const std::string& name) const; @@ -241,16 +235,17 @@ public: static json_t* server_json_attributes(const Server* server); /** - * @brief Set server parameter + * Set server custom parameter. * - * @param server Server to update * @param name Parameter to set * @param value Value of parameter */ - void set_parameter(const std::string& name, const std::string& value); + void set_custom_parameter(const std::string& name, const std::string& value); std::string get_custom_parameter(const std::string& name) const override; + void set_normal_parameter(const std::string& name, const std::string& value); + /** * @brief Serialize a server to a file * @@ -330,7 +325,7 @@ private: /** All config settings in text form. This is only read and written from the admin thread * so no need for locking. */ - std::vector all_parameters; + MXS_CONFIG_PARAMETER all_parameters; std::string protocol; /**< Backend protocol module name. Does not change so needs no locking. */ std::string authenticator; /**< Authenticator module name. Does not change so needs no locking. */ @@ -347,9 +342,9 @@ private: * by mutex. */ DiskSpaceLimits disk_space_limits; - /** Additional custom parameters which may affect routing decisions or the monitor module. + /** Additional custom parameters which may affect routing decisions or a monitor. * Can be queried from modules at any time so access must be protected by mutex. */ - std::map custom_parameters; + MXS_CONFIG_PARAMETER custom_parameters; }; /** diff --git a/server/core/server.cc b/server/core/server.cc index 6bf1cfc8a..94246a14e 100644 --- a/server/core/server.cc +++ b/server/core/server.cc @@ -122,34 +122,6 @@ ThisUnit this_unit; const char ERR_TOO_LONG_CONFIG_VALUE[] = "The new value for %s is too long. Maximum length is %i characters."; -// Converts Server::ConfigParam to MXS_CONFIG_PARAM and keeps them in the same order. Required for some -// functions taking MXS_CONFIG_PARAMs as arguments. -class ParamAdaptor -{ -public: - ParamAdaptor(const std::vector& parameters) - { - m_params = new MXS_CONFIG_PARAMETER; - for (const auto& elem : parameters) - { - m_params->set(elem.name, elem.value); - } - } - - ~ParamAdaptor() - { - delete m_params; - } - - operator MXS_CONFIG_PARAMETER*() - { - return m_params; - } - -private: - MXS_CONFIG_PARAMETER* m_params = nullptr; -}; - /** * Write to char array by first zeroing any extra space. This reduces effects of concurrent reading. * Concurrent writing should be prevented by the caller. @@ -273,14 +245,14 @@ Server* Server::server_alloc(const char* name, MXS_CONFIG_PARAMETER* params) server->set_monitor_password(monpw); } + server->m_settings.all_parameters = *params; for (auto p : *params) { - const string name = p.first; - const string value = p.second; - server->m_settings.all_parameters.push_back({name, value}); - if (server->is_custom_parameter(name)) + const string& param_name = p.first; + const string& param_value = p.second; + if (server->is_custom_parameter(param_name)) { - server->set_parameter(name, value); + server->set_custom_parameter(param_name, param_value); } } @@ -522,7 +494,7 @@ void Server::print_to_dcb(DCB* dcb) const for (const auto& elem : server->m_settings.all_parameters) { dcb_printf(dcb, "\t %s\t%s\n", - elem.name.c_str(), elem.value.c_str()); + elem.first.c_str(), elem.second.c_str()); } } dcb_printf(dcb, "\tNumber of connections: %d\n", server->stats.n_connections); @@ -757,37 +729,23 @@ string Server::monitor_password() const return m_settings.monpw; } -void Server::set_parameter(const string& name, const string& value) +void Server::set_custom_parameter(const string& name, const string& value) { - // Set/add the parameter in both containers - bool found = false; - for (auto& elem : m_settings.all_parameters) - { - if (name == elem.name) - { - found = true; - elem.value = value; // Update - break; - } - } - if (!found) - { - m_settings.all_parameters.push_back({name, value}); - } - std::lock_guard guard(m_settings.lock); - m_settings.custom_parameters[name] = value; + // Set/add the parameter in both containers. + m_settings.all_parameters.set(name, value); + Guard guard(m_settings.lock); + m_settings.custom_parameters.set(name, value); } string Server::get_custom_parameter(const string& name) const { - string rval; - std::lock_guard guard(m_settings.lock); - auto iter = m_settings.custom_parameters.find(name); - if (iter != m_settings.custom_parameters.end()) - { - rval = iter->second; - } - return rval; + Guard guard(m_settings.lock); + return m_settings.custom_parameters.get_string(name); +} + +void Server::set_normal_parameter(const std::string& name, const std::string& value) +{ + m_settings.all_parameters.set(name, value); } /** @@ -898,16 +856,20 @@ bool Server::create_server_config(const Server* server, const char* filename) // TODO: Check for return values on all of the dprintf calls const MXS_MODULE* mod = get_module(server->m_settings.protocol.c_str(), MODULE_PROTOCOL); string config = generate_config_string(server->name(), - *ParamAdaptor(server->m_settings.all_parameters), + server->m_settings.all_parameters, config_server_params, mod->parameters); - // Print custom parameters - for (const auto& elem : server->m_settings.custom_parameters) + // Print custom parameters. The generate_config_string()-call doesn't print them. { - config += elem.first + "=" + elem.second + "\n"; + Guard guard(server->m_settings.lock); + for (const auto& elem : server->m_settings.custom_parameters) + { + config += elem.first + "=" + elem.second + "\n"; + } } + if (dprintf(file, "%s", config.c_str()) == -1) { MXS_ERROR("Could not write serialized configuration to file '%s': %d, %s", @@ -1029,18 +991,21 @@ json_t* Server::server_json_attributes(const Server* server) json_t* params = json_object(); const MXS_MODULE* mod = get_module(server->m_settings.protocol.c_str(), MODULE_PROTOCOL); - config_add_module_params_json(ParamAdaptor(server->m_settings.all_parameters), + config_add_module_params_json(&server->m_settings.all_parameters, {CN_TYPE}, config_server_params, mod->parameters, params); // Add weighting parameters that weren't added by config_add_module_params_json - for (const auto& elem : server->m_settings.custom_parameters) { - if (!json_object_get(params, elem.first.c_str())) + Guard guard(server->m_settings.lock); + for (const auto& elem : server->m_settings.custom_parameters) { - json_object_set_new(params, elem.first.c_str(), json_string(elem.second.c_str())); + if (!json_object_get(params, elem.first.c_str())) + { + json_object_set_new(params, elem.first.c_str(), json_string(elem.second.c_str())); + } } } diff --git a/server/core/test/test_server.cc b/server/core/test/test_server.cc index d003c6d41..d8b4831f1 100644 --- a/server/core/test/test_server.cc +++ b/server/core/test/test_server.cc @@ -51,7 +51,6 @@ static MXS_CONFIG_PARAMETER* params = new MXS_CONFIG_PARAMETER; */ static int test1() { - int result; std::string status; /* Server tests */ @@ -62,7 +61,7 @@ static int test1() fprintf(stderr, "\t..done\nTest Parameter for Server."); mxb_assert_message(server->get_custom_parameter("name").empty(), "Parameter should be empty when not set"); - server->set_parameter("name", "value"); + server->set_custom_parameter("name", "value"); std::string buf = server->get_custom_parameter("name"); mxb_assert_message(buf == "value", "Parameter should be returned correctly"); fprintf(stderr, "\t..done\nTesting Unique Name for Server.");