[trafficserver] 01/02: traffic_manager: Cleanup handling of proxy args.

[zw...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

zwoop pushed a commit to branch 8.0.x
in repository https://gitbox.apache.org/repos/asf/trafficserver.git

commit f447876d1ea5236f89ed18c10e546723883aab50
Author: Alan M. Carroll <am...@apache.org>
AuthorDate: Fri Jun 29 11:24:08 2018 -0500

    traffic_manager: Cleanup handling of proxy args.
    
    (cherry picked from commit e10da228172ac548ec7d497850c6a5e9d7b43114)
---
 mgmt/LocalManager.cc                   | 56 +++++++++++++++++-----------------
 mgmt/LocalManager.h                    |  4 ++-
 mgmt/api/CoreAPI.cc                    |  2 +-
 src/traffic_manager/traffic_manager.cc | 22 ++++---------
 4 files changed, 38 insertions(+), 46 deletions(-)

diff --git a/mgmt/LocalManager.cc b/mgmt/LocalManager.cc
index 907650c..b31d00f 100644
--- a/mgmt/LocalManager.cc
+++ b/mgmt/LocalManager.cc
@@ -32,12 +32,17 @@
 #include "ts/ink_cap.h"
 #include "FileManager.h"
 #include <string_view>
+#include <algorithm>
+#include <ts/TextView.h>
+#include <ts/BufferWriter.h>
+#include <ts/bwf_std_format.h>
 
 #if TS_USE_POSIX_CAP
 #include <sys/capability.h>
 #endif
 
-#define MGMT_OPT "-M"
+using namespace std::literals;
+static const std::string_view MGMT_OPT{"-M"};
 
 void
 LocalManager::mgmtCleanup()
@@ -253,7 +258,6 @@ LocalManager::~LocalManager()
   ats_free(absolute_proxy_binary);
   ats_free(proxy_name);
   ats_free(proxy_binary);
-  ats_free(proxy_options);
   ats_free(env_prep);
 }
 
@@ -883,52 +887,48 @@ LocalManager::startProxy(const char *onetime_options)
   } else {
     int i = 0;
     char *options[32], *last, *tok;
-    bool open_ports_p = false;
-    char real_proxy_options[OPTIONS_SIZE];
+    char options_buffer[OPTIONS_SIZE];
+    ts::FixedBufferWriter w{options_buffer, OPTIONS_SIZE};
 
-    ink_strlcpy(real_proxy_options, proxy_options, OPTIONS_SIZE);
-    if (onetime_options && *onetime_options) {
-      ink_strlcat(real_proxy_options, " ", OPTIONS_SIZE);
-      ink_strlcat(real_proxy_options, onetime_options, OPTIONS_SIZE);
-    }
+    w.clip(1);
+    w.print("{}{}", ts::bwf::OptionalAffix(proxy_options), ts::bwf::OptionalAffix(onetime_options));
 
     // Make sure we're starting the proxy in mgmt mode
-    if (strstr(real_proxy_options, MGMT_OPT) == nullptr) {
-      ink_strlcat(real_proxy_options, " ", OPTIONS_SIZE);
-      ink_strlcat(real_proxy_options, MGMT_OPT, OPTIONS_SIZE);
+    if (w.view().find(MGMT_OPT) == std::string_view::npos) {
+      w.write(MGMT_OPT);
+      w.write(' ');
     }
 
-    // Check if we need to pass down port/fd information to
-    // traffic_server by seeing if there are any open ports.
-    for (int i = 0, limit = m_proxy_ports.size(); !open_ports_p && i < limit; ++i) {
-      if (ts::NO_FD != m_proxy_ports[i].m_fd) {
-        open_ports_p = true;
-      }
-    }
-
-    if (open_ports_p) {
+    // Pass down port/fd information to traffic_server if there are any open ports.
+    if (std::any_of(m_proxy_ports.begin(), m_proxy_ports.end(), [](HttpProxyPort &p) { return ts::NO_FD != p.m_fd; })) {
       char portbuf[128];
       bool need_comma_p = false;
 
-      ink_strlcat(real_proxy_options, " --httpport ", OPTIONS_SIZE);
+      w.write("--httpport "sv);
       for (auto &p : m_proxy_ports) {
         if (ts::NO_FD != p.m_fd) {
           if (need_comma_p) {
-            ink_strlcat(real_proxy_options, ",", OPTIONS_SIZE);
+            w.write(',');
           }
           need_comma_p = true;
           p.print(portbuf, sizeof(portbuf));
-          ink_strlcat(real_proxy_options, (const char *)portbuf, OPTIONS_SIZE);
+          w.write(portbuf);
         }
       }
     }
 
-    Debug("lm", "[LocalManager::startProxy] Launching %s with options '%s'", absolute_proxy_binary, real_proxy_options);
+    w.extend(1);
+    w.write('\0'); // null terminate.
+
+    Debug("lm", "[LocalManager::startProxy] Launching %s '%s'", absolute_proxy_binary, w.data());
 
+    // Unfortunately the normally obnoxious null writing of strtok is in this case a required
+    // side effect and other alternatives are noticeably more clunky.
     ink_zero(options);
-    options[0]   = absolute_proxy_binary;
-    i            = 1;
-    tok          = strtok_r(real_proxy_options, " ", &last);
+    options[0] = absolute_proxy_binary;
+    i          = 1;
+    tok        = strtok_r(options_buffer, " ", &last);
+    Debug("lm", "opt %d = '%s'", i, tok);
     options[i++] = tok;
     while (i < 32 && (tok = strtok_r(nullptr, " ", &last))) {
       Debug("lm", "opt %d = '%s'", i, tok);
diff --git a/mgmt/LocalManager.h b/mgmt/LocalManager.h
index 4cc5257..4ff3591 100644
--- a/mgmt/LocalManager.h
+++ b/mgmt/LocalManager.h
@@ -33,6 +33,8 @@
 
 #pragma once
 
+#include <string>
+
 #include "Alarms.h"
 #include "BaseManager.h"
 #include <records/I_RecHttp.h>
@@ -119,7 +121,7 @@ public:
   char *absolute_proxy_binary;
   char *proxy_name;
   char *proxy_binary;
-  char *proxy_options = nullptr; // These options should persist across proxy reboots
+  std::string proxy_options; // These options should persist across proxy reboots
   char *env_prep;
 
   int process_server_sockfd = ts::NO_FD;
diff --git a/mgmt/api/CoreAPI.cc b/mgmt/api/CoreAPI.cc
index 1d8eb77..69a4201 100644
--- a/mgmt/api/CoreAPI.cc
+++ b/mgmt/api/CoreAPI.cc
@@ -183,7 +183,7 @@ ProxyStateSet(TSProxyStateT state, TSCacheClearT clear)
       ink_strlcat(tsArgs, " -k", sizeof(tsArgs));
     }
 
-    mgmt_log("[ProxyStateSet] Traffic Server Args: '%s %s'\n", lmgmt->proxy_options ? lmgmt->proxy_options : "", tsArgs);
+    mgmt_log("[ProxyStateSet] Traffic Server Args: '%s %s'\n", lmgmt->proxy_options.c_str(), tsArgs);
 
     lmgmt->run_proxy = true;
     lmgmt->listenForProxy();
diff --git a/src/traffic_manager/traffic_manager.cc b/src/traffic_manager/traffic_manager.cc
index e1439c0..2fae482c 100644
--- a/src/traffic_manager/traffic_manager.cc
+++ b/src/traffic_manager/traffic_manager.cc
@@ -56,6 +56,7 @@
 #endif
 #include <grp.h>
 #include <atomic>
+#include <ts/bwf_std_format.h>
 
 #define FD_THROTTLE_HEADROOM (128 + 64) // TODO: consolidate with THROTTLE_FD_HEADROOM
 #define DIAGS_LOG_FILENAME "manager.log"
@@ -64,6 +65,8 @@
 #error "Need lock free std::atomic<int>"
 #endif
 
+using namespace std::literals;
+
 // These globals are still referenced directly by management API.
 LocalManager *lmgmt = nullptr;
 FileManager *configFiles;
@@ -643,22 +646,9 @@ main(int argc, const char **argv)
   // so we append the outputlog location to the persistent proxy options
   //
   // TS needs them to be able to create BaseLogFiles for each value
-  TextBuffer args(1024);
-
-  if (*bind_stdout) {
-    const char *space = args.empty() ? "" : " ";
-    args.format("%s%s %s", space, "--" TM_OPT_BIND_STDOUT, bind_stdout);
-  }
-
-  if (*bind_stderr) {
-    const char *space = args.empty() ? "" : " ";
-    args.format("%s%s %s", space, "--" TM_OPT_BIND_STDERR, bind_stderr);
-  }
-  if (tsArgs) {
-    args.format("%s", tsArgs);
-  }
-
-  lmgmt->proxy_options = args.release();
+  ts::bwprint(lmgmt->proxy_options, "{}{}{}", ts::bwf::OptionalAffix(tsArgs),
+              ts::bwf::OptionalAffix(bind_stdout, " "sv, "--bind_stdout "sv),
+              ts::bwf::OptionalAffix(bind_stderr, " "sv, "--bind_stderr "sv));
 
   if (proxy_port) {
     HttpProxyPort::loadValue(lmgmt->m_proxy_ports, proxy_port);