From d5c78eb31f1bfc1025fddbb618e9e6c2538c1890 Mon Sep 17 00:00:00 2001 From: Esa Korhonen Date: Mon, 7 Jan 2019 13:33:31 +0200 Subject: [PATCH] MXS-2220 Move more server functions inside class --- include/maxscale/server.hh | 79 ++++++-------- server/core/config.cc | 10 +- server/core/config_runtime.cc | 4 +- server/core/internal/server.hh | 33 +++++- server/core/modulecmd.cc | 3 +- server/core/monitor.cc | 2 +- server/core/resource.cc | 8 +- server/core/server.cc | 100 ++++++------------ server/core/test/test_server.cc | 8 +- .../authenticator/MySQLAuth/dbusers.cc | 4 +- .../namedserverfilter/namedserverfilter.cc | 4 +- server/modules/routing/debugcli/debugcmd.cc | 2 +- .../modules/routing/maxinfo/maxinfo_exec.cc | 4 +- 13 files changed, 119 insertions(+), 142 deletions(-) diff --git a/include/maxscale/server.hh b/include/maxscale/server.hh index aa7321f09..23f8f1ed7 100644 --- a/include/maxscale/server.hh +++ b/include/maxscale/server.hh @@ -212,13 +212,6 @@ public: */ virtual MxsDiskSpaceThreshold get_disk_space_limits() const = 0; - /** - * Set new disk space limits for the server. - * - * @param new_limits New limits - */ - virtual void set_disk_space_limits(const MxsDiskSpaceThreshold& new_limits) = 0; - /** * Is persistent connection pool enabled. * @@ -291,6 +284,14 @@ public: */ void update_extra_port(int new_port); + /** + * @brief Check if a server points to a local MaxScale service + * + * @param server Server to check + * @return True if the server points to a local MaxScale service + */ + bool is_mxs_service(); + /** * Is the server valid and active? TODO: Rename once "is_active" is moved to internal class. * @@ -406,6 +407,29 @@ public: return status_is_disk_space_exhausted(status); } + /** + * Find a server with the specified name. + * + * @param name Name of the server + * @return The server or NULL if not found + */ + 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. + * + * @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 + */ + static int server_find_by_unique_names(char** server_names, int size, SERVER*** output); + protected: SERVER() { @@ -414,42 +438,6 @@ private: static const int DEFAULT_CHARSET = 0x08; /** The latin1 charset */ }; -/** - * @brief Add a server parameter - * - * @param server Server where the parameter is added - * @param name Parameter name - * @param value Parameter value - */ -void server_add_parameter(SERVER* server, const char* name, const char* value); - -/** - * @brief Check if a server points to a local MaxScale service - * - * @param server Server to check - * @return True if the server points to a local MaxScale service - */ -bool server_is_mxs_service(const SERVER* server); - -/** - * @brief Convert all servers into JSON format - * - * @param host Hostname of this server - * - * @return JSON array of servers or NULL if an error occurred - */ -json_t* server_list_to_json(const char* host); - -/** - * @brief Set the disk space threshold of the server - * - * @param server The server. - * @param disk_space_threshold The disk space threshold as specified in the config file. - * - * @return True, if the provided string is valid and the threshold could be set. - */ -bool server_set_disk_space_threshold(SERVER* server, const char* disk_space_threshold); - /** * @brief Add a response average to the server response average. * @@ -461,8 +449,6 @@ bool server_set_disk_space_threshold(SERVER* server, const char* disk_space_thre void server_add_response_average(SERVER* server, double ave, int num_samples); extern int server_free(SERVER* server); -extern SERVER* server_find_by_unique_name(const char* name); -extern int server_find_by_unique_names(char** server_names, int size, SERVER*** output); extern void server_clear_set_status_nolock(SERVER* server, uint64_t bits_to_clear, uint64_t bits_to_set); extern void server_set_status_nolock(SERVER* server, uint64_t bit); extern void server_clear_status_nolock(SERVER* server, uint64_t bit); @@ -470,9 +456,6 @@ extern void server_transfer_status(SERVER* dest_server, const SERVER* source extern void server_update_address(SERVER* server, const char* address); extern uint64_t server_map_status(const char* str); -extern void printServer(const SERVER*); -extern void printAllServers(); - int server_response_time_num_samples(const SERVER* server); double server_response_time_average(const SERVER* server); diff --git a/server/core/config.cc b/server/core/config.cc index 5b744125f..c84700642 100644 --- a/server/core/config.cc +++ b/server/core/config.cc @@ -1797,7 +1797,7 @@ SERVICE* config_get_service(const MXS_CONFIG_PARAMETER* params, const char* key) SERVER* config_get_server(const MXS_CONFIG_PARAMETER* params, const char* key) { const char* value = config_get_value_string(params, key); - return server_find_by_unique_name(value); + return Server::find_by_unique_name(value); } int config_get_server_list(const MXS_CONFIG_PARAMETER* params, @@ -1811,7 +1811,7 @@ int config_get_server_list(const MXS_CONFIG_PARAMETER* params, if (n_names > 0) { SERVER** servers; - found = server_find_by_unique_names(server_names, n_names, &servers); + found = SERVER::server_find_by_unique_names(server_names, n_names, &servers); for (int i = 0; i < n_names; i++) { MXS_FREE(server_names[i]); @@ -3673,7 +3673,7 @@ int create_new_service(CONFIG_CONTEXT* obj) { fix_object_name(a); - if (SERVER* s = server_find_by_unique_name(a.c_str())) + if (auto s = Server::find_by_unique_name(a)) { serviceAddBackend(service, s); } @@ -3760,7 +3760,7 @@ int create_new_server(CONFIG_CONTEXT* obj) const char* disk_space_threshold = config_get_value(obj->parameters, CN_DISK_SPACE_THRESHOLD); if (disk_space_threshold) { - if (!server_set_disk_space_threshold(server, disk_space_threshold)) + if (!server->server_set_disk_space_threshold(disk_space_threshold)) { MXS_ERROR("Invalid value for '%s' for server %s: %s", CN_DISK_SPACE_THRESHOLD, @@ -3795,7 +3795,7 @@ int create_new_monitor(CONFIG_CONTEXT* obj, std::set& monitored_ser for (auto& s : mxs::strtok(config_get_string(obj->parameters, CN_SERVERS), ",")) { fix_object_name(s); - SERVER* server = server_find_by_unique_name(s.c_str()); + auto server = Server::find_by_unique_name(s); if (!server) { diff --git a/server/core/config_runtime.cc b/server/core/config_runtime.cc index 89710a261..8e18cb05a 100644 --- a/server/core/config_runtime.cc +++ b/server/core/config_runtime.cc @@ -214,7 +214,7 @@ bool runtime_create_server(const char* name, std::lock_guard guard(crt_lock); bool rval = false; - if (server_find_by_unique_name(name) == NULL) + if (Server::find_by_unique_name(name) == NULL) { if (protocol == NULL) { @@ -2061,7 +2061,7 @@ bool runtime_alter_server_relationships_from_json(Server* server, const char* ty static bool object_relation_is_valid(const std::string& type, const std::string& value) { - return type == CN_SERVERS && server_find_by_unique_name(value.c_str()); + return type == CN_SERVERS && Server::find_by_unique_name(value); } static bool filter_relation_is_valid(const std::string& type, const std::string& value) diff --git a/server/core/internal/server.hh b/server/core/internal/server.hh index 03d427c2f..2830ca292 100644 --- a/server/core/internal/server.hh +++ b/server/core/internal/server.hh @@ -90,7 +90,7 @@ public: return m_settings.disk_space_limits; } - void set_disk_space_limits(const MxsDiskSpaceThreshold& new_limits) override + void set_disk_space_limits(const MxsDiskSpaceThreshold& new_limits) { std::lock_guard guard(m_settings.lock); m_settings.disk_space_limits = new_limits; @@ -226,6 +226,14 @@ public: */ static void dListServers(DCB*); + /** + * Convert all servers into JSON format + * + * @param host Hostname of this server + * @return JSON array of servers or NULL if an error occurred + */ + static json_t* server_list_to_json(const char* host); + static bool create_server_config(const Server* server, const char* filename); static json_t* server_json_attributes(const Server* server); @@ -273,6 +281,29 @@ public: std::string monitor_user() const; std::string monitor_password() const; + /** + * @brief Set the disk space threshold of the server + * + * @param server The server. + * @param disk_space_threshold The disk space threshold as specified in the config file. + * + * @return True, if the provided string is valid and the threshold could be set. + */ + bool server_set_disk_space_threshold(const char* disk_space_threshold); + + /** + * Print all servers + * + * Designed to be called within a debugger session in order + * to display all active servers within the gateway + */ + static void printAllServers(); + + /** + * Print details of an individual server + */ + void printServer(); + mutable std::mutex m_lock; DCB** persistent = nullptr;/**< List of unused persistent connections to the server */ diff --git a/server/core/modulecmd.cc b/server/core/modulecmd.cc index dffceb173..f0bc3db62 100644 --- a/server/core/modulecmd.cc +++ b/server/core/modulecmd.cc @@ -22,6 +22,7 @@ #include "internal/filter.hh" #include "internal/modules.hh" #include "internal/monitor.hh" +#include "internal/server.hh" /** Size of the error buffer */ #define MODULECMD_ERRBUF_SIZE 512 @@ -292,7 +293,7 @@ static bool process_argument(const MODULECMD* cmd, break; case MODULECMD_ARG_SERVER: - if ((arg->value.server = server_find_by_unique_name((char*)value))) + if ((arg->value.server = Server::find_by_unique_name((char*)value))) { if (MODULECMD_ALLOW_NAME_MISMATCH(type) || (arg->value.server->protocol() == cmd->domain)) diff --git a/server/core/monitor.cc b/server/core/monitor.cc index 2b8a81761..c8c7870be 100644 --- a/server/core/monitor.cc +++ b/server/core/monitor.cc @@ -151,7 +151,7 @@ MXS_MONITOR* monitor_create(const char* name, const char* module, MXS_CONFIG_PAR for (auto& s : mxs::strtok(config_get_string(params, CN_SERVERS), ",")) { fix_object_name(s); - monitor_add_server(mon, server_find_by_unique_name(s.c_str())); + monitor_add_server(mon, Server::find_by_unique_name(s)); } monitor_add_user(mon, config_get_string(params, CN_USER), config_get_string(params, CN_PASSWORD)); diff --git a/server/core/resource.cc b/server/core/resource.cc index 4080c9e04..42910b32b 100644 --- a/server/core/resource.cc +++ b/server/core/resource.cc @@ -123,7 +123,7 @@ bool Resource::matching_variable_path(const string& path, const string& target) if (path[0] == ':') { if ((path == ":service" && service_find(target.c_str())) - || (path == ":server" && server_find_by_unique_name(target.c_str())) + || (path == ":server" && Server::find_by_unique_name(target)) || (path == ":filter" && filter_find(target.c_str())) || (path == ":monitor" && monitor_find(target.c_str())) || (path == ":module" && get_module(target.c_str(), NULL)) @@ -543,7 +543,7 @@ HttpResponse cb_delete_filter(const HttpRequest& request) } HttpResponse cb_all_servers(const HttpRequest& request) { - return HttpResponse(MHD_HTTP_OK, server_list_to_json(request.host())); + return HttpResponse(MHD_HTTP_OK, Server::server_list_to_json(request.host())); } HttpResponse cb_get_server(const HttpRequest& request) @@ -766,7 +766,7 @@ HttpResponse cb_delete_user(const HttpRequest& request) HttpResponse cb_set_server(const HttpRequest& request) { - SERVER* server = server_find_by_unique_name(request.uri_part(1).c_str()); + SERVER* server = Server::find_by_unique_name(request.uri_part(1)); int opt = server_map_status(request.get_option(CN_STATE).c_str()); if (opt) @@ -790,7 +790,7 @@ HttpResponse cb_set_server(const HttpRequest& request) HttpResponse cb_clear_server(const HttpRequest& request) { - SERVER* server = server_find_by_unique_name(request.uri_part(1).c_str()); + SERVER* server = Server::find_by_unique_name(request.uri_part(1)); int opt = server_map_status(request.get_option(CN_STATE).c_str()); if (opt) diff --git a/server/core/server.cc b/server/core/server.cc index 886bff1dc..6b2428eaf 100644 --- a/server/core/server.cc +++ b/server/core/server.cc @@ -338,18 +338,17 @@ DCB* Server::get_persistent_dcb(const string& user, const string& ip, const stri return NULL; } -/** - * @brief Find a server with the specified name - * - * @param name Name of the server - * @return The server or NULL if not found - */ -SERVER* server_find_by_unique_name(const char* name) +SERVER* SERVER::find_by_unique_name(const string& name) +{ + return Server::find_by_unique_name(name); +} + +Server* Server::find_by_unique_name(const string& name) { Guard guard(this_unit.all_servers_lock); for (Server* server : this_unit.all_servers) { - if (server->is_active && strcmp(server->name(), name) == 0) + if (server->is_active && server->m_name == name) { return server; } @@ -357,20 +356,7 @@ SERVER* server_find_by_unique_name(const char* name) return nullptr; } -/** - * 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. - * - * @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 - */ -int server_find_by_unique_names(char** server_names, int size, SERVER*** output) +int SERVER::server_find_by_unique_names(char** server_names, int size, SERVER*** output) { mxb_assert(server_names && (size > 0)); @@ -383,7 +369,7 @@ int server_find_by_unique_names(char** server_names, int size, SERVER*** output) int found = 0; for (int i = 0; i < size; i++) { - results[i] = server_find_by_unique_name(server_names[i]); + results[i] = Server::find_by_unique_name(server_names[i]); found += (results[i]) ? 1 : 0; } @@ -398,37 +384,26 @@ int server_find_by_unique_names(char** server_names, int size, SERVER*** output) return found; } -/** - * Print details of an individual server - * - * @param server Server to print - */ -void printServer(const SERVER* server) +void Server::printServer() { - printf("Server %p\n", server); - printf("\tServer: %s\n", server->address); - printf("\tProtocol: %s\n", server->protocol().c_str()); - printf("\tPort: %d\n", server->port); - printf("\tTotal connections: %d\n", server->stats.n_connections); - printf("\tCurrent connections: %d\n", server->stats.n_current); - printf("\tPersistent connections: %d\n", server->stats.n_persistent); - printf("\tPersistent actual max: %d\n", server->persistmax); + printf("Server %p\n", this); + printf("\tServer: %s\n", address); + printf("\tProtocol: %s\n", m_settings.protocol.c_str()); + printf("\tPort: %d\n", port); + printf("\tTotal connections: %d\n", stats.n_connections); + printf("\tCurrent connections: %d\n", stats.n_current); + printf("\tPersistent connections: %d\n", stats.n_persistent); + printf("\tPersistent actual max: %d\n", persistmax); } -/** - * Print all servers - * - * Designed to be called within a debugger session in order - * to display all active servers within the gateway - */ -void printAllServers() +void Server::printAllServers() { Guard guard(this_unit.all_servers_lock); for (Server* server : this_unit.all_servers) { - if (server->is_active) + if (server->server_is_active()) { - printServer(server); + server->printServer(); } } } @@ -447,7 +422,7 @@ void Server::dprintAllServers(DCB* dcb) void Server::dprintAllServersJson(DCB* dcb) { - json_t* all_servers_json = server_list_to_json(""); + json_t* all_servers_json = Server::server_list_to_json(""); char* dump = json_dumps(all_servers_json, JSON_INDENT(4)); dcb_printf(dcb, "%s", dump); MXS_FREE(dump); @@ -1136,17 +1111,17 @@ bool mxs::server_clear_status(SERVER* srv, int bit, string* errmsg_out) return written; } -bool server_is_mxs_service(const SERVER* server) +bool SERVER::is_mxs_service() { bool rval = false; /** Do a coarse check for local server pointing to a MaxScale service */ - if (strcmp(server->address, "127.0.0.1") == 0 - || strcmp(server->address, "::1") == 0 - || strcmp(server->address, "localhost") == 0 - || strcmp(server->address, "localhost.localdomain") == 0) + if (strcmp(address, "127.0.0.1") == 0 + || strcmp(address, "::1") == 0 + || strcmp(address, "localhost") == 0 + || strcmp(address, "localhost.localdomain") == 0) { - if (service_port_is_used(server->port)) + if (service_port_is_used(port)) { rval = true; } @@ -1267,7 +1242,7 @@ json_t* server_to_json(const Server* server, const char* host) return mxs_json_resource(host, self.c_str(), server_to_json_data(server, host)); } -json_t* server_list_to_json(const char* host) +json_t* Server::server_list_to_json(const char* host) { json_t* data = json_array(); @@ -1283,13 +1258,13 @@ json_t* server_list_to_json(const char* host) return mxs_json_resource(host, MXS_JSON_API_SERVERS, data); } -bool server_set_disk_space_threshold(SERVER* server, const char* disk_space_threshold) +bool Server::server_set_disk_space_threshold(const char* disk_space_threshold) { MxsDiskSpaceThreshold dst; bool rv = config_parse_disk_space_threshold(&dst, disk_space_threshold); if (rv) { - server->set_disk_space_limits(dst); + set_disk_space_limits(dst); } return rv; } @@ -1352,19 +1327,6 @@ void Server::response_time_add(double ave, int num_samples) m_response_time.add(ave, num_samples); } -Server* Server::find_by_unique_name(const string& name) -{ - Guard guard(this_unit.all_servers_lock); - for (Server* server : this_unit.all_servers) - { - if (server->is_active && server->m_name == name) - { - return server; - } - } - return nullptr; -} - bool Server::is_custom_parameter(const string& name) const { for (int i = 0; config_server_params[i].name; i++) diff --git a/server/core/test/test_server.cc b/server/core/test/test_server.cc index bd02c85c0..a13dcf607 100644 --- a/server/core/test/test_server.cc +++ b/server/core/test/test_server.cc @@ -73,9 +73,9 @@ static int test1() std::string buf = server->get_custom_parameter("name"); mxb_assert_message(buf == "value", "Parameter should be returned correctly"); fprintf(stderr, "\t..done\nTesting Unique Name for Server."); - mxb_assert_message(NULL == server_find_by_unique_name("non-existent"), + mxb_assert_message(NULL == Server::find_by_unique_name("non-existent"), "Should not find non-existent unique name."); - mxb_assert_message(server == server_find_by_unique_name("uniquename"), "Should find by unique name."); + mxb_assert_message(server == Server::find_by_unique_name("uniquename"), "Should find by unique name."); fprintf(stderr, "\t..done\nTesting Status Setting for Server."); status = server_status(server); mxb_assert_message(status == "Running", "Status of Server should be Running by default."); @@ -87,8 +87,8 @@ static int test1() mxb_assert_message(status == "Running", "Status of Server should be Running after master status cleared."); fprintf(stderr, "\t..done\nRun Prints for Server and all Servers."); - printServer(server); - printAllServers(); + server->printServer(); + Server::printAllServers(); fprintf(stderr, "\t..done\nFreeing Server."); server_free((Server*)server); fprintf(stderr, "\t..done\n"); diff --git a/server/modules/authenticator/MySQLAuth/dbusers.cc b/server/modules/authenticator/MySQLAuth/dbusers.cc index af34854c7..02e9ad21b 100644 --- a/server/modules/authenticator/MySQLAuth/dbusers.cc +++ b/server/modules/authenticator/MySQLAuth/dbusers.cc @@ -950,7 +950,7 @@ bool check_service_permissions(SERVICE* service) for (SERVER_REF* server = service->dbref; server; server = server->next) { - if (server_is_mxs_service(server->server) + if (server->server->is_mxs_service() || check_server_permissions(service, server->server, user, dpasswd)) { rval = true; @@ -1221,7 +1221,7 @@ static int get_users(Listener* listener, bool skip_local) for (server = service->dbref; !maxscale_is_shutting_down() && server; server = server->next) { if (!server_ref_is_active(server) || !server->server->server_is_active() - || (skip_local && server_is_mxs_service(server->server)) + || (skip_local && server->server->is_mxs_service()) || !server->server->is_running()) { continue; diff --git a/server/modules/filter/namedserverfilter/namedserverfilter.cc b/server/modules/filter/namedserverfilter/namedserverfilter.cc index 9e54b8bb8..ae06bcaa7 100644 --- a/server/modules/filter/namedserverfilter/namedserverfilter.cc +++ b/server/modules/filter/namedserverfilter/namedserverfilter.cc @@ -490,7 +490,7 @@ int RegexToServers::add_servers(const std::string& server_names, bool legacy_mod { /* The string contains a server list, all must be valid servers */ SERVER** servers; - int found = server_find_by_unique_names(names_arr, n_names, &servers); + int found = SERVER::server_find_by_unique_names(names_arr, n_names, &servers); if (found != n_names) { @@ -522,7 +522,7 @@ int RegexToServers::add_servers(const std::string& server_names, bool legacy_mod else if (n_names == 1) { /* The string is either a server name or a special reserved id */ - if (server_find_by_unique_name(names_arr[0])) + if (SERVER::find_by_unique_name(names_arr[0])) { m_targets.push_back(names_arr[0]); } diff --git a/server/modules/routing/debugcli/debugcmd.cc b/server/modules/routing/debugcli/debugcmd.cc index 264f157b0..96c51c9ae 100644 --- a/server/modules/routing/debugcli/debugcmd.cc +++ b/server/modules/routing/debugcli/debugcmd.cc @@ -1235,7 +1235,7 @@ static void createServer(DCB* dcb, { pthread_mutex_lock(&server_mod_lock); - if (server_find_by_unique_name(name) == NULL) + if (Server::find_by_unique_name(name) == NULL) { if (runtime_create_server(name, address, port, protocol, authenticator)) { diff --git a/server/modules/routing/maxinfo/maxinfo_exec.cc b/server/modules/routing/maxinfo/maxinfo_exec.cc index 2f719e86f..d965f4b01 100644 --- a/server/modules/routing/maxinfo/maxinfo_exec.cc +++ b/server/modules/routing/maxinfo/maxinfo_exec.cc @@ -315,7 +315,7 @@ static void exec_flush(DCB* dcb, MAXINFO_TREE* tree) */ void exec_set_server(DCB* dcb, MAXINFO_TREE* tree) { - SERVER* server = server_find_by_unique_name(tree->value); + auto server = Server::find_by_unique_name(tree->value); char errmsg[120]; if (server) @@ -402,7 +402,7 @@ static void exec_set(DCB* dcb, MAXINFO_TREE* tree) */ void exec_clear_server(DCB* dcb, MAXINFO_TREE* tree) { - SERVER* server = server_find_by_unique_name(tree->value); + auto server = Server::find_by_unique_name(tree->value); char errmsg[120]; if (server)