From 5e03ff35eb805b829531cff82cfb97420fa4340b Mon Sep 17 00:00:00 2001 From: Esa Korhonen Date: Wed, 29 May 2019 16:02:53 +0300 Subject: [PATCH] Continue external command cleanup Simplify serverlist creation code. --- include/maxscale/monitor.hh | 10 +-- server/core/externcmd.cc | 54 +---------- server/core/internal/externcmd.hh | 29 +++--- server/core/monitor.cc | 144 +++++++++++------------------- 4 files changed, 74 insertions(+), 163 deletions(-) diff --git a/include/maxscale/monitor.hh b/include/maxscale/monitor.hh index b5e02a0f9..f31abcebc 100644 --- a/include/maxscale/monitor.hh +++ b/include/maxscale/monitor.hh @@ -554,14 +554,14 @@ private: }; /** - * Create a list of running servers + * Create a list of server addresses and ports. * - * @param dest Destination where the string is appended, must be null terminated - * @param len Length of @c dest + * @param status Server status bitmask. At least one bit must match with a server for it to be included + * in the resulting list. 0 allows all servers regardless of status. * @param approach Whether credentials should be included or not. + * @return Comma-separated list */ - void append_node_names(char* dest, int len, int status, - CredentialsApproach approach = CredentialsApproach::EXCLUDE); + std::string gen_serverlist(int status, CredentialsApproach approach = CredentialsApproach::EXCLUDE); FILE* open_data_file(Monitor* monitor, char* path); int get_data_file_path(char* path) const; diff --git a/server/core/externcmd.cc b/server/core/externcmd.cc index 0d969d799..3ac1facd0 100644 --- a/server/core/externcmd.cc +++ b/server/core/externcmd.cc @@ -247,7 +247,7 @@ int ExternalCmd::externcmd_execute() { MXS_INFO("Executing command '%s' in process %d", cmdname, pid); - std::string output; + string output; bool first_warning = true; bool again = true; uint64_t t = 0; @@ -319,8 +319,7 @@ int ExternalCmd::externcmd_execute() // Read all available output output.append(buf, n); - for (size_t pos = output.find("\n"); - pos != std::string::npos; pos = output.find("\n")) + for (size_t pos = output.find("\n"); pos != std::string::npos; pos = output.find("\n")) { if (pos == 0) { @@ -372,54 +371,9 @@ void ExternalCmd::substitute_arg(const std::string& match, const std::string& re } } -/** - * Get the name of the command being executed. - * - * This copies the command being executed into a new string. - * @param str Command string, optionally with arguments - * @return Command part of the string if arguments were defined - */ -static char* get_command(const char* str) +void ExternalCmd::match_substitute(const string& keyword, const std::function& generator) { - char* rval = NULL; - const char* start = str; - - while (*start && isspace(*start)) - { - start++; - } - - const char* end = start; - - while (*end && !isspace(*end)) - { - end++; - } - - size_t len = end - start; - - if (len > 0) - { - rval = (char*)MXS_MALLOC(len + 1); - - if (rval) - { - memcpy(rval, start, len); - rval[len] = '\0'; - } - } - - return rval; -} - -bool ExternalCmd::externcmd_matches(const string& match) -{ - return m_orig_command.find(match) != string::npos; -} - -void ExternalCmd::match_substitute(const std::string& keyword, std::function generator) -{ - if (externcmd_matches(keyword)) + if (m_orig_command.find(keyword) != string::npos) { substitute_arg(keyword, generator()); } diff --git a/server/core/internal/externcmd.hh b/server/core/internal/externcmd.hh index 9f728f5c9..8286595fa 100644 --- a/server/core/internal/externcmd.hh +++ b/server/core/internal/externcmd.hh @@ -39,30 +39,17 @@ public: */ int externcmd_execute(); - /** - * Substitute all occurrences of @c match with @c replace in the arguments. - * - * @param match Match string - * @param replace Replacement string - */ - void substitute_arg(const std::string& match, const std::string& replace); - - /** - * Simple matching of string and command - * - * @param match String to search for - * @return True if the string matched - */ - bool externcmd_matches(const std::string& match); - /** * If keyword is found in command script, replace keyword with output of generator function. * * @param keyword Keyword to replace * @param generator Function which generates the replacement string. Only ran if keyword was found. */ - void match_substitute(const std::string& keyword, std::function generator); + void match_substitute(const std::string& keyword, const std::function& generator); + /** + * Reset substituted command to the unaltered command. Should be ran before a substitution pass begins. + */ void reset_substituted(); const char* substituted() const; @@ -77,5 +64,13 @@ private: ExternalCmd(const std::string& script, int timeout); int tokenize_args(char* dest[], int dest_size); + + /** + * Substitute all occurrences of @c match with @c replace in the arguments. + * + * @param match Match string + * @param replace Replacement string + */ + void substitute_arg(const std::string& match, const std::string& replace); }; diff --git a/server/core/monitor.cc b/server/core/monitor.cc index 7ea9871f3..a45fb1c9c 100644 --- a/server/core/monitor.cc +++ b/server/core/monitor.cc @@ -930,26 +930,20 @@ const char* MonitorServer::get_event_name() return Monitor::get_event_name((mxs_monitor_event_t) server->last_event); } -void Monitor::append_node_names(char* dest, int len, int status, CredentialsApproach approach) +string Monitor::gen_serverlist(int status, CredentialsApproach approach) { - const char* separator = ""; - // Some extra space for port and separator - char arr[SERVER::MAX_MONUSER_LEN + SERVER::MAX_MONPW_LEN + SERVER::MAX_ADDRESS_LEN + 64]; - dest[0] = '\0'; + string rval; + rval.reserve(100 * m_servers.size()); - for (auto iter = m_servers.begin(); iter != m_servers.end() && len; ++iter) + string separator; + for (auto mon_server : m_servers) { - Server* server = static_cast((*iter)->server); + auto server = static_cast(mon_server->server); if (status == 0 || server->status & status) { if (approach == CredentialsApproach::EXCLUDE) { - snprintf(arr, - sizeof(arr), - "%s[%s]:%d", - separator, - server->address, - server->port); + rval += separator + mxb::string_printf("[%s]:%i", server->address, server->port); } else { @@ -962,26 +956,13 @@ void Monitor::append_node_names(char* dest, int len, int status, CredentialsAppr password = server->monitor_password(); } - snprintf(arr, - sizeof(arr), - "%s%s:%s@[%s]:%d", - separator, - user.c_str(), - password.c_str(), - server->address, - server->port); + rval += separator + mxb::string_printf("%s:%s@[%s]:%d", user.c_str(), password.c_str(), + server->address, server->port); } - separator = ","; - int arrlen = strlen(arr); - - if (arrlen < len) - { - strcat(dest, arr); - len -= arrlen; - } } } + return rval; } /** @@ -1072,6 +1053,8 @@ std::string Monitor::child_nodes(MonitorServer* parent) int Monitor::launch_command(MonitorServer* ptr) { + m_scriptcmd->reset_substituted(); + // A generator function is ran only if the matching substitution keyword is found. auto gen_initiator = [ptr] { @@ -1088,81 +1071,60 @@ int Monitor::launch_command(MonitorServer* ptr) return ss; }; - auto cmd = m_scriptcmd.get(); - cmd->reset_substituted(); - cmd->match_substitute("$INITIATOR", gen_initiator); - cmd->match_substitute("$PARENT", gen_parent); + m_scriptcmd->match_substitute("$INITIATOR", gen_initiator); + m_scriptcmd->match_substitute("$PARENT", gen_parent); - cmd->match_substitute("$CHILDREN", [this, ptr] { - return child_nodes(ptr); + m_scriptcmd->match_substitute("$CHILDREN", [this, ptr] { + return child_nodes(ptr); + }); + + m_scriptcmd->match_substitute("$EVENT", [ptr] { + return ptr->get_event_name(); + }); + + m_scriptcmd->match_substitute("$CREDENTIALS", [this] { + // Provides credentials for all servers. + return gen_serverlist(0, CredentialsApproach::INCLUDE); + }); + + m_scriptcmd->match_substitute("$NODELIST", [this] { + return gen_serverlist(SERVER_RUNNING); }); - cmd->match_substitute("$EVENT", [ptr] { - return ptr->get_event_name(); + m_scriptcmd->match_substitute("$LIST", [this] { + return gen_serverlist(0); }); - char nodelist[PATH_MAX + MON_ARG_MAX + 1] = {'\0'}; + m_scriptcmd->match_substitute("$MASTERLIST", [this] { + return gen_serverlist(SERVER_MASTER); + }); - if (cmd->externcmd_matches("$CREDENTIALS")) + m_scriptcmd->match_substitute("$SLAVELIST", [this] { + return gen_serverlist(SERVER_SLAVE); + }); + + m_scriptcmd->match_substitute("$SYNCEDLIST", [this] { + return gen_serverlist(SERVER_JOINED); + }); + + int rv = m_scriptcmd->externcmd_execute(); + if (rv == 0) { - // We provide the credentials for _all_ servers. - append_node_names(nodelist, sizeof(nodelist), 0, CredentialsApproach::INCLUDE); - cmd->substitute_arg("$CREDENTIALS", nodelist); + MXS_NOTICE("Executed monitor script on event '%s'. Script was: '%s'", + ptr->get_event_name(), m_scriptcmd->substituted()); } - - if (cmd->externcmd_matches("$NODELIST")) + else if (rv == -1) { - append_node_names(nodelist, sizeof(nodelist), SERVER_RUNNING); - cmd->substitute_arg("$NODELIST", nodelist); - } - - if (cmd->externcmd_matches("$LIST")) - { - append_node_names(nodelist, sizeof(nodelist), 0); - cmd->substitute_arg("$LIST", nodelist); - } - - if (cmd->externcmd_matches("$MASTERLIST")) - { - append_node_names(nodelist, sizeof(nodelist), SERVER_MASTER); - cmd->substitute_arg("$MASTERLIST", nodelist); - } - - if (cmd->externcmd_matches("$SLAVELIST")) - { - append_node_names(nodelist, sizeof(nodelist), SERVER_SLAVE); - cmd->substitute_arg("$SLAVELIST", nodelist); - } - - if (cmd->externcmd_matches("$SYNCEDLIST")) - { - append_node_names(nodelist, sizeof(nodelist), SERVER_JOINED); - cmd->substitute_arg("$SYNCEDLIST", nodelist); - } - - int rv = cmd->externcmd_execute(); - - if (rv) - { - if (rv == -1) - { - // Internal error - MXS_ERROR("Failed to execute script on server state change event '%s'. Script was: '%s'", - ptr->get_event_name(), cmd->substituted()); - } - else - { - // Script returned a non-zero value - MXS_ERROR("Script returned %d on event '%s'. Script was: '%s'", - rv, ptr->get_event_name(), cmd->substituted()); - } + // Internal error + MXS_ERROR("Failed to execute script on server state change event '%s'. Script was: '%s'", + ptr->get_event_name(), m_scriptcmd->substituted()); } else { - MXS_NOTICE("Executed monitor script on event '%s'. Script was: '%s'", - ptr->get_event_name(), cmd->substituted()); + // Script returned a non-zero value + MXS_ERROR("Script returned %d on event '%s'. Script was: '%s'", + rv, ptr->get_event_name(), m_scriptcmd->substituted()); } - return rv; }