From 11bee30f614ffdc818065b17fe9a5a96677fd2bd Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Sat, 12 Nov 2016 13:19:14 +0200 Subject: [PATCH 01/10] Fix backend SSL The backend SSL connections weren't authenticated due to an inverted check. This caused all SSL connections to fail. --- server/modules/protocol/MySQL/MySQLBackend/mysql_backend.c | 5 +++++ server/modules/protocol/MySQL/mysql_common.c | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/server/modules/protocol/MySQL/MySQLBackend/mysql_backend.c b/server/modules/protocol/MySQL/MySQLBackend/mysql_backend.c index 89de78cc5..df89c4b01 100644 --- a/server/modules/protocol/MySQL/MySQLBackend/mysql_backend.c +++ b/server/modules/protocol/MySQL/MySQLBackend/mysql_backend.c @@ -617,6 +617,11 @@ gw_read_backend_event(DCB *dcb) gw_reply_on_error(dcb, proto->protocol_auth_state); } } + else if (proto->protocol_auth_state == MXS_AUTH_STATE_CONNECTED && + dcb->ssl_state == SSL_ESTABLISHED) + { + proto->protocol_auth_state = gw_send_backend_auth(dcb); + } } return rc; diff --git a/server/modules/protocol/MySQL/mysql_common.c b/server/modules/protocol/MySQL/mysql_common.c index 5104720be..9873b30ae 100644 --- a/server/modules/protocol/MySQL/mysql_common.c +++ b/server/modules/protocol/MySQL/mysql_common.c @@ -1339,7 +1339,7 @@ mxs_auth_state_t gw_send_backend_auth(DCB *dcb) (dcb->session->state != SESSION_STATE_READY && dcb->session->state != SESSION_STATE_ROUTER_READY) || (dcb->server->server_ssl && - dcb->ssl_state != SSL_HANDSHAKE_FAILED)) + dcb->ssl_state == SSL_HANDSHAKE_FAILED)) { return MXS_AUTH_STATE_FAILED; } From b893ca7ba8f7a7f25ab0b473a904f6e4f76f21ca Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Sat, 12 Nov 2016 13:22:18 +0200 Subject: [PATCH 02/10] Move configuration context processing into subfunctions The functions allow simple operations on configuration context objects. This makes it easier to understand what the code does and allows reuse of the configuration processing code. --- include/maxscale/config.h | 81 ++++++++++++++++- include/maxscale/server.h | 2 - server/core/config.c | 177 +++++++++++++++++++++++--------------- server/core/server.c | 11 --- 4 files changed, 186 insertions(+), 85 deletions(-) diff --git a/include/maxscale/config.h b/include/maxscale/config.h index 8e616aae4..6f02ec613 100644 --- a/include/maxscale/config.h +++ b/include/maxscale/config.h @@ -32,6 +32,7 @@ #include #include #include +#include MXS_BEGIN_DECLS @@ -129,13 +130,91 @@ typedef struct } GATEWAY_CONF; +/** + * @brief Creates an empty configuration context + * + * @param section Context name + * @return New context or NULL on memory allocation failure + */ +CONFIG_CONTEXT* config_context_create(const char *section); + +/** + * @brief Free a configuration context + * + * @param context The context to free + */ +void config_context_free(CONFIG_CONTEXT *context); + +/** + * @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 + */ +CONFIG_PARAMETER* config_get_param(CONFIG_PARAMETER* params, const char* name); + +/** + * @brief Add a parameter to a configuration context + * + * @param obj Context where the parameter should be added + * @param key Key to add + * @param value Value for the key + * @return True on success, false on memory allocation error + */ +bool config_add_param(CONFIG_CONTEXT* obj, const char* key, const char* value); + +/** + * @brief Append to an existing parameter + * + * @param obj Configuration context + * @param key Parameter name + * @param value Value to append to the parameter + * @return True on success, false on memory allocation error + */ +bool config_append_param(CONFIG_CONTEXT* obj, const char* key, const char* value); + +/** + * @brief Check if all SSL parameters are defined + * + * Helper function to check whether all of the required SSL parameters are defined + * in the configuration context. The checked parameters are 'ssl', 'ssl_key', + * 'ssl_cert' and 'ssl_ca_cert'. The 'ssl' parameter must also have a value of + * 'required'. + * + * @param obj Configuration context + * @return True if all required parameters are present + */ +bool config_have_required_ssl_params(CONFIG_CONTEXT *obj); + +/** + * @brief Helper function for checking SSL parameters + * + * @param key Parameter name + * @return True if the parameter is an SSL parameter + */ +bool config_is_ssl_parameter(const char *key); + +/** + * @brief Construct an SSL structure + * + * The SSL structure is used by both listeners and servers. + * + * TODO: Rename to something like @c config_construct_ssl + * + * @param obj Configuration context + * @param require_cert Whether certificates are required + * @param error_count Pointer to an int which is incremented for each error + * @return New SSL_LISTENER structure or NULL on error + */ +SSL_LISTENER *make_ssl_structure(CONFIG_CONTEXT *obj, bool require_cert, int *error_count); + char* config_clean_string_list(const char* str); CONFIG_PARAMETER* config_clone_param(const CONFIG_PARAMETER* param); void config_enable_feedback_task(void); void config_disable_feedback_task(void); unsigned long config_get_gateway_id(void); GATEWAY_CONF* config_get_global_options(); -CONFIG_PARAMETER* config_get_param(CONFIG_PARAMETER* params, const char* name); config_param_type_t config_get_paramtype(const CONFIG_PARAMETER* param); bool config_get_valint(int* val, const CONFIG_PARAMETER* param, diff --git a/include/maxscale/server.h b/include/maxscale/server.h index eb28e1271..d47493237 100644 --- a/include/maxscale/server.h +++ b/include/maxscale/server.h @@ -280,7 +280,5 @@ extern void server_update_port(SERVER *, unsigned short); extern RESULTSET *serverGetList(); extern unsigned int server_map_status(char *str); extern bool server_set_version_string(SERVER* server, const char* string); -extern bool server_is_ssl_parameter(const char *key); -extern void server_update_ssl(SERVER *server, const char *key, const char *value); MXS_END_DECLS diff --git a/server/core/config.c b/server/core/config.c index 43a738397..6e8abb7a1 100644 --- a/server/core/config.c +++ b/server/core/config.c @@ -80,7 +80,6 @@ static void duplicate_context_finish(DUPLICATE_CONTEXT* context); extern int setipaddress(struct in_addr *, char *); static bool process_config_context(CONFIG_CONTEXT *); static bool process_config_update(CONFIG_CONTEXT *); -static void free_config_context(CONFIG_CONTEXT *); static char *config_get_value(CONFIG_PARAMETER *, const char *); static char *config_get_password(CONFIG_PARAMETER *); static const char *config_get_value_string(CONFIG_PARAMETER *, const char *); @@ -90,13 +89,11 @@ static void global_defaults(); static void feedback_defaults(); static bool check_config_objects(CONFIG_CONTEXT *context); static int maxscale_getline(char** dest, int* size, FILE* file); -static SSL_LISTENER *make_ssl_structure(CONFIG_CONTEXT *obj, bool require_cert, int *error_count); int config_truth_value(char *str); int config_get_ifaddr(unsigned char *output); static int config_get_release_string(char* release); FEEDBACK_CONF *config_get_feedback_data(); -void config_add_param(CONFIG_CONTEXT*, char*, char*); bool config_has_duplicate_sections(const char* config, DUPLICATE_CONTEXT* context); int create_new_service(CONFIG_CONTEXT *obj); int create_new_server(CONFIG_CONTEXT *obj); @@ -326,6 +323,20 @@ char* config_clean_string_list(const char* str) return dest; } +CONFIG_CONTEXT* config_context_create(const char *section) +{ + CONFIG_CONTEXT* ctx = (CONFIG_CONTEXT *)MXS_MALLOC(sizeof(CONFIG_CONTEXT)); + if (ctx) + { + ctx->object = MXS_STRDUP_A(section); + ctx->parameters = NULL; + ctx->next = NULL; + ctx->element = NULL; + } + + return ctx; +} + /** * Config item handler for the ini file reader * @@ -340,7 +351,6 @@ ini_handler(void *userdata, const char *section, const char *name, const char *v { CONFIG_CONTEXT *cntxt = (CONFIG_CONTEXT *)userdata; CONFIG_CONTEXT *ptr = cntxt; - CONFIG_PARAMETER *param, *p1; if (strcmp(section, "gateway") == 0 || strcasecmp(section, "MaxScale") == 0) { @@ -368,54 +378,24 @@ ini_handler(void *userdata, const char *section, const char *name, const char *v if (!ptr) { - if ((ptr = (CONFIG_CONTEXT *)MXS_MALLOC(sizeof(CONFIG_CONTEXT))) == NULL) + if ((ptr = config_context_create(section)) == NULL) { return 0; } - ptr->object = MXS_STRDUP_A(section); - ptr->parameters = NULL; ptr->next = cntxt->next; - ptr->element = NULL; cntxt->next = ptr; } - /* Check to see if the parameter already exists for the section */ - p1 = ptr->parameters; - while (p1) - { - if (!strcmp(p1->name, name)) - { - char *tmp; - int paramlen = strlen(p1->value) + strlen(value) + 2; - if ((tmp = MXS_REALLOC(p1->value, sizeof(char) * (paramlen))) == NULL) - { - return 0; - } - strcat(tmp, ","); - strcat(tmp, value); - if ((p1->value = config_clean_string_list(tmp)) == NULL) - { - p1->value = tmp; - MXS_ERROR("[%s] Cleaning configuration parameter failed.", __FUNCTION__); - return 0; - } - MXS_FREE(tmp); - return 1; - } - p1 = p1->next; - } - - if ((param = (CONFIG_PARAMETER *)MXS_MALLOC(sizeof(CONFIG_PARAMETER))) == NULL) + if (config_get_param(ptr->parameters, name) && + !config_append_param(ptr, name, value)) + { + return 0; + } + else if (!config_add_param(ptr, name, value)) { return 0; } - - param->name = MXS_STRDUP_A(name); - param->value = MXS_STRDUP_A(value); - param->next = ptr->parameters; - param->qfd_param_type = UNDEFINED_TYPE; - ptr->parameters = param; return 1; } @@ -637,7 +617,7 @@ config_load_and_process(const char* filename, bool (*process_config)(CONFIG_CONT } } - free_config_context(ccontext.next); + config_context_free(ccontext.next); duplicate_context_finish(&dcontext); } @@ -876,10 +856,7 @@ config_get_value_string(CONFIG_PARAMETER *params, const char *name) return ""; } - -CONFIG_PARAMETER* config_get_param( - CONFIG_PARAMETER* params, - const char* name) +CONFIG_PARAMETER* config_get_param(CONFIG_PARAMETER* params, const char* name) { while (params) { @@ -1042,22 +1019,15 @@ void free_config_parameter(CONFIG_PARAMETER* p1) } } -/** - * Free a config tree - * - * @param context The configuration data - */ -static void -free_config_context(CONFIG_CONTEXT *context) +void config_context_free(CONFIG_CONTEXT *context) { CONFIG_CONTEXT *obj; - CONFIG_PARAMETER *p1, *p2; while (context) { - MXS_FREE(context->object); - free_config_parameter(context->parameters); obj = context->next; + free_config_parameter(context->parameters); + MXS_FREE(context->object); MXS_FREE(context); context = obj; } @@ -1362,8 +1332,7 @@ free_ssl_structure(SSL_LISTENER *ssl) * @param *error_count An error count which may be incremented * @return SSL_LISTENER structure or NULL */ -static SSL_LISTENER * -make_ssl_structure (CONFIG_CONTEXT *obj, bool require_cert, int *error_count) +SSL_LISTENER* make_ssl_structure (CONFIG_CONTEXT *obj, bool require_cert, int *error_count) { char *ssl, *ssl_version, *ssl_cert, *ssl_key, *ssl_ca_cert, *ssl_cert_verify_depth; int local_errors = 0; @@ -2288,26 +2257,57 @@ unsigned long config_get_gateway_id() return gateway.id; } -void config_add_param(CONFIG_CONTEXT* obj, char* key, char* value) +bool config_add_param(CONFIG_CONTEXT* obj, const char* key, const char* value) { - key = MXS_STRDUP(key); - value = MXS_STRDUP(value); + ss_dassert(config_get_param(obj->parameters, key) == NULL); + bool rval = false; + char *my_key = MXS_STRDUP(key); + char *my_value = MXS_STRDUP(value); + CONFIG_PARAMETER* param = (CONFIG_PARAMETER *)MXS_MALLOC(sizeof(*param)); - CONFIG_PARAMETER* param = (CONFIG_PARAMETER *)MXS_MALLOC(sizeof(CONFIG_PARAMETER)); - - if (!key || !value || !param) + if (my_key && my_value && param) { - MXS_FREE(key); - MXS_FREE(value); + param->name = my_key; + param->value = my_value; + param->qfd_param_type = UNDEFINED_TYPE; + param->next = obj->parameters; + obj->parameters = param; + rval = true; + } + else + { + MXS_FREE(my_key); + MXS_FREE(my_value); MXS_FREE(param); - return; } - param->name = key; - param->value = value; - param->next = obj->parameters; - obj->parameters = param; + return rval; } + +bool config_append_param(CONFIG_CONTEXT* obj, const char* key, const char* value) +{ + CONFIG_PARAMETER *param = config_get_param(obj->parameters, key); + ss_dassert(param); + int paramlen = strlen(param->value) + strlen(value) + 2; + char tmp[paramlen]; + bool rval = false; + + strcpy(tmp, param->value); + strcat(tmp, ","); + strcat(tmp, value); + + char *new_value = config_clean_string_list(tmp); + + if (new_value) + { + MXS_FREE(param->value); + param->value = new_value; + rval = true; + } + + return rval; +} + /** * Return the pointer to the global options for MaxScale. * @return Pointer to the GATEWAY_CONF structure. This is a static structure and @@ -3158,3 +3158,38 @@ int create_new_filter(CONFIG_CONTEXT *obj) return error_count; } + +bool config_have_required_ssl_params(CONFIG_CONTEXT *obj) +{ + CONFIG_PARAMETER *param = obj->parameters; + + return config_get_param(param, "ssl") && + config_get_param(param, "ssl_key") && + config_get_param(param, "ssl_cert") && + config_get_param(param, "ssl_ca_cert") && + strcmp(config_get_value_string(param, "ssl"), "required") == 0; +} + +bool config_is_ssl_parameter(const char *key) +{ + const char *ssl_params[] = + { + "ssl_cert", + "ssl_ca_cert", + "ssl", + "ssl_key", + "ssl_version", + "ssl_cert_verify_depth", + NULL + }; + + for (int i = 0; ssl_params[i]; i++) + { + if (strcmp(key, ssl_params[i]) == 0) + { + return true; + } + } + + return false; +} diff --git a/server/core/server.c b/server/core/server.c index 1faee1076..6c096709f 100644 --- a/server/core/server.c +++ b/server/core/server.c @@ -1354,14 +1354,3 @@ bool server_destroy(SERVER *server) return rval; } - -bool server_is_ssl_parameter(const char *key) -{ - // TODO: Implement this - return false; -} - -void server_update_ssl(SERVER *server, const char *key, const char *value) -{ - // TODO: Implement this -} From 8b692b0754fae603a2533ec0f1b6a597aa5334e8 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Sat, 12 Nov 2016 13:23:50 +0200 Subject: [PATCH 03/10] Add SSL support for created servers Servers created at runtime can now be configured to use SSL. The configuration is only possible if the server is not in use. The `alter server` command in maxadmin now takes a list of `key=value` strings. This allows the user to define multiple alter operations with one command. --- include/maxscale/server.h | 12 +++ server/core/server.c | 18 ++-- server/modules/routing/debugcli/debugcmd.c | 115 +++++++++++++++++++-- 3 files changed, 122 insertions(+), 23 deletions(-) diff --git a/include/maxscale/server.h b/include/maxscale/server.h index d47493237..51fe48bea 100644 --- a/include/maxscale/server.h +++ b/include/maxscale/server.h @@ -245,6 +245,18 @@ extern bool server_create(const char *name, const char *address, const char *por const char *protocol, const char *authenticator, const char *options); +/** + * @brief Serialize a server to a file + * + * This converts @c server into an INI format file. This allows created servers + * to be persisted to disk. This will replace any existing files with the same + * name. + * + * @param server Server to serialize + * @return False if the serialization of the server fails, true if it was successful + */ +bool server_serialize(const SERVER *server); + /** * @brief Destroy a server * diff --git a/server/core/server.c b/server/core/server.c index 6c096709f..a51aa2521 100644 --- a/server/core/server.c +++ b/server/core/server.c @@ -987,6 +987,8 @@ server_update_address(SERVER *server, char *address) spinlock_acquire(&server_spin); if (server && address) { + MXS_NOTICE("Updated the address of server '%s' from '%s' to '%s'.", + server->unique_name, server->name, address); strcpy(server->name, address); } spinlock_release(&server_spin); @@ -1005,6 +1007,8 @@ server_update_port(SERVER *server, unsigned short port) spinlock_acquire(&server_spin); if (server && port > 0) { + MXS_NOTICE("Updated the port of server '%s' from %d to %d.", + server->unique_name, server->port, port); server->port = port; } spinlock_release(&server_spin); @@ -1082,7 +1086,7 @@ bool server_set_version_string(SERVER* server, const char* string) * @param filename Filename where configuration is written * @return True on success, false on error */ -static bool create_server_config(SERVER *server, const char *filename) +static bool create_server_config(const SERVER *server, const char *filename) { int file = open(filename, O_EXCL | O_CREAT | O_WRONLY, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH); @@ -1182,17 +1186,7 @@ static bool create_server_config(SERVER *server, const char *filename) return true; } -/** - * @brief Serialize a server to a file - * - * This converts @c server into an INI format file. This allows created servers - * to be persisted to disk. This will replace any existing files with the same - * name. - * - * @param server Server to serialize - * @return False if the serialization of the server fails, true if it was successful - */ -static bool server_serialize(SERVER *server) +bool server_serialize(const SERVER *server) { bool rval = false; char filename[PATH_MAX]; diff --git a/server/modules/routing/debugcli/debugcmd.c b/server/modules/routing/debugcli/debugcmd.c index eff2e3bef..47dde9896 100644 --- a/server/modules/routing/debugcli/debugcmd.c +++ b/server/modules/routing/debugcli/debugcmd.c @@ -1032,9 +1032,9 @@ struct subcommand destroyoptions[] = } }; -static void alterServer(DCB *dcb, SERVER *server, char *key, char *value) +static bool handle_alter_server(SERVER *server, char *key, char *value) { - bool unknown = false; + bool valid = true; if (strcmp(key, "address") == 0) { @@ -1052,18 +1052,108 @@ static void alterServer(DCB *dcb, SERVER *server, char *key, char *value) { server_update_credentials(server, server->monuser, value); } - else if (server_is_ssl_parameter(key)) + else { - server_update_ssl(server, key, value); + valid = false; + } + + return valid; +} + +void handle_server_ssl(DCB *dcb, SERVER *server, CONFIG_CONTEXT *obj) +{ + if (config_have_required_ssl_params(obj)) + { + int err = 0; + SSL_LISTENER *ssl = make_ssl_structure(obj, true, &err); + + if (err == 0 && ssl && listener_init_SSL(ssl) == 0) + { + /** Sync to prevent reads on partially initialized server_ssl */ + atomic_synchronize(); + + server->server_ssl = ssl; + if (server_serialize(server)) + { + dcb_printf(dcb, "SSL enabled for server '%s'\n", server->unique_name); + } + else + { + dcb_printf(dcb, "SSL enabled for server '%s' but persisting " + "it to disk failed, see log for more details.\n", + server->unique_name); + } + } + else + { + dcb_printf(dcb, "Enabling SSL for server '%s' failed, see log " + "for more details.\n", server->unique_name); + } } else { - unknown = true; + dcb_printf(dcb, "Error: SSL configuration requires the following parameters:\n" + "ssl=required ssl_key=PATH ssl_cert=PATH ssl_ca_cert=PATH\n"); + } +} + +/** + * @brief Process multiple alter operations at once + * + * This is a somewhat ugly way to handle multiple key-value changes in one operation + * with one function. This could be handled with a variadic function but the + * required complexity would probably negate any benefits. + */ +static void alterServer(DCB *dcb, SERVER *server, char *v1, char *v2, char *v3, + char *v4, char *v5, char *v6, char *v7, char *v8, char *v9, + char *v10, char *v11) +{ + char *values[11] = {v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11}; + const int items = sizeof(values) / sizeof(values[0]); + CONFIG_CONTEXT *obj = NULL; + + for (int i = 0; i < items; i++) + { + if (values[i]) + { + char *key = values[i]; + char *value = strchr(key, '='); + + if (value) + { + *value++ = '\0'; + + if (config_is_ssl_parameter(key)) + { + /** + * All the required SSL parameters must be defined at once to + * enable SSL for created servers. This removes the problem + * of partial configuration and allows a somewhat atomic + * operation. + */ + if ((obj == NULL && (obj = config_context_create(server->unique_name)) == NULL) || + (!config_add_param(obj, key, value))) + { + dcb_printf(dcb, "Internal error, see log for more details\n"); + } + } + else if (!handle_alter_server(server, key, value)) + { + dcb_printf(dcb, "Error: Unknown key-value parameter: %s=%s\n", key, value); + } + } + else + { + dcb_printf(dcb, "Error: not a key-value parameter: %s\n", values[i]); + } + } } - if (unknown) + if (obj) { - dcb_printf(dcb, "Unknown parameter '%s'", key); + /** We have SSL parameters, try to process them */ + handle_server_ssl(dcb, server, obj); + config_context_free(obj); } } @@ -1147,12 +1237,15 @@ static void alterMonitor(DCB *dcb, MONITOR *monitor, char *key, char *value) struct subcommand alteroptions[] = { { - "server", 3, 3, alterServer, + "server", 2, 12, alterServer, "Alter server parameters", - "Usage: alter server NAME KEY VALUE\n" + "Usage: alter server NAME KEY=VALUE ...\n" "This will alter an existing parameter of a server. The accepted values\n" - "for KEY are: 'address', 'port', 'monuser', 'monpw'", - {ARG_TYPE_SERVER, ARG_TYPE_STRING, ARG_TYPE_STRING} + "for KEY are: 'address', 'port', 'monuser', 'monpw'\n" + "A maximum of 11 parameters can be changed at one time\n", + { ARG_TYPE_SERVER, ARG_TYPE_STRING, ARG_TYPE_STRING, ARG_TYPE_STRING, + ARG_TYPE_STRING, ARG_TYPE_STRING, ARG_TYPE_STRING, ARG_TYPE_STRING, + ARG_TYPE_STRING, ARG_TYPE_STRING, ARG_TYPE_STRING, ARG_TYPE_STRING} }, { "monitor", 3, 3, alterMonitor, From a651eff633d3923e8b2e49c9f076c4ab1dcd051c Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Sat, 12 Nov 2016 13:56:31 +0200 Subject: [PATCH 04/10] Serialize repurposed servers When a destroyed server is taken into use, it needs to be serialized. This will allow the server to be created on restart. --- server/core/server.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/server/core/server.c b/server/core/server.c index a51aa2521..729f8916a 100644 --- a/server/core/server.c +++ b/server/core/server.c @@ -1287,15 +1287,19 @@ bool server_create(const char *name, const char *address, const char *port, server->is_active = true; rval = true; } - else if ((server = server_alloc(name, address, atoi(port), protocol, authenticator, - authenticator_options))) + else { - if (server_serialize(server)) - { - /** server_alloc will add the server to the global list of - * servers so we don't need to manually add it. */ - rval = true; - } + /** + * server_alloc will add the server to the global list of + * servers so we don't need to manually add it. + */ + server = server_alloc(name, address, atoi(port), protocol, + authenticator, authenticator_options); + } + + if (server && server_serialize(server)) + { + rval = true; } } From a17aa28eedae9393ce3ba9a1b4fbe3f2fe026c51 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Sat, 12 Nov 2016 21:55:42 +0200 Subject: [PATCH 05/10] Persist changes to the list of monitored servers When a server is added to a monitor, an supplementary configuration file is generated to persist this information. This will allow dynamic modifications to server lists which will survive restarts and unexpected downtime. The monitor will only add new servers to its list of monitored servers. This prevents duplicate entries in the list and makes it safe to persist all used servers to the supplementary configuration file instead of only the ones that are not listed in the main configuration. --- include/maxscale/monitor.h | 16 ++ server/core/config.c | 66 ++++++++- server/core/monitor.c | 162 +++++++++++++++++---- server/modules/routing/debugcli/debugcmd.c | 2 + 4 files changed, 214 insertions(+), 32 deletions(-) diff --git a/include/maxscale/monitor.h b/include/maxscale/monitor.h index 55e795922..1db55f8c3 100644 --- a/include/maxscale/monitor.h +++ b/include/maxscale/monitor.h @@ -233,6 +233,22 @@ connect_result_t mon_connect_to_db(MONITOR* mon, MONITOR_SERVERS *database); void mon_log_connect_error(MONITOR_SERVERS* database, connect_result_t rval); void mon_log_state_change(MONITOR_SERVERS *ptr); +/** + * @brief Serialize a monitor to a file + * + * This partially converts @c monitor into an INI format file. Only the servers + * of the monitor are serialized. This allows the monitor to keep monitoring + * the servers that were added at runtime even after a restart. + * + * NOTE: This does not persist the complete monitor configuration and requires + * that an existing monitor configuration is in the main configuration file. + * Changes to monitor parameters are not persisted. + * + * @param monitor Monitor to serialize + * @return False if the serialization of the monitor fails, true if it was successful + */ +bool monitor_serialize_servers(const MONITOR *monitor); + /** * Check if a monitor uses @c servers * @param server Server that is queried diff --git a/server/core/config.c b/server/core/config.c index 6e8abb7a1..26c1c4784 100644 --- a/server/core/config.c +++ b/server/core/config.c @@ -561,6 +561,47 @@ static bool is_directory(const char *dir) return rval; } +/** + * @brief Check if a directory contains .cnf files + * + * @param path Path to a directory + * @return True if the directory contained one or more .cnf files + */ +static bool contains_cnf_files(const char *path) +{ + bool rval = false; + glob_t matches; + const char suffix[] = "/*.cnf"; + char pattern[strlen(path) + sizeof(suffix)]; + + strcpy(pattern, path); + strcat(pattern, suffix); + int rc = glob(pattern, GLOB_NOSORT, NULL, &matches); + + switch (rc) + { + case 0: + rval = true; + break; + + case GLOB_NOSPACE: + MXS_OOM(); + break; + + case GLOB_ABORTED: + MXS_ERROR("Failed to read directory '%s'", path); + break; + + default: + ss_dassert(rc == GLOB_NOMATCH); + break; + } + + globfree(&matches); + + return rval; +} + /** * @brief Load the specified configuration file for MaxScale * @@ -605,7 +646,23 @@ config_load_and_process(const char* filename, bool (*process_config)(CONFIG_CONT if (is_directory(persist_cnf)) { - rval = config_load_dir(persist_cnf, &dcontext, &ccontext); + DUPLICATE_CONTEXT p_dcontext; + /** + * We need to initialize a second duplicate context for the + * generated configuration files as the monitors and services will + * have duplicate sections. The duplicate sections are used to + * store changes to the list of servers the services and monitors + * use, and thus should not be treated as errors. + */ + if (duplicate_context_init(&p_dcontext)) + { + rval = config_load_dir(persist_cnf, &p_dcontext, &ccontext); + duplicate_context_finish(&p_dcontext); + } + else + { + rval = false; + } } if (rval) @@ -613,6 +670,13 @@ config_load_and_process(const char* filename, bool (*process_config)(CONFIG_CONT if (!check_config_objects(ccontext.next) || !process_config(ccontext.next)) { rval = false; + if (contains_cnf_files(persist_cnf)) + { + MXS_WARNING("One or more generated configurations were found at '%s'. " + "If the error relates to any of the files located there, " + "remove the offending configurations from this directory.", + persist_cnf); + } } } } diff --git a/server/core/monitor.c b/server/core/monitor.c index 94f930a8a..39c2c14f1 100644 --- a/server/core/monitor.c +++ b/server/core/monitor.c @@ -40,6 +40,7 @@ #include #include #include +#include /* * Create declarations of the enum for monitor events and also the array of @@ -244,46 +245,62 @@ monitorStopAll() void monitorAddServer(MONITOR *mon, SERVER *server) { - MONITOR_SERVERS *db = (MONITOR_SERVERS *)MXS_MALLOC(sizeof(MONITOR_SERVERS)); - MXS_ABORT_IF_NULL(db); - - db->server = server; - db->con = NULL; - db->next = NULL; - db->mon_err_count = 0; - db->log_version_err = true; - /** Server status is uninitialized */ - db->mon_prev_status = -1; - /* pending status is updated by get_replication_tree */ - db->pending_status = 0; - - monitor_state_t old_state = mon->state; - - if (old_state == MONITOR_STATE_RUNNING) - { - monitorStop(mon); - } - + bool new_server = true; spinlock_acquire(&mon->lock); - if (mon->databases == NULL) + for (MONITOR_SERVERS *db = mon->databases; db; db = db->next) { - mon->databases = db; - } - else - { - MONITOR_SERVERS *ptr = mon->databases; - while (ptr->next != NULL) + if (db->server == server) { - ptr = ptr->next; + new_server = false; } - ptr->next = db; } + spinlock_release(&mon->lock); - if (old_state == MONITOR_STATE_RUNNING) + if (new_server) { - monitorStart(mon, mon->parameters); + MONITOR_SERVERS *db = (MONITOR_SERVERS *)MXS_MALLOC(sizeof(MONITOR_SERVERS)); + MXS_ABORT_IF_NULL(db); + + db->server = server; + db->con = NULL; + db->next = NULL; + db->mon_err_count = 0; + db->log_version_err = true; + /** Server status is uninitialized */ + db->mon_prev_status = -1; + /* pending status is updated by get_replication_tree */ + db->pending_status = 0; + + monitor_state_t old_state = mon->state; + + if (old_state == MONITOR_STATE_RUNNING) + { + monitorStop(mon); + } + + spinlock_acquire(&mon->lock); + + if (mon->databases == NULL) + { + mon->databases = db; + } + else + { + MONITOR_SERVERS *ptr = mon->databases; + while (ptr->next != NULL) + { + ptr = ptr->next; + } + ptr->next = db; + } + spinlock_release(&mon->lock); + + if (old_state == MONITOR_STATE_RUNNING) + { + monitorStart(mon, mon->parameters); + } } } @@ -1152,3 +1169,86 @@ bool monitor_server_in_use(const SERVER *server) return rval; } + +/** + * Creates a monitor configuration at the location pointed by @c filename + * + * @param monitor Monitor to serialize into a configuration + * @param filename Filename where configuration is written + * @return True on success, false on error + */ +static bool create_monitor_config(const MONITOR *monitor, const char *filename) +{ + int file = open(filename, O_EXCL | O_CREAT | O_WRONLY, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH); + + if (file == -1) + { + char errbuf[MXS_STRERROR_BUFLEN]; + MXS_ERROR("Failed to open file '%s' when serializing monitor '%s': %d, %s", + filename, monitor->name, errno, strerror_r(errno, errbuf, sizeof(errbuf))); + return false; + } + + /** + * Only additional parameters are added to the configuration. This prevents + * duplication or addition of parameters that don't support it. + * + * TODO: Check for return values on all of the dprintf calls + */ + dprintf(file, "[%s]\n", monitor->name); + + if (monitor->databases) + { + dprintf(file, "servers="); + for (MONITOR_SERVERS *db = monitor->databases; db; db = db->next) + { + if (db != monitor->databases) + { + dprintf(file, ","); + } + dprintf(file, "%s", db->server->unique_name); + } + dprintf(file, "\n"); + } + + close(file); + + return true; +} + +bool monitor_serialize_servers(const MONITOR *monitor) +{ + bool rval = false; + char filename[PATH_MAX]; + snprintf(filename, sizeof(filename), "%s/%s.cnf.tmp", get_config_persistdir(), + monitor->name); + + if (unlink(filename) == -1 && errno != ENOENT) + { + char err[MXS_STRERROR_BUFLEN]; + MXS_ERROR("Failed to remove temporary monitor configuration at '%s': %d, %s", + filename, errno, strerror_r(errno, err, sizeof(err))); + } + else if (create_monitor_config(monitor, filename)) + { + char final_filename[PATH_MAX]; + strcpy(final_filename, filename); + + char *dot = strrchr(final_filename, '.'); + ss_dassert(dot); + *dot = '\0'; + + if (rename(filename, final_filename) == 0) + { + rval = true; + } + else + { + char err[MXS_STRERROR_BUFLEN]; + MXS_ERROR("Failed to rename temporary monitor configuration at '%s': %d, %s", + filename, errno, strerror_r(errno, err, sizeof(err))); + } + } + + return rval; +} diff --git a/server/modules/routing/debugcli/debugcmd.c b/server/modules/routing/debugcli/debugcmd.c index 47dde9896..19e33470e 100644 --- a/server/modules/routing/debugcli/debugcmd.c +++ b/server/modules/routing/debugcli/debugcmd.c @@ -753,6 +753,7 @@ static void cmd_AddServer(DCB *dcb, void *a, void *b) else if (monitor) { monitorAddServer(monitor, server); + monitor_serialize_servers(monitor); } const char *target = service ? "service" : "monitor"; @@ -809,6 +810,7 @@ static void cmd_RemoveServer(DCB *dcb, void *a, void *b) else if (monitor) { monitorRemoveServer(monitor, server); + monitor_serialize_servers(monitor); } const char *target = service ? "service" : "monitor"; From 878d01e276aed790d6ac6b90263e6e7bddd2d93e Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Sat, 12 Nov 2016 22:49:27 +0200 Subject: [PATCH 06/10] Persist server changes to services When a service is added or removed from a service, a supplementary configuration file is created. This allows MaxScale to survive restars and unexpected downtime even if runtime changes to the servers of a service have been made. With these changes, it is possible to start MaxScale without any servers, create servers, add the created servers to services and monitors and restart Maxscale without losing the runtime configuration changes. --- include/maxscale/service.h | 18 ++- server/core/service.c | 148 ++++++++++++++++----- server/modules/routing/debugcli/debugcmd.c | 2 + 3 files changed, 136 insertions(+), 32 deletions(-) diff --git a/include/maxscale/service.h b/include/maxscale/service.h index e8fd7850e..95ac954c6 100644 --- a/include/maxscale/service.h +++ b/include/maxscale/service.h @@ -200,7 +200,7 @@ extern int serviceHasProtocol(SERVICE *service, const char *protocol, const char* address, unsigned short port); extern void serviceAddBackend(SERVICE *, SERVER *); extern void serviceRemoveBackend(SERVICE *, const SERVER *); -extern int serviceHasBackend(SERVICE *, SERVER *); +extern bool serviceHasBackend(SERVICE *, SERVER *); extern void serviceAddRouterOption(SERVICE *, char *); extern void serviceClearRouterOptions(SERVICE *); extern int serviceStart(SERVICE *); @@ -264,4 +264,20 @@ static inline uint64_t service_get_capabilities(const SERVICE *service) */ bool service_server_in_use(const SERVER *server); +/** + * @brief Serialize a service to a file + * + * This partially converts @c service into an INI format file. Only the servers + * of the service are serialized. This allows the service to keep using the servers + * added at runtime even after a restart. + * + * NOTE: This does not persist the complete service configuration and requires + * that an existing service configuration is in the main configuration file. + * Changes to service parameters are not persisted. + * + * @param service Service to serialize + * @return False if the serialization of the service fails, true if it was successful + */ +bool service_serialize_servers(const SERVICE *service); + MXS_END_DECLS diff --git a/server/core/service.c b/server/core/service.c index 7fe22d93e..f205338ed 100644 --- a/server/core/service.c +++ b/server/core/service.c @@ -43,6 +43,10 @@ #include #include #include +#include +#include +#include +#include #include #include #include @@ -56,12 +60,9 @@ #include #include #include -#include -#include #include #include #include -#include #include #include #include @@ -769,43 +770,46 @@ static SERVER_REF* server_ref_create(SERVER *server) void serviceAddBackend(SERVICE *service, SERVER *server) { - SERVER_REF *new_ref = server_ref_create(server); - - if (new_ref) + if (!serviceHasBackend(service, server)) { - spinlock_acquire(&service->spin); + SERVER_REF *new_ref = server_ref_create(server); - service->n_dbref++; - - if (service->dbref) + if (new_ref) { - SERVER_REF *ref = service->dbref; - SERVER_REF *prev = ref; + spinlock_acquire(&service->spin); - while (ref) + service->n_dbref++; + + if (service->dbref) { - if (ref->server == server) + SERVER_REF *ref = service->dbref; + SERVER_REF *prev = ref; + + while (ref) { - ref->active = true; - break; + if (ref->server == server) + { + ref->active = true; + break; + } + prev = ref; + ref = ref->next; } - prev = ref; - ref = ref->next; - } - if (ref == NULL) - { - /** A new server that hasn't been used by this service */ - atomic_synchronize(); - prev->next = new_ref; + if (ref == NULL) + { + /** A new server that hasn't been used by this service */ + atomic_synchronize(); + prev->next = new_ref; + } } + else + { + atomic_synchronize(); + service->dbref = new_ref; + } + spinlock_release(&service->spin); } - else - { - atomic_synchronize(); - service->dbref = new_ref; - } - spinlock_release(&service->spin); } } @@ -841,7 +845,7 @@ void serviceRemoveBackend(SERVICE *service, const SERVER *server) * @param server The server to add * @return Non-zero if the server is already part of the service */ -int +bool serviceHasBackend(SERVICE *service, SERVER *server) { SERVER_REF *ptr; @@ -2217,3 +2221,85 @@ bool service_server_in_use(const SERVER *server) return rval; } + +/** + * Creates a service configuration at the location pointed by @c filename + * + * @param service Service to serialize into a configuration + * @param filename Filename where configuration is written + * @return True on success, false on error + */ +static bool create_service_config(const SERVICE *service, const char *filename) +{ + int file = open(filename, O_EXCL | O_CREAT | O_WRONLY, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH); + + if (file == -1) + { + char errbuf[MXS_STRERROR_BUFLEN]; + MXS_ERROR("Failed to open file '%s' when serializing service '%s': %d, %s", + filename, service->name, errno, strerror_r(errno, errbuf, sizeof(errbuf))); + return false; + } + + /** + * Only additional parameters are added to the configuration. This prevents + * duplication or addition of parameters that don't support it. + * + * TODO: Check for return values on all of the dprintf calls + */ + dprintf(file, "[%s]\n", service->name); + if (service->dbref) + { + dprintf(file, "servers="); + for (SERVER_REF *db = service->dbref; db; db = db->next) + { + if (db != service->dbref) + { + dprintf(file, ","); + } + dprintf(file, "%s", db->server->unique_name); + } + dprintf(file, "\n"); + } + + close(file); + + return true; +} + +bool service_serialize_servers(const SERVICE *service) +{ + bool rval = false; + char filename[PATH_MAX]; + snprintf(filename, sizeof(filename), "%s/%s.cnf.tmp", get_config_persistdir(), + service->name); + + if (unlink(filename) == -1 && errno != ENOENT) + { + char err[MXS_STRERROR_BUFLEN]; + MXS_ERROR("Failed to remove temporary service configuration at '%s': %d, %s", + filename, errno, strerror_r(errno, err, sizeof(err))); + } + else if (create_service_config(service, filename)) + { + char final_filename[PATH_MAX]; + strcpy(final_filename, filename); + + char *dot = strrchr(final_filename, '.'); + ss_dassert(dot); + *dot = '\0'; + + if (rename(filename, final_filename) == 0) + { + rval = true; + } + else + { + char err[MXS_STRERROR_BUFLEN]; + MXS_ERROR("Failed to rename temporary service configuration at '%s': %d, %s", + filename, errno, strerror_r(errno, err, sizeof(err))); + } + } + + return rval; +} diff --git a/server/modules/routing/debugcli/debugcmd.c b/server/modules/routing/debugcli/debugcmd.c index 19e33470e..ad568d625 100644 --- a/server/modules/routing/debugcli/debugcmd.c +++ b/server/modules/routing/debugcli/debugcmd.c @@ -749,6 +749,7 @@ static void cmd_AddServer(DCB *dcb, void *a, void *b) if (service) { serviceAddBackend(service, server); + service_serialize_servers(service); } else if (monitor) { @@ -806,6 +807,7 @@ static void cmd_RemoveServer(DCB *dcb, void *a, void *b) if (service) { serviceRemoveBackend(service, server); + service_serialize_servers(service); } else if (monitor) { From a5bb02bd14ac4d03db8a3640030b654d40907324 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Sat, 12 Nov 2016 23:49:26 +0200 Subject: [PATCH 07/10] Expect at least two arguments for `create server` When a server is created in server_create, it sets the port to the default of 3306 if no explicit port is defined. The code that called this function still expected a minimum of three arguments: name, address and port. --- server/modules/routing/debugcli/debugcmd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/modules/routing/debugcli/debugcmd.c b/server/modules/routing/debugcli/debugcmd.c index ad568d625..bf92d5d43 100644 --- a/server/modules/routing/debugcli/debugcmd.c +++ b/server/modules/routing/debugcli/debugcmd.c @@ -984,9 +984,9 @@ static void createServer(DCB *dcb, char *name, char *address, char *port, struct subcommand createoptions[] = { { - "server", 3, 6, createServer, + "server", 2, 6, createServer, "Create a new server", - "Usage: create server NAME HOST PORT [PROTOCOL] [AUTHENTICATOR] [OPTIONS]\n" + "Usage: create server NAME HOST [PORT] [PROTOCOL] [AUTHENTICATOR] [OPTIONS]\n" "Create a new server from the following parameters.\n" "NAME Server name\n" "HOST Server host address\n" From f18cf407a796b48439d198f2cb01a5f08bc3b207 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Sun, 13 Nov 2016 00:18:13 +0200 Subject: [PATCH 08/10] Improve maxadmin error messages The error messages now report more detailed information about the expected number of arguments to commands. --- server/modules/routing/debugcli/debugcmd.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/server/modules/routing/debugcli/debugcmd.c b/server/modules/routing/debugcli/debugcmd.c index bf92d5d43..e076c7ee7 100644 --- a/server/modules/routing/debugcli/debugcmd.c +++ b/server/modules/routing/debugcli/debugcmd.c @@ -1553,12 +1553,29 @@ execute_cmd(CLI_SESSION *cli) if (strcasecmp(args[1], cmds[i].options[j].arg1) == 0) { found = 1; /**< command and sub-command match */ - if (argc < cmds[i].options[j].argc_min) + + if (cmds[i].options[j].argc_min == cmds[i].options[j].argc_max && + argc != cmds[i].options[j].argc_min) { + /** Wrong number of arguments */ + dcb_printf(dcb, "Incorrect number of arguments: %s %s expects %d arguments\n", + cmds[i].cmd, cmds[i].options[j].arg1, + cmds[i].options[j].argc_min); + } + else if (argc < cmds[i].options[j].argc_min) + { + /** Not enough arguments */ dcb_printf(dcb, "Incorrect number of arguments: %s %s expects at least %d arguments\n", cmds[i].cmd, cmds[i].options[j].arg1, cmds[i].options[j].argc_min); } + else if (argc > cmds[i].options[j].argc_max) + { + /** Too many arguments */ + dcb_printf(dcb, "Incorrect number of arguments: %s %s expects at most %d arguments\n", + cmds[i].cmd, cmds[i].options[j].arg1, + cmds[i].options[j].argc_max); + } else { unsigned long arg_list[MAXARGS] = {}; From 2202ec7a334fa1f6cf00ac28c80f42faa0e310c9 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Sun, 13 Nov 2016 10:51:09 +0200 Subject: [PATCH 09/10] Allow multiple monitor alterations at one time The monitor alteration now also uses a list of key-value pairs. This allows multiple changes to be made in one command. --- server/modules/routing/debugcli/debugcmd.c | 79 +++++++++++++++------- 1 file changed, 56 insertions(+), 23 deletions(-) diff --git a/server/modules/routing/debugcli/debugcmd.c b/server/modules/routing/debugcli/debugcmd.c index e076c7ee7..0b19e3bb3 100644 --- a/server/modules/routing/debugcli/debugcmd.c +++ b/server/modules/routing/debugcli/debugcmd.c @@ -1143,7 +1143,7 @@ static void alterServer(DCB *dcb, SERVER *server, char *v1, char *v2, char *v3, } else if (!handle_alter_server(server, key, value)) { - dcb_printf(dcb, "Error: Unknown key-value parameter: %s=%s\n", key, value); + dcb_printf(dcb, "Error: Bad key-value parameter: %s=%s\n", key, value); } } else @@ -1166,11 +1166,10 @@ static void alterServer(DCB *dcb, SERVER *server, char *v1, char *v2, char *v3, * * If the value is not a positive integer, an error is printed to @c dcb. * - * @param dcb Client DCB * @param value String value * @return 0 on error, otherwise a positive integer */ -static long get_positive_int(DCB *dcb, const char *value) +static long get_positive_int(const char *value) { char *endptr; long ival = strtol(value, &endptr, 10); @@ -1180,62 +1179,93 @@ static long get_positive_int(DCB *dcb, const char *value) return ival; } - dcb_printf(dcb, "Invalid value: %s", value); return 0; } -static void alterMonitor(DCB *dcb, MONITOR *monitor, char *key, char *value) +static bool handle_alter_monitor(MONITOR *monitor, char *key, char *value) { - bool unknown = false; + bool valid = false; + if (strcmp(key, "user") == 0) { + valid = true; monitorAddUser(monitor, value, monitor->password); } else if (strcmp(key, "password") == 0) { + valid = true; monitorAddUser(monitor, monitor->user, value); } else if (strcmp(key, "monitor_interval") == 0) { - long ival = get_positive_int(dcb, value); + long ival = get_positive_int(value); if (ival) { + valid = true; monitorSetInterval(monitor, ival); } } else if (strcmp(key, "backend_connect_timeout") == 0) { - long ival = get_positive_int(dcb, value); + long ival = get_positive_int(value); if (ival) { + valid = true; monitorSetNetworkTimeout(monitor, MONITOR_CONNECT_TIMEOUT, ival); } } else if (strcmp(key, "backend_write_timeout") == 0) { - long ival = get_positive_int(dcb, value); + long ival = get_positive_int(value); if (ival) { - monitorSetNetworkTimeout(monitor, MONITOR_READ_TIMEOUT, ival); + valid = true; + monitorSetNetworkTimeout(monitor, MONITOR_WRITE_TIMEOUT, ival); } } else if (strcmp(key, "backend_read_timeout") == 0) { - long ival = get_positive_int(dcb, value); + long ival = get_positive_int(value); if (ival) { - monitorSetNetworkTimeout(monitor, MONITOR_WRITE_TIMEOUT, ival); + valid = true; + monitorSetNetworkTimeout(monitor, MONITOR_READ_TIMEOUT, ival); } } - else + + return valid; +} + +static void alterMonitor(DCB *dcb, MONITOR *monitor, char *v1, char *v2, char *v3, + char *v4, char *v5, char *v6, char *v7, char *v8, char *v9, + char *v10, char *v11) +{ + char *values[11] = {v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11}; + const int items = sizeof(values) / sizeof(values[0]); + + for (int i = 0; i < items; i++) { - unknown = true; + if (values[i]) + { + char *key = values[i]; + char *value = strchr(key, '='); + + if (value) + { + *value++ = '\0'; + + if (!handle_alter_monitor(monitor, key, value)) + { + dcb_printf(dcb, "Error: Bad key-value parameter: %s=%s\n", key, value); + } + } + else + { + dcb_printf(dcb, "Error: not a key-value parameter: %s\n", values[i]); + } + } } - if (unknown) - { - dcb_printf(dcb, "Unknown parameter '%s'", key); - } } struct subcommand alteroptions[] = @@ -1246,19 +1276,22 @@ struct subcommand alteroptions[] = "Usage: alter server NAME KEY=VALUE ...\n" "This will alter an existing parameter of a server. The accepted values\n" "for KEY are: 'address', 'port', 'monuser', 'monpw'\n" - "A maximum of 11 parameters can be changed at one time\n", + "A maximum of 11 parameters can be changed at one time", { ARG_TYPE_SERVER, ARG_TYPE_STRING, ARG_TYPE_STRING, ARG_TYPE_STRING, ARG_TYPE_STRING, ARG_TYPE_STRING, ARG_TYPE_STRING, ARG_TYPE_STRING, ARG_TYPE_STRING, ARG_TYPE_STRING, ARG_TYPE_STRING, ARG_TYPE_STRING} }, { - "monitor", 3, 3, alterMonitor, + "monitor", 2, 12, alterMonitor, "Alter monitor parameters", - "Usage: alter monitor NAME KEY VALUE\n" + "Usage: alter monitor NAME KEY=VALUE ...\n" "This will alter an existing parameter of a monitor. The accepted values\n" "for KEY are: 'user', 'password', 'monitor_interval',\n" - "'backend_connect_timeout', 'backend_write_timeout', 'backend_read_timeout'", - {ARG_TYPE_MONITOR, ARG_TYPE_STRING, ARG_TYPE_STRING} + "'backend_connect_timeout', 'backend_write_timeout', 'backend_read_timeout'\n" + "A maximum of 11 parameters can be changed at one time", + {ARG_TYPE_MONITOR, ARG_TYPE_STRING, ARG_TYPE_STRING, ARG_TYPE_STRING, + ARG_TYPE_STRING, ARG_TYPE_STRING, ARG_TYPE_STRING, ARG_TYPE_STRING, + ARG_TYPE_STRING, ARG_TYPE_STRING, ARG_TYPE_STRING, ARG_TYPE_STRING} }, { EMPTY_OPTION From 548182afe39b7b9b584af977880a84e970106c9e Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Tue, 15 Nov 2016 10:50:06 +0200 Subject: [PATCH 10/10] Fix debug assertion in config.c The debug assertion was triggered due to a wrongly structured conditional statement. --- server/core/config.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/server/core/config.c b/server/core/config.c index 26c1c4784..ea812487f 100644 --- a/server/core/config.c +++ b/server/core/config.c @@ -387,10 +387,12 @@ ini_handler(void *userdata, const char *section, const char *name, const char *v cntxt->next = ptr; } - if (config_get_param(ptr->parameters, name) && - !config_append_param(ptr, name, value)) + if (config_get_param(ptr->parameters, name)) { - return 0; + if (!config_append_param(ptr, name, value)) + { + return 0; + } } else if (!config_add_param(ptr, name, value)) {