From 56ecdc219b057b0e9d9f5e21919650d1e0dbb35b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Sun, 8 Jan 2017 12:00:42 +0200 Subject: [PATCH] Refactor config.c functions for testing To test the configuration validation and default value generation, the functions needed to be refactored to allow parameters to be passed directly to the function. --- include/maxscale/config.h | 26 +- server/core/config.c | 295 +++++++++++---------- server/core/config_runtime.c | 16 +- server/modules/routing/debugcli/debugcmd.c | 18 -- 4 files changed, 188 insertions(+), 167 deletions(-) diff --git a/include/maxscale/config.h b/include/maxscale/config.h index 7a5b02918..b0fd70930 100644 --- a/include/maxscale/config.h +++ b/include/maxscale/config.h @@ -209,24 +209,27 @@ SSL_LISTENER *make_ssl_structure(CONFIG_CONTEXT *obj, bool require_cert, int *er * the given parameters are compared to the expected ones. This function also * does preliminary type checking for various basic values as well as enumerations. * - * @param module Module name - * @param type Module type + * @param params Module parameters * @param key Parameter key * @param value Parameter value * @param context Configuration context or NULL for no context (uses runtime checks) * * @return True if the configuration parameter is valid */ -bool config_param_is_valid(const char *module, const char *type, const char *key, +bool config_param_is_valid(const MXS_MODULE_PARAM *params, const char *key, const char *value, const CONFIG_CONTEXT *context); /** * @brief Get a boolean value * + * The existence of the parameter should be checked with config_get_param() before + * calling this function to determine whether the return value represents an existing + * value or a missing value. + * * @param params List of configuration parameters * @param key Parameter name * - * @return The value as a boolean + * @return The value as a boolean or false if none was found */ bool config_get_bool(const CONFIG_PARAMETER *params, const char *key); @@ -238,7 +241,7 @@ bool config_get_bool(const CONFIG_PARAMETER *params, const char *key); * @param params List of configuration parameters * @param key Parameter name * - * @return The integer value of the parameter + * @return The integer value of the parameter or 0 if no parameter was found */ int config_get_integer(const CONFIG_PARAMETER *params, const char *key); @@ -248,7 +251,7 @@ int config_get_integer(const CONFIG_PARAMETER *params, const char *key); * @param params List of configuration parameters * @param key Parameter name * - * @return The raw string value + * @return The raw string value or an empty string if no parameter was found */ const char* config_get_string(const CONFIG_PARAMETER *params, const char *key); @@ -259,9 +262,11 @@ const char* config_get_string(const CONFIG_PARAMETER *params, const char *key); * @param key Parameter name * @param values All possible enumeration values * - * @return The enumeration value converted to an int + * @return The enumeration value converted to an int or -1 if the parameter was not found * - * TODO: Allow multiple enumeration values + * @note The enumeration values should not use -1 so that an undefined parameter is + * detected. If -1 is used, config_get_param() should be used to detect whether + * the parameter exists */ int config_get_enum(const CONFIG_PARAMETER *params, const char *key, const MXS_ENUM_VALUE *values); @@ -271,12 +276,11 @@ int config_get_enum(const CONFIG_PARAMETER *params, const char *key, const MXS_E * Adds any default parameters to @c ctx that aren't already in it. * * @param ctx Configuration context where the parameters are added - * @param module Module name - * @param type Module type + * @param params Module parameters * * TODO: Move this to a header internal to the MaxScale core */ -void config_add_defaults(CONFIG_CONTEXT *ctx, const char *module, const char *type); +void config_add_defaults(CONFIG_CONTEXT *ctx, const MXS_MODULE_PARAM *params); char* config_clean_string_list(const char* str); CONFIG_PARAMETER* config_clone_param(const CONFIG_PARAMETER* param); diff --git a/server/core/config.c b/server/core/config.c index 3fcb650c2..a1d92b620 100644 --- a/server/core/config.c +++ b/server/core/config.c @@ -943,40 +943,46 @@ CONFIG_PARAMETER* config_get_param(CONFIG_PARAMETER* params, const char* name) bool config_get_bool(const CONFIG_PARAMETER *params, const char *key) { const char *value = config_get_value_string(params, key); - ss_dassert(*value); - return config_truth_value(value); + return *value ? config_truth_value(value) : false; } int config_get_integer(const CONFIG_PARAMETER *params, const char *key) { const char *value = config_get_value_string(params, key); - ss_dassert(*value); - return strtol(value, NULL, 10); + return *value ? strtol(value, NULL, 10) : 0; } const char* config_get_string(const CONFIG_PARAMETER *params, const char *key) { - const char *value = config_get_value_string(params, key); - ss_dassert(*value); - return value; + return config_get_value_string(params, key); } int config_get_enum(const CONFIG_PARAMETER *params, const char *key, const MXS_ENUM_VALUE *enum_values) { const char *value = config_get_value_string(params, key); + char tmp_val[strlen(value) + 1]; + strcpy(tmp_val, value); - ss_dassert(*value); + int rv = 0; + bool found = false; + char *endptr; + const char *delim = ", \t"; + char *tok = strtok_r(tmp_val, delim, &endptr); - for (int i = 0; enum_values[i].name; i++) + while (tok) { - if (strcmp(enum_values[i].name, value) == 0) + for (int i = 0; enum_values[i].name; i++) { - return enum_values[i].enum_value; + if (strcmp(enum_values[i].name, tok) == 0) + { + found = true; + rv |= enum_values[i].enum_value; + } } + tok = strtok_r(NULL, delim, &endptr); } - ss_dassert(false); - return -1; + return found ? rv : -1; } CONFIG_PARAMETER* config_clone_param(const CONFIG_PARAMETER* param) @@ -1715,22 +1721,19 @@ process_config_update(CONFIG_CONTEXT *context) * @param params List of parameters for the object * @return True if at least one of the required parameters is missing */ -static bool missing_required_parameters(const char *name, const char *type, +static bool missing_required_parameters(const MXS_MODULE_PARAM *mod_params, CONFIG_PARAMETER *params) { bool rval = false; - const MXS_MODULE *mod = get_module(name, type); - - if (mod) + if (mod_params) { - for (int i = 0; mod->parameters[i].name; i++) + for (int i = 0; mod_params[i].name; i++) { - if ((mod->parameters[i].options & MXS_MODULE_OPT_REQUIRED) && - config_get_param(params, mod->parameters[i].name) == NULL) + if ((mod_params[i].options & MXS_MODULE_OPT_REQUIRED) && + config_get_param(params, mod_params[i].name) == NULL) { - MXS_ERROR("Mandatory parameter '%s' for module '%s' of type %s is not defined.", - mod->parameters[i].name, name, type); + MXS_ERROR("Mandatory parameter '%s' is not defined.", mod_params[i].name); rval = true; } } @@ -1785,6 +1788,8 @@ check_config_objects(CONFIG_CONTEXT *context) } } + const MXS_MODULE *mod = module ? get_module(module, module_type) : NULL; + if (param_set != NULL) { CONFIG_PARAMETER *params = obj->parameters; @@ -1801,8 +1806,8 @@ check_config_objects(CONFIG_CONTEXT *context) if (found == 0) { - if (module == NULL || - !config_param_is_valid(module, module_type, params->name, params->value, context)) + if (mod == NULL || + !config_param_is_valid(mod->parameters, params->name, params->value, context)) { MXS_ERROR("Unexpected parameter '%s' for object '%s' of type '%s'.", params->name, obj->object, type); @@ -1813,8 +1818,7 @@ check_config_objects(CONFIG_CONTEXT *context) } } - if (module && module_type && - missing_required_parameters(module, module_type, obj->parameters)) + if (mod && missing_required_parameters(mod->parameters, obj->parameters)) { rval = false; } @@ -2384,22 +2388,19 @@ static int validate_ssl_parameters(CONFIG_CONTEXT* obj, char *ssl_cert, char *ss * @param ctx Configuration context where the default parameters are added * @param module Name of the module */ -void config_add_defaults(CONFIG_CONTEXT *ctx, const char *module, const char *type) +void config_add_defaults(CONFIG_CONTEXT *ctx, const MXS_MODULE_PARAM *params) { - const MXS_MODULE *mod = get_module(module, type); - - if (mod) + if (params) { - for (int i = 0; mod->parameters[i].name; i++) + for (int i = 0; params[i].name; i++) { - if (mod->parameters[i].default_value && - config_get_param(ctx->parameters, mod->parameters[i].name) == NULL) + if (params[i].default_value && + config_get_param(ctx->parameters, params[i].name) == NULL) { - ss_dassert(config_param_is_valid(module, type, mod->parameters[i].name, - mod->parameters[i].default_value, ctx)); + ss_dassert(config_param_is_valid(params, params[i].name, + params[i].default_value, ctx)); - bool rv = config_add_param(ctx, mod->parameters[i].name, - mod->parameters[i].default_value); + bool rv = config_add_param(ctx, params[i].name, params[i].default_value); MXS_ABORT_IF_FALSE(rv); } } @@ -2541,8 +2542,17 @@ int create_new_service(CONFIG_CONTEXT *obj) /** Store the configuration parameters for the service */ - config_add_defaults(obj, router, MODULE_ROUTER); - service_add_parameters(obj->element, obj->parameters); + const MXS_MODULE *mod = get_module(router, MODULE_ROUTER); + + if (mod) + { + config_add_defaults(obj, mod->parameters); + service_add_parameters(obj->element, obj->parameters); + } + else + { + error_count++; + } return error_count; } @@ -2772,8 +2782,17 @@ int create_new_monitor(CONFIG_CONTEXT *context, CONFIG_CONTEXT *obj, HASHTABLE* if (error_count == 0) { - config_add_defaults(obj, module, MODULE_MONITOR); - monitorAddParameters(obj->element, obj->parameters); + const MXS_MODULE *mod = get_module(module, MODULE_MONITOR); + + if (mod) + { + config_add_defaults(obj, mod->parameters); + monitorAddParameters(obj->element, obj->parameters); + } + else + { + error_count++; + } char *interval_str = config_get_value(obj->parameters, "monitor_interval"); if (interval_str) @@ -2985,7 +3004,17 @@ int create_new_filter(CONFIG_CONTEXT *obj) } } - config_add_defaults(obj, module, MODULE_FILTER); + const MXS_MODULE *mod = get_module(module, MODULE_FILTER); + + if (mod) + { + config_add_defaults(obj, mod->parameters); + } + else + { + error_count++; + } + CONFIG_PARAMETER *params = obj->parameters; while (params) @@ -3064,116 +3093,112 @@ static bool config_contains_type(const CONFIG_CONTEXT *ctx, const char *name, co return false; } -bool config_param_is_valid(const char *module, const char *type, const char *key, +bool config_param_is_valid(const MXS_MODULE_PARAM *params, const char *key, const char *value, const CONFIG_CONTEXT *context) { bool valid = false; - const MXS_MODULE *mod = get_module(module, type); - if (mod) + for (int i = 0; params[i].name && !valid; i++) { - for (int i = 0; mod->parameters[i].name && !valid; i++) + if (strcmp(params[i].name, key) == 0) { - if (strcmp(mod->parameters[i].name, key) == 0) + char *endptr; + + switch (params[i].type) { - char *endptr; + case MXS_MODULE_PARAM_COUNT: + if ((strtol(value, &endptr, 10)) >= 0 && endptr != value && *endptr == '\0') + { + valid = true; + } + break; - switch (mod->parameters[i].type) - { - case MXS_MODULE_PARAM_COUNT: - if ((strtol(value, &endptr, 10)) >= 0 && endptr != value && *endptr == '\0') + case MXS_MODULE_PARAM_INT: + strtol(value, &endptr, 10); + if (endptr != value && *endptr == '\0') + { + valid = true; + } + break; + + case MXS_MODULE_PARAM_BOOL: + if (config_truth_value(value) != -1) + { + valid = true; + } + break; + + case MXS_MODULE_PARAM_STRING: + if (*value) + { + valid = true; + } + break; + + case MXS_MODULE_PARAM_ENUM: + if (params[i].accepted_values) + { + for (int j = 0; params[i].accepted_values[j].name; j++) { - valid = true; - } - break; - - case MXS_MODULE_PARAM_INT: - strtol(value, &endptr, 10); - if (endptr != value && *endptr == '\0') - { - valid = true; - } - break; - - case MXS_MODULE_PARAM_BOOL: - if (config_truth_value(value) != -1) - { - valid = true; - } - break; - - case MXS_MODULE_PARAM_STRING: - if (*value) - { - valid = true; - } - break; - - case MXS_MODULE_PARAM_ENUM: - if (mod->parameters[i].accepted_values) - { - for (int j = 0; mod->parameters[i].accepted_values[j].name; j++) - { - if (strcmp(mod->parameters[i].accepted_values[j].name, value) == 0) - { - valid = true; - break; - } - } - } - break; - - case MXS_MODULE_PARAM_SERVICE: - if ((context && config_contains_type(context, "service", value)) || - service_find(value)) - { - valid = true; - } - break; - - case MXS_MODULE_PARAM_PATH: - if (mod->parameters[i].options & (MXS_MODULE_OPT_PATH_W_OK | - MXS_MODULE_OPT_PATH_R_OK | - MXS_MODULE_OPT_PATH_X_OK | - MXS_MODULE_OPT_PATH_F_OK)) - { - int mode = F_OK; - if (mod->parameters[i].options & MXS_MODULE_OPT_PATH_W_OK) - { - mode |= W_OK; - } - if (mod->parameters[i].options & MXS_MODULE_OPT_PATH_R_OK) - { - mode |= R_OK; - } - if (mod->parameters[i].options & MXS_MODULE_OPT_PATH_X_OK) - { - mode |= X_OK; - } - - if (access(value, mode) == 0) + if (strcmp(params[i].accepted_values[j].name, value) == 0) { valid = true; + break; } - else - { - char err[MXS_STRERROR_BUFLEN]; - MXS_ERROR("Bad path parameter '%s': %d, %s", value, - errno, strerror_r(errno, err, sizeof(err))); - } + } + } + break; + + case MXS_MODULE_PARAM_SERVICE: + if ((context && config_contains_type(context, value, "service")) || + service_find(value)) + { + valid = true; + } + break; + + case MXS_MODULE_PARAM_PATH: + if (params[i].options & (MXS_MODULE_OPT_PATH_W_OK | + MXS_MODULE_OPT_PATH_R_OK | + MXS_MODULE_OPT_PATH_X_OK | + MXS_MODULE_OPT_PATH_F_OK)) + { + int mode = F_OK; + if (params[i].options & MXS_MODULE_OPT_PATH_W_OK) + { + mode |= W_OK; + } + if (params[i].options & MXS_MODULE_OPT_PATH_R_OK) + { + mode |= R_OK; + } + if (params[i].options & MXS_MODULE_OPT_PATH_X_OK) + { + mode |= X_OK; + } + + if (access(value, mode) == 0) + { + valid = true; } else { - /** No checks for the path are required */ - valid = true; + char err[MXS_STRERROR_BUFLEN]; + MXS_ERROR("Bad path parameter '%s': %d, %s", value, + errno, strerror_r(errno, err, sizeof(err))); } - break; + } + else + { + /** No checks for the path are required */ + valid = true; + } + break; - default: - MXS_ERROR("Unexpected module parameter type: %d", mod->parameters[i].type); - ss_dassert(false); - break; - } + default: + MXS_ERROR("Unexpected module parameter type: %d", params[i].type); + ss_dassert(false); + break; } } } diff --git a/server/core/config_runtime.c b/server/core/config_runtime.c index 3a1631c1e..9e3d4fcef 100644 --- a/server/core/config_runtime.c +++ b/server/core/config_runtime.c @@ -347,9 +347,19 @@ static void add_monitor_defaults(MONITOR *monitor) /** Inject the default module parameters in case we only deleted * a parameter */ CONFIG_CONTEXT ctx = {.object = ""}; - config_add_defaults(&ctx, monitor->module_name, MODULE_MONITOR); - monitorAddParameters(monitor, ctx.parameters); - config_parameter_free(ctx.parameters); + const MXS_MODULE *mod = get_module(monitor->module_name, MODULE_MONITOR); + + if (mod) + { + config_add_defaults(&ctx, mod->parameters); + monitorAddParameters(monitor, ctx.parameters); + config_parameter_free(ctx.parameters); + } + else + { + MXS_ERROR("Failed to load module '%s'. See previous error messages for more details.", + monitor->module_name); + } } bool runtime_alter_monitor(MONITOR *monitor, char *key, char *value) diff --git a/server/modules/routing/debugcli/debugcmd.c b/server/modules/routing/debugcli/debugcmd.c index db1877877..dd458597e 100644 --- a/server/modules/routing/debugcli/debugcmd.c +++ b/server/modules/routing/debugcli/debugcmd.c @@ -635,8 +635,6 @@ static void enable_sess_log_action(DCB *dcb, char *arg1, char *arg2); static void disable_sess_log_action(DCB *dcb, char *arg1, char *arg2); static void enable_sess_log_priority(DCB *dcb, char *arg1, char *arg2); static void disable_sess_log_priority(DCB *dcb, char *arg1, char *arg2); -static void enable_monitor_replication_heartbeat(DCB *dcb, MONITOR *monitor); -static void disable_monitor_replication_heartbeat(DCB *dcb, MONITOR *monitor); static void enable_service_root(DCB *dcb, SERVICE *service); static void disable_service_root(DCB *dcb, SERVICE *service); static void enable_feedback_action(); @@ -653,14 +651,6 @@ static void disable_account(DCB *, char *user); * */ struct subcommand enableoptions[] = { - { - "heartbeat", - 1, 1, - enable_monitor_replication_heartbeat, - "Enable monitor replication heartbeat", - "Enable the monitor replication heartbeat, the parameter is the monitor name", - {ARG_TYPE_MONITOR, 0, 0} - }, { "log", 1, 1, @@ -750,14 +740,6 @@ struct subcommand enableoptions[] = * */ struct subcommand disableoptions[] = { - { - "heartbeat", - 1, 1, - disable_monitor_replication_heartbeat, - "Disable replication heartbeat", - "Disable the monitor replication heartbeat", - {ARG_TYPE_MONITOR, 0, 0} - }, { "log", 1, 1,