From e2ace578d27671e4d6aeba534827ceb3c00e8d2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Wed, 15 Aug 2018 23:32:53 +0300 Subject: [PATCH] Validate parameters before storing them The runtime modification of servers, services and monitors now validates the parameters before starting the update process. This guarantees that the set of parameters is valid before it is processed. After the validation, the parameters are now also stored in the list of configuration parameters. This will simplify the serialization process by removing the need to explicitly serialize all common object parameters. --- server/core/config_runtime.cc | 108 +++++++++++++------------------- server/core/internal/service.hh | 4 +- server/core/service.cc | 47 +++----------- 3 files changed, 55 insertions(+), 104 deletions(-) diff --git a/server/core/config_runtime.cc b/server/core/config_runtime.cc index 1e9521863..4fbee5e33 100644 --- a/server/core/config_runtime.cc +++ b/server/core/config_runtime.cc @@ -410,39 +410,38 @@ static inline bool is_valid_integer(const char* value) bool runtime_alter_server(SERVER *server, const char *key, const char *value) { + if (!value[0]) + { + config_runtime_error("Empty value for parameter: %s", key); + return false; + } + mxs::SpinLockGuard guard(crt_lock); - bool valid = false; + server_update_parameter(server, key, value); if (strcmp(key, CN_ADDRESS) == 0) { - valid = true; server_update_address(server, value); } else if (strcmp(key, CN_PORT) == 0) { - long ival = get_positive_int(value); - - if (ival) + if (long ival = get_positive_int(value)) { - valid = true; server_update_port(server, ival); } } else if (strcmp(key, CN_MONITORUSER) == 0) { - valid = true; server_update_credentials(server, value, server->monpw); } else if (strcmp(key, CN_MONITORPW) == 0) { - valid = true; server_update_credentials(server, server->monuser, value); } else if (strcmp(key, CN_PERSISTPOOLMAX) == 0) { if (is_valid_integer(value)) { - valid = true; server->persistpoolmax = atoi(value); } } @@ -450,47 +449,26 @@ bool runtime_alter_server(SERVER *server, const char *key, const char *value) { if (is_valid_integer(value)) { - valid = true; server->persistmaxtime = atoi(value); } } else { - if (!value[0] && !server_remove_parameter(server, key)) - { - // Not a valid parameter - } - else if (value[0]) - { - valid = true; - server_update_parameter(server, key, value); - - /** - * It's likely that this parameter is used as a weighting parameter. - * We need to update the weights of services that use this. - */ - service_update_weights(); - } + /** + * It's likely that this parameter is used as a weighting parameter. + * We need to update the weights of services that use this. + */ + service_update_weights(); } - if (valid) - { - if (server_serialize(server)) - { - MXS_NOTICE("Updated server '%s': %s=%s", server->name, key, value); - } - } - else - { - config_runtime_error("Invalid server parameter: %s=%s", key, value); - } + server_serialize(server); + MXS_NOTICE("Updated server '%s': %s=%s", server->name, key, value); - return valid; + return true; } bool runtime_alter_monitor(MXS_MONITOR *monitor, const char *key, const char *value) { - mxs::SpinLockGuard guard(crt_lock);; const MXS_MODULE *mod = get_module(monitor->module_name, MODULE_MONITOR); if (!config_param_is_valid(config_monitor_params, key, value, NULL) && @@ -505,6 +483,7 @@ bool runtime_alter_monitor(MXS_MONITOR *monitor, const char *key, const char *va return false; } + mxs::SpinLockGuard guard(crt_lock); monitor_stop(monitor); monitor_set_parameter(monitor, key, value); @@ -584,21 +563,31 @@ bool runtime_alter_monitor(MXS_MONITOR *monitor, const char *key, const char *va bool runtime_alter_service(Service *service, const char* zKey, const char* zValue) { - std::string key(zKey); - std::string value(zValue); - bool valid = false; + const MXS_MODULE* mod = get_module(service->routerModule, MODULE_ROUTER); + ss_dassert(mod); - const MXS_MODULE* module = get_module(service->routerModule, MODULE_ROUTER); - ss_dassert(module); + if (!config_param_is_valid(config_service_params, zKey, zValue, NULL) && + !config_param_is_valid(mod->parameters, zKey, zValue, NULL)) + { + config_runtime_error("Invalid service parameter: %s", zKey); + return false; + } + else if (!zValue[0]) + { + config_runtime_error("Empty value for parameter: %s", zKey); + return false; + } mxs::SpinLockGuard guard(crt_lock); + std::string key(zKey); + std::string value(zValue); + bool rval = true; if (service->is_basic_parameter(key)) { - valid = service->update_basic_parameter(key, value); + service->update_basic_parameter(key, value); } - else if (config_param_is_valid(module->parameters, key.c_str(), value.c_str(), NULL) || - key == CN_ROUTER_OPTIONS) + else { if (service->router->configureInstance && service->capabilities & RCAP_TYPE_RUNTIME_CONFIG) { @@ -606,11 +595,7 @@ bool runtime_alter_service(Service *service, const char* zKey, const char* zValu std::string old_value = config_get_string(service->svc_config_param, key.c_str()); service_replace_parameter(service, key.c_str(), value.c_str()); - if (service->router->configureInstance(service->router_instance, service->svc_config_param)) - { - valid = true; - } - else + if (!service->router->configureInstance(service->router_instance, service->svc_config_param)) { // Reconfiguration failed, restore the old value of the parameter if (old_value.empty()) @@ -621,30 +606,27 @@ bool runtime_alter_service(Service *service, const char* zKey, const char* zValu { service_replace_parameter(service, key.c_str(), old_value.c_str()); } + + rval = false; config_runtime_error("Reconfiguration of service '%s' failed. See log " "file for more details.", service->name); } } else { + rval = false; config_runtime_error("Router '%s' does not support reconfiguration.", service->routerModule); } } - else - { - config_runtime_error("Invalid service parameter: %s=%s", key.c_str(), zValue); - MXS_ERROR("Unknown parameter for service '%s': %s=%s", - service->name, key.c_str(), value.c_str()); - } - if (valid) + if (rval) { service_serialize(service); MXS_NOTICE("Updated service '%s': %s=%s", service->name, key.c_str(), value.c_str()); } - return valid; + return rval; } bool runtime_alter_maxscale(const char* name, const char* value) @@ -1343,9 +1325,9 @@ bool runtime_is_count_or_null(json_t* json, const char* path) config_runtime_error("Parameter '%s' is not an integer but %s", path, json_type_to_string(value)); rval = false; } - else if (json_integer_value(value) <= 0) + else if (json_integer_value(value) < 0) { - config_runtime_error("Parameter '%s' is not a positive integer", path); + config_runtime_error("Parameter '%s' is a negative integer", path); rval = false; } } @@ -1819,7 +1801,7 @@ static bool validate_object_json(json_t* json, std::vector paths, } else { - for (const auto& a: paths) + for (const auto& a : paths) { if (!(value = mxs_json_pointer(json, a.c_str()))) { @@ -1831,7 +1813,7 @@ static bool validate_object_json(json_t* json, std::vector paths, } } - for (const auto& a: relationships) + for (const auto& a : relationships) { StringSet relations; if (extract_relations(json, relations, a.first, a.second)) diff --git a/server/core/internal/service.hh b/server/core/internal/service.hh index 4b3307fc4..a9db0f51a 100644 --- a/server/core/internal/service.hh +++ b/server/core/internal/service.hh @@ -59,10 +59,8 @@ public: * * @param name Name of the parameter to update * @param value The new value of the parameter - * - * @return True if the parameter and its value were valid */ - bool update_basic_parameter(const std::string& name, const std::string& value); + void update_basic_parameter(const std::string& name, const std::string& value); /** * Set the list of filters for this service diff --git a/server/core/service.cc b/server/core/service.cc index 3a7822c2b..c53329f77 100644 --- a/server/core/service.cc +++ b/server/core/service.cc @@ -2352,98 +2352,69 @@ bool Service::is_basic_parameter(const std::string& name) return names.find(name) != names.end(); } -bool Service::update_basic_parameter(const std::string& key, const std::string& value) +void Service::update_basic_parameter(const std::string& key, const std::string& value) { - bool valid = false; - if (key == CN_USER) { m_user = value; snprintf(user, sizeof(user), "%s", value.c_str()); - valid = true; } else if (key == CN_PASSWORD) { m_password = value; snprintf(password, sizeof(password), "%s", value.c_str()); - valid = true; } else if (key == CN_ENABLE_ROOT_USER) { enable_root = config_truth_value(value.c_str()); - valid = true; } else if (key == CN_MAX_RETRY_INTERVAL) { - int i = std::stoi(value); - - if (i > 0) - { - max_retry_interval = i; - valid = true; - } + max_retry_interval = std::stoi(value); + ss_dassert(max_retry_interval > 0); } else if (key == CN_MAX_CONNECTIONS) { - int i = std::stoi(value); - - if (i > 0) - { - max_connections = i; - valid = true; - } + max_connections = std::stoi(value); + ss_dassert(max_connections > 0); } else if (key == CN_CONNECTION_TIMEOUT) { - int i = std::stoi(value); - - if (i > 0) + if ((conn_idle_timeout = std::stoi(value))) { - valid = true; - - if ((conn_idle_timeout = i)) - { - dcb_enable_session_timeouts(); - } + dcb_enable_session_timeouts(); } + + ss_dassert(conn_idle_timeout >= 0); } else if (key == CN_AUTH_ALL_SERVERS) { users_from_all = config_truth_value(value.c_str()); - valid = true; } else if (key == CN_STRIP_DB_ESC) { strip_db_esc = config_truth_value(value.c_str()); - valid = true; } else if (key == CN_LOCALHOST_MATCH_WILDCARD_HOST) { localhost_match_wildcard_host = config_truth_value(value.c_str()); - valid = true; } else if (key == CN_VERSION_STRING) { m_version_string = value; snprintf(version_string, sizeof(version_string), "%s", value.c_str()); - valid = true; } else if (key == CN_WEIGHTBY) { m_weightby = value; snprintf(weightby, sizeof(weightby), "%s", value.c_str()); - valid = true; } else if (key == CN_LOG_AUTH_WARNINGS) { log_auth_warnings = config_truth_value(value.c_str()); - valid = true; } else if (key == CN_RETRY_ON_FAILURE) { retry_start = config_truth_value(value.c_str()); - valid = true; } - - return valid; }