Continue external command cleanup

Simplify serverlist creation code.

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 status Server status bitmask. At least one bit must match with a server for it to be included
-     * @param len Length of @c dest
+     * 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,
+    std::string gen_serverlist(int status, CredentialsApproach approach = CredentialsApproach::EXCLUDE);
-                           CredentialsApproach approach = CredentialsApproach::EXCLUDE);
 
     FILE* open_data_file(Monitor* monitor, char* path);
     int   get_data_file_path(char* path) const;

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");
+                for (size_t pos = output.find("\n"); pos != std::string::npos; 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
     }
 }
 
-/**
+void ExternalCmd::match_substitute(const string& keyword, const std::function<string(void)>& generator)
- * 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)
 {
-    char* rval = NULL;
+    if (m_orig_command.find(keyword) != string::npos)
-    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<std::string(void)> generator)
-{
-    if (externcmd_matches(keyword))
     {
         substitute_arg(keyword, generator());
     }

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<std::string(void)> generator);
+    void match_substitute(const std::string& keyword, const std::function<std::string(void)>& 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);
 };
 

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 = "";
+    string rval;
-    // Some extra space for port and separator
+    rval.reserve(100 * m_servers.size());
-    char arr[SERVER::MAX_MONUSER_LEN + SERVER::MAX_MONPW_LEN + SERVER::MAX_ADDRESS_LEN + 64];
-    dest[0] = '\0';
 
-    for (auto iter = m_servers.begin(); iter != m_servers.end() && len; ++iter)
+    string separator;
+    for (auto mon_server : m_servers)
     {
-        Server* server = static_cast<Server*>((*iter)->server);
+        auto server = static_cast<Server*>(mon_server->server);
         if (status == 0 || server->status & status)
         {
             if (approach == CredentialsApproach::EXCLUDE)
             {
-                snprintf(arr,
+                rval += separator + mxb::string_printf("[%s]:%i", server->address, server->port);
-                         sizeof(arr),
-                         "%s[%s]:%d",
-                         separator,
-                         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,
+                rval += separator + mxb::string_printf("%s:%s@[%s]:%d", user.c_str(), password.c_str(),
-                         sizeof(arr),
+                                                       server->address, server->port);
-                         "%s%s:%s@[%s]:%d",
-                         separator,
-                         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();
+    m_scriptcmd->match_substitute("$INITIATOR", gen_initiator);
-    cmd->reset_substituted();
+    m_scriptcmd->match_substitute("$PARENT", gen_parent);
-    cmd->match_substitute("$INITIATOR", gen_initiator);
-    cmd->match_substitute("$PARENT", gen_parent);
 
-    cmd->match_substitute("$CHILDREN", [this, ptr] {
+    m_scriptcmd->match_substitute("$CHILDREN", [this, ptr] {
                                       return child_nodes(ptr);
                                   });
 
-    cmd->match_substitute("$EVENT", [ptr] {
+    m_scriptcmd->match_substitute("$EVENT", [ptr] {
                                       return ptr->get_event_name();
                                   });
 
-    char nodelist[PATH_MAX + MON_ARG_MAX + 1] = {'\0'};
+    m_scriptcmd->match_substitute("$CREDENTIALS", [this] {
+                                        // Provides credentials for all servers.
+                                      return gen_serverlist(0, CredentialsApproach::INCLUDE);
+                                  });
 
-    if (cmd->externcmd_matches("$CREDENTIALS"))
+    m_scriptcmd->match_substitute("$NODELIST", [this] {
+                              return gen_serverlist(SERVER_RUNNING);
+                          });
+
+    m_scriptcmd->match_substitute("$LIST", [this] {
+                              return gen_serverlist(0);
+                          });
+
+    m_scriptcmd->match_substitute("$MASTERLIST", [this] {
+                              return gen_serverlist(SERVER_MASTER);
+                          });
+
+    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.
+        MXS_NOTICE("Executed monitor script on event '%s'. Script was: '%s'",
-        append_node_names(nodelist, sizeof(nodelist), 0, CredentialsApproach::INCLUDE);
+                   ptr->get_event_name(), m_scriptcmd->substituted());
-        cmd->substitute_arg("$CREDENTIALS", nodelist);
     }
-
+    else if (rv == -1)
-    if (cmd->externcmd_matches("$NODELIST"))
-    {
-        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());
+                  ptr->get_event_name(), m_scriptcmd->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());
+                  rv, 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());
-    }
-
     return rv;
 }