From 7cb969b0d9b32c48d374d3d1080c887d0da50460 Mon Sep 17 00:00:00 2001 From: Esa Korhonen Date: Wed, 30 Jan 2019 12:22:34 +0200 Subject: [PATCH] MXS-2271 Clean up server list parsing functions, use in monitor config The functions now return the parsed array. --- include/maxscale/config.hh | 35 ++-- include/maxscale/monitor.hh | 14 +- include/maxscale/server.hh | 17 +- server/core/config.cc | 167 +++++------------- server/core/monitor.cc | 69 ++++---- server/core/server.cc | 29 +-- .../namedserverfilter/namedserverfilter.cc | 53 ++---- .../modules/monitor/mariadbmon/mariadbmon.cc | 22 ++- 8 files changed, 143 insertions(+), 263 deletions(-) diff --git a/include/maxscale/config.hh b/include/maxscale/config.hh index 6e466c39a..12a21a0a1 100644 --- a/include/maxscale/config.hh +++ b/include/maxscale/config.hh @@ -24,6 +24,7 @@ #include #include #include +#include #include #include @@ -433,20 +434,20 @@ SERVICE* config_get_service(const MXS_CONFIG_PARAMETER* params, const char* key) * * @return Pointer to configured server */ -struct SERVER* config_get_server(const MXS_CONFIG_PARAMETER* params, const char* key); +SERVER* config_get_server(const MXS_CONFIG_PARAMETER* params, const char* key); /** - * @brief Get an array of servers. The caller should free the produced array, - * but not the array elements. + * Get a serverlist value. The returned list is empty if even a partial error occurs. * * @param params List of configuration parameters * @param key Parameter name - * @param output Where to save the output - * @return How many servers were found, equal to output array size + * @param name_error_out If a server name was not found, it is written here. Only the first such name + * is written. + * @return Found servers. If even one server name was invalid, the array will be empty. */ -int config_get_server_list(const MXS_CONFIG_PARAMETER* params, - const char* key, - struct SERVER*** output); +std::vector +config_get_server_list(const MXS_CONFIG_PARAMETER* params, const char* key, + std::string* name_error_out = nullptr); /** * Get a compiled regular expression and the ovector size of the pattern. The @@ -488,22 +489,14 @@ bool config_get_compiled_regexes(const MXS_CONFIG_PARAMETER* params, uint32_t options, uint32_t* out_ovec_size, pcre2_code** out_codes[]); + /** - * Parse a list of server names and write the results in an array of strings - * with one server name in each. The output array and its elements should be - * deallocated by the caller. The server names are not checked to be actual - * configured servers. + * Break a comma-separated list into a string array. Removes whitespace from list items. * - * The output array may contain more elements than the the value returned, but these - * extra elements are null and in the end of the array. If no server names were - * parsed or if an error occurs, nothing is written to the output parameter. - * - * @param servers A list of server names - * @param output_array Where to save the output - * @return How many servers were found and set into the array. 0 on error or if - * none were found. + * @param list_string A list of items + * @return The array */ -int config_parse_server_list(const char* servers, char*** output_array); +std::vector config_break_list_string(const char* list_string); /** * @brief Get copy of parameter value if it is defined diff --git a/include/maxscale/monitor.hh b/include/maxscale/monitor.hh index d8a401af1..4c6b86c1c 100644 --- a/include/maxscale/monitor.hh +++ b/include/maxscale/monitor.hh @@ -545,19 +545,17 @@ MXS_MONITORED_SERVER* mon_get_monitored_server(const Monitor* mon, SERVER* searc /** * Get an array of monitored servers. If a server defined in the config setting is not monitored by - * the given monitor, that server is ignored and not inserted into the output array. + * the given monitor, the returned array will be empty. * * @param params Config parameters * @param key Setting name * @param mon Monitor which should monitor the servers - * @param monitored_servers_out Where to save output array. The caller should free the array, but not the - * elements. The output must contain NULL before calling this function. - * @return Output array size. + * @param error_out Set to true if an error occurs + * @return Output array */ -int mon_config_get_servers(const MXS_CONFIG_PARAMETER* params, - const char* key, - const Monitor* mon, - MXS_MONITORED_SERVER*** monitored_array_out); +std::vector mon_config_get_servers(const MXS_CONFIG_PARAMETER* params, + const char* key, const Monitor* mon, + bool* error_out); // Function for waiting one monitor interval void monitor_debug_wait(); diff --git a/include/maxscale/server.hh b/include/maxscale/server.hh index cd1ad65f9..66a64866a 100644 --- a/include/maxscale/server.hh +++ b/include/maxscale/server.hh @@ -448,19 +448,14 @@ public: static SERVER* find_by_unique_name(const std::string& name); /** - * Find several servers with the names specified in an array with a given size. - * The returned array (but not the elements) should be freed by the caller. - * If no valid server names were found or in case of error, nothing is written - * to the output parameter. + * Find several servers with the names specified in an array. The returned array is equal in size + * to the server_names-array. If any server name was not found, then the corresponding element + * will be NULL. * - * @param servers An array of server names - * @param size Number of elements in the input server names array, equal to output - * size if any servers are found. - * @param output Where to save the output. Contains null elements for invalid server - * names. If all were invalid, the output is left untouched. - * @return Number of valid server names found + * @param server_names An array of server names + * @return Array of servers */ - static int server_find_by_unique_names(char** server_names, int size, SERVER*** output); + static std::vector server_find_by_unique_names(const std::vector& server_names); /** * Convert the current server status flags to a string. diff --git a/server/core/config.cc b/server/core/config.cc index 5c7a57d48..c75cec84b 100644 --- a/server/core/config.cc +++ b/server/core/config.cc @@ -360,7 +360,7 @@ const MXS_MODULE_PARAM config_monitor_params[] = {CN_PASSWORD, MXS_MODULE_PARAM_STRING, NULL,MXS_MODULE_OPT_REQUIRED }, {"passwd", MXS_MODULE_PARAM_STRING}, - {CN_SERVERS, MXS_MODULE_PARAM_STRING}, + {CN_SERVERS, MXS_MODULE_PARAM_SERVERLIST}, {CN_MONITOR_INTERVAL, MXS_MODULE_PARAM_COUNT, "2000"}, {CN_BACKEND_CONNECT_TIMEOUT, MXS_MODULE_PARAM_COUNT, "3"}, {CN_BACKEND_READ_TIMEOUT, MXS_MODULE_PARAM_COUNT, "1"}, @@ -1814,46 +1814,26 @@ SERVER* config_get_server(const MXS_CONFIG_PARAMETER* params, const char* key) return Server::find_by_unique_name(value); } -int config_get_server_list(const MXS_CONFIG_PARAMETER* params, - const char* key, - SERVER*** output) +std::vector config_get_server_list(const MXS_CONFIG_PARAMETER* params, const char* key, + string* name_error_out) { - const char* value = config_get_value_string(params, key); - char** server_names = NULL; - int found = 0; - const int n_names = config_parse_server_list(value, &server_names); - if (n_names > 0) + const char* names_list = config_get_value_string(params, key); + auto server_names = config_break_list_string(names_list); + std::vector server_arr = SERVER::server_find_by_unique_names(server_names); + for (size_t i = 0; i < server_arr.size(); i++) { - SERVER** servers; - found = SERVER::server_find_by_unique_names(server_names, n_names, &servers); - for (int i = 0; i < n_names; i++) + if (server_arr[i] == nullptr) { - MXS_FREE(server_names[i]); - } - MXS_FREE(server_names); - - if (found) - { - /* Fill in the result array */ - SERVER** result = (SERVER**)MXS_CALLOC(found, sizeof(SERVER*)); - if (result) + if (name_error_out) { - int res_ind = 0; - for (int i = 0; i < n_names; i++) - { - if (servers[i]) - { - result[res_ind] = servers[i]; - res_ind++; - } - } - *output = result; - mxb_assert(found == res_ind); + *name_error_out = server_names[i]; } - MXS_FREE(servers); + // If even one server name was not found, the parameter is in error. + server_arr.clear(); + break; } } - return found; + return server_arr; } char* config_copy_string(const MXS_CONFIG_PARAMETER* params, const char* key) @@ -3830,25 +3810,28 @@ int create_new_monitor(CONFIG_CONTEXT* obj, std::set& monitored_ser { bool err = false; - // TODO: Use server list parameter type for this - for (auto& s : mxs::strtok(config_get_string(obj->parameters, CN_SERVERS), ",")) + 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)) { - fix_object_name(s); - auto server = Server::find_by_unique_name(s); - - if (!server) + string name_not_found; + auto servers = config_get_server_list(params, CN_SERVERS, &name_not_found); + if (servers.empty()) { - MXS_ERROR("Unable to find server '%s' that is configured in " - "the monitor '%s'.", - s.c_str(), - obj->object); err = true; + mxb_assert(!name_not_found.empty()); + MXS_ERROR("Unable to find server '%s' that is configured in monitor '%s'.", + name_not_found.c_str(), obj->object); } - else if (monitored_servers.insert(s.c_str()).second == false) + for (auto server : servers) { - MXS_WARNING("Multiple monitors are monitoring server [%s]. " - "This will cause undefined behavior.", - s.c_str()); + mxb_assert(server); + if (monitored_servers.insert(server->name()).second == false) + { + MXS_WARNING("Multiple monitors are monitoring server [%s]. " + "This will cause undefined behavior.", server->name()); + } } } @@ -4369,22 +4352,19 @@ bool config_param_is_valid(const MXS_MODULE_PARAM* params, case MXS_MODULE_PARAM_SERVERLIST: if (context) { - valid = true; - char** server_names = NULL; - int n_serv = config_parse_server_list(value, &server_names); - if (n_serv > 0) + auto server_names = config_break_list_string(value); + if (!server_names.empty()) { + valid = true; /* Check that every server name in the list is found in the config. */ - for (int i = 0; i < n_serv; i++) + for (auto elem : server_names) { - if (valid - && !config_contains_type(context, server_names[i], CN_SERVER)) + if (!config_contains_type(context, elem.c_str(), CN_SERVER)) { valid = false; + break; } - MXS_FREE(server_names[i]); } - MXS_FREE(server_names); } break; } @@ -4405,74 +4385,17 @@ bool config_param_is_valid(const MXS_MODULE_PARAM* params, return valid; } -int config_parse_server_list(const char* servers, char*** output_array) +std::vector config_break_list_string(const char* list_string) { - mxb_assert(servers); - - /* First, check the string for the maximum amount of servers it - * might contain by counting the commas. */ - int out_arr_size = 1; - const char* pos = servers; - while ((pos = strchr(pos, ',')) != NULL) + mxb_assert(list_string); + string copy = list_string; + /* Parse the elements from the list. They are separated by ',' and are trimmed of whitespace. */ + std::vector tokenized = mxs::strtok(copy, ","); + for (auto& elem : tokenized) { - pos++; - out_arr_size++; + fix_object_name(elem); } - char** results = (char**)MXS_CALLOC(out_arr_size, sizeof(char*)); - if (!results) - { - return 0; - } - - /* Parse the server names from the list. They are separated by ',' and will - * be trimmed of whitespace. */ - char srv_list_tmp[strlen(servers) + 1]; - strcpy(srv_list_tmp, servers); - mxb::trim(srv_list_tmp); - - bool error = false; - int output_ind = 0; - char* lasts; - char* s = strtok_r(srv_list_tmp, ",", &lasts); - while (s) - { - char srv_name_tmp[strlen(s) + 1]; - strcpy(srv_name_tmp, s); - fix_object_name(srv_name_tmp); - - if (strlen(srv_name_tmp) > 0) - { - results[output_ind] = MXS_STRDUP(srv_name_tmp); - if (!results[output_ind]) - { - error = true; - break; - } - output_ind++; - } - s = strtok_r(NULL, ",", &lasts); - } - - if (error) - { - int i = 0; - while (results[i]) - { - MXS_FREE(results[i]); - i++; - } - output_ind = 0; - } - - if (output_ind == 0) - { - MXS_FREE(results); - } - else - { - *output_array = results; - } - return output_ind; + return tokenized; } json_t* config_maxscale_to_json(const char* host) diff --git a/server/core/monitor.cc b/server/core/monitor.cc index 22dbfa742..fb54d1bf7 100644 --- a/server/core/monitor.cc +++ b/server/core/monitor.cc @@ -149,7 +149,7 @@ static uint64_t all_server_bits = SERVER_RUNNING | SERVER_MAINT | SERVER_MASTER | SERVER_JOINED | SERVER_NDB; Monitor* MonitorManager::create_monitor(const string& name, const string& module, - MXS_CONFIG_PARAMETER* params) + MXS_CONFIG_PARAMETER* params) { MXS_MONITOR_API* api = (MXS_MONITOR_API*)load_module(module.c_str(), MODULE_MONITOR); if (!api) @@ -200,10 +200,12 @@ bool Monitor::configure_base(const MXS_CONFIG_PARAMETER* params) m_settings.conn_settings.username = config_get_string(params, CN_USER); m_settings.conn_settings.password = config_get_string(params, CN_PASSWORD); - for (auto& s : mxs::strtok(config_get_string(params, CN_SERVERS), ",")) + // The monitor serverlist has already been checked to be valid. Empty value is ok too. + auto servers_temp = config_get_server_list(params, CN_SERVERS); + for (auto elem : servers_temp) { - fix_object_name(s); - monitor_add_server(this, Server::find_by_unique_name(s)); + // This function checks if server is already monitored. TODO: This should be a config error. + monitor_add_server(this, elem); } /* The previous config values were normal types and were checked by the config manager @@ -2259,49 +2261,48 @@ MXS_MONITORED_SERVER* mon_get_monitored_server(const Monitor* mon, SERVER* searc return NULL; } -int mon_config_get_servers(const MXS_CONFIG_PARAMETER* params, - const char* key, - const Monitor* mon, - MXS_MONITORED_SERVER*** monitored_servers_out) +std::vector mon_config_get_servers(const MXS_CONFIG_PARAMETER* params, + const char* key, const Monitor* mon, + bool* error_out) { - mxb_assert(monitored_servers_out != NULL && *monitored_servers_out == NULL); - SERVER** servers = NULL; - int servers_size = config_get_server_list(params, key, &servers); - int found = 0; - // All servers in the array must be monitored by the given monitor. - if (servers_size > 0) + std::vector monitored_array; + // Check that value exists. + if (*config_get_string(params, key) == '\0') { - MXS_MONITORED_SERVER** monitored_array = - (MXS_MONITORED_SERVER**)MXS_CALLOC(servers_size, sizeof(MXS_MONITORED_SERVER*)); - for (int i = 0; i < servers_size; i++) + return monitored_array; + } + + string name_error; + auto servers = config_get_server_list(params, key, &name_error); + if (!servers.empty()) + { + // All servers in the array must be monitored by the given monitor. + for (auto elem : servers) { - MXS_MONITORED_SERVER* mon_serv = mon_get_monitored_server(mon, servers[i]); - if (mon_serv != NULL) + MXS_MONITORED_SERVER* mon_serv = mon_get_monitored_server(mon, elem); + if (mon_serv) { - monitored_array[found++] = mon_serv; + monitored_array.push_back(mon_serv); } else { - MXS_WARNING("Server '%s' is not monitored by monitor '%s'.", - servers[i]->name(), - mon->m_name); + MXS_ERROR("Server '%s' is not monitored by monitor '%s'.", elem->name(), mon->m_name); + *error_out = true; } } - MXS_FREE(servers); - mxb_assert(found <= servers_size); - if (found == 0) + if (monitored_array.size() < servers.size()) { - MXS_FREE(monitored_array); - monitored_array = NULL; + monitored_array.clear(); } - else if (found < servers_size) - { - monitored_array = (MXS_MONITORED_SERVER**)MXS_REALLOC(monitored_array, found); - } - *monitored_servers_out = monitored_array; } - return found; + else + { + MXS_ERROR("Serverlist setting '%s' contains invalid server name '%s'.", key, name_error.c_str()); + *error_out = true; + } + + return monitored_array; } bool Monitor::set_disk_space_threshold(const string& dst_setting) diff --git a/server/core/server.cc b/server/core/server.cc index 2bea681f8..b92b5bcfe 100644 --- a/server/core/server.cc +++ b/server/core/server.cc @@ -370,32 +370,15 @@ Server* Server::find_by_unique_name(const string& name) return rval; } -int SERVER::server_find_by_unique_names(char** server_names, int size, SERVER*** output) +std::vector SERVER::server_find_by_unique_names(const std::vector& server_names) { - mxb_assert(server_names && (size > 0)); - - SERVER** results = (SERVER**)MXS_CALLOC(size, sizeof(SERVER*)); - if (!results) + std::vector rval; + rval.reserve(server_names.size()); + for (auto elem : server_names) { - return 0; + rval.push_back(Server::find_by_unique_name(elem)); } - - int found = 0; - for (int i = 0; i < size; i++) - { - results[i] = Server::find_by_unique_name(server_names[i]); - found += (results[i]) ? 1 : 0; - } - - if (found) - { - *output = results; - } - else - { - MXS_FREE(results); - } - return found; + return rval; } void Server::printServer() diff --git a/server/modules/filter/namedserverfilter/namedserverfilter.cc b/server/modules/filter/namedserverfilter/namedserverfilter.cc index 7a64f1a77..370014570 100644 --- a/server/modules/filter/namedserverfilter/namedserverfilter.cc +++ b/server/modules/filter/namedserverfilter/namedserverfilter.cc @@ -468,6 +468,7 @@ json_t* RegexHintFilter::diagnostics_json() const * list. Server names are verified to be valid servers. * * @param server_names The list of servers as read from the config file + * @param legacy_mode Using legacy mode for backwards compatibility? * @return How many were found */ int RegexToServers::add_servers(const std::string& server_names, bool legacy_mode) @@ -480,63 +481,48 @@ int RegexToServers::add_servers(const std::string& server_names, bool legacy_mod } /* Have to parse the server list here instead of in config loader, since the list - * may contain special placeholder strings. - */ + * may contain special placeholder strings. */ bool error = false; - char** names_arr = NULL; - const int n_names = config_parse_server_list(server_names.c_str(), &names_arr); - - if (n_names > 1) + auto names_arr = config_break_list_string(server_names.c_str()); + if (names_arr.size() > 1) { - /* The string contains a server list, all must be valid servers */ - SERVER** servers; - int found = SERVER::server_find_by_unique_names(names_arr, n_names, &servers); - - if (found != n_names) + /* The string contains a server list. Check that all names are valid. */ + auto servers = SERVER::server_find_by_unique_names(names_arr); + for (size_t i = 0; i < servers.size(); i++) { - error = true; - - for (int i = 0; i < n_names; i++) + if (servers[i] == nullptr) { - /* servers is valid only if found > 0 */ - if (!found || !servers[i]) - { - MXS_ERROR("'%s' is not a valid server name.", names_arr[i]); - } + error = true; + MXS_ERROR("'%s' is not a valid server name.", names_arr[i].c_str()); } } - if (found) - { - MXS_FREE(servers); - } - if (!error) { - for (int i = 0; i < n_names; i++) + for (auto elem : names_arr) { - m_targets.push_back(names_arr[i]); + m_targets.push_back(elem); } } } - else if (n_names == 1) + else if (names_arr.size() == 1) { /* The string is either a server name or a special reserved id */ if (SERVER::find_by_unique_name(names_arr[0])) { m_targets.push_back(names_arr[0]); } - else if (strcmp(names_arr[0], "->master") == 0) + else if (names_arr[0] == "->master") { m_targets.push_back(names_arr[0]); m_htype = HINT_ROUTE_TO_MASTER; } - else if (strcmp(names_arr[0], "->slave") == 0) + else if (names_arr[0] == "->slave") { m_targets.push_back(names_arr[0]); m_htype = HINT_ROUTE_TO_SLAVE; } - else if (strcmp(names_arr[0], "->all") == 0) + else if (names_arr[0] == "->all") { m_targets.push_back(names_arr[0]); m_htype = HINT_ROUTE_TO_ALL; @@ -551,12 +537,7 @@ int RegexToServers::add_servers(const std::string& server_names, bool legacy_mod error = true; } - for (int i = 0; i < n_names; i++) - { - MXS_FREE(names_arr[i]); - } - MXS_FREE(names_arr); - return error ? 0 : n_names; + return error ? 0 : names_arr.size(); } bool RegexHintFilter::regex_compile_and_add(int pcre_ops, diff --git a/server/modules/monitor/mariadbmon/mariadbmon.cc b/server/modules/monitor/mariadbmon/mariadbmon.cc index 070d4b89d..92d0aaf4d 100644 --- a/server/modules/monitor/mariadbmon/mariadbmon.cc +++ b/server/modules/monitor/mariadbmon/mariadbmon.cc @@ -210,15 +210,21 @@ bool MariaDBMonitor::configure(const MXS_CONFIG_PARAMETER* params) m_handle_event_scheduler = config_get_bool(params, CN_HANDLE_EVENTS); m_excluded_servers.clear(); - MXS_MONITORED_SERVER** excluded_array = NULL; - int n_excluded = mon_config_get_servers(params, CN_NO_PROMOTE_SERVERS, m_monitor, &excluded_array); - for (int i = 0; i < n_excluded; i++) - { - m_excluded_servers.push_back(get_server(excluded_array[i])); - } - MXS_FREE(excluded_array); - bool settings_ok = true; + bool list_error = false; + auto excluded = mon_config_get_servers(params, CN_NO_PROMOTE_SERVERS, m_monitor, &list_error); + if (list_error) + { + settings_ok = false; + } + else + { + for (auto elem : excluded) + { + m_excluded_servers.push_back(get_server(elem)); + } + } + if (!check_sql_files()) { settings_ok = false;