apache · jrushford · Nov 6, 2019 · Jul 12, 2019
diff --git a/doc/admin-guide/plugins/slice.en.rst b/doc/admin-guide/plugins/slice.en.rst
@@ -69,17 +69,25 @@ The slice plugin supports the following options::
         Suffix k,m,g supported
         Limited to 32k and 32m inclusive.
 
-    --test-blockbytes=<bytes> (optional)
+    --blockbytes-test=<bytes> (optional)
         Suffix k,m,g supported
         -t <bytes> for short.
         Limited to any positive number.
         Ignored if --blockbytes provided.
 
+    --remap-host=<loopback hostname> (optional)
+        Uses effective url with given hostname for remapping.
+        Requires setting up an intermediate loopback remap rule.
+        -r for short
+
     --pace-errorlog=<seconds> (optional)
         Limit stitching error logs to every 'n' second(s)
+        -p for short
 
     --disable-errorlog (optional)
         Disable writing block stitch errors to the error log.
+        -d for short
+
 
 Examples::
 
@@ -95,10 +103,10 @@ Byte suffix examples::
     slice.so -b 512k
     slice.so --blockbytes=32m
 
-For testing and extreme purposes the parameter ``test-blockbytes`` may
+For testing and extreme purposes the parameter ``blockbytes-test`` may
 be used instead which is unchecked::
 
-    slice.so --test-blockbytes=1G
+    slice.so --blockbytes-test=1G
     slice.so -t 13
 
 Because the slice plugin is susceptible to errors during block stitching
@@ -128,9 +136,11 @@ Under normal logging these slice block errors tend to show up as::
     crc value ERR_READ_ERROR
 
 By default more detailed stitching errors are written to ``diags.log``.
-An example is as follows::
+Examples are as follows::
 
-[Apr 19 20:26:13.639] [ET_NET 17] ERROR: [slice] 1555705573.639 reason="Non 206 internal block response" uri="http://localhost:18080/%7Ep.tex/%7Es.50M/%7Eui.20000/" uas="curl/7.29.0" req_range="bytes=1000000-" norm_range="bytes 1000000-52428799/52428800" etag_exp="%221603934496%22" lm_exp="Fri, 19 Apr 2019 18:53:20 GMT" blk_range="21000000-21999999" status_got="400" cr_got="" etag_got="" lm_got="" cc="no-store" via=""
+ERROR: [slice.cc: 288] logSliceError(): 1555705573.639 reason="Non 206 internal block response" uri="http://ats_ep/someasset.mp4" uas="curl" req_range="bytes=1000000-" norm_range="bytes 1000000-52428799/52428800" etag_exp="%221603934496%22" lm_exp="Fri, 19 Apr 2019 18:53:20 GMT" blk_range="21000000-21999999" status_got="206" cr_got="" etag_got="%221603934496%22" lm_got="" cc="no-store" via=""
+
+ERROR: [server.cc: 288] logSliceError(): 1572370000.219 reason="Mismatch block Etag" uri="http://ats_ep/someasset.mp4" uas="curl" req_range="bytes=1092779033-1096299354" norm_range="bytes 1092779033-1096299354/2147483648" etag_exp="%223719843648%22" lm_exp="Tue, 29 Oct 2019 14:40:00 GMT" blk_range="1095000000-1095999999" status_got="206" cr_got="bytes 1095000000-1095999999/2147483648" etag_got="%223719853648%22" lm_got="Tue, 29 Oct 2019 17:26:40 GMT" cc="max-age=10000" via=""
 
 Whether or how often these detailed log entries are written are
 configurable plugin options.
@@ -204,14 +214,43 @@ by cache_range_requests.  The parent will trim those requests to
 account for the asset Content-Length so only the appropriate number
 of bytes are actually transferred and cached.
 
-Current Limitations
+Effective URL remap
 ===================
 
-By restoring the pristine Url the plugin as it works today reuses the
-same remap rule for each slice block.  This is wasteful in that it reruns
+By default the plugin restores the Pristine Url which reuses the same
+remap rule for each slice block.  This is wasteful in that it reruns
 the previous remap rules, and those remap rules must be smart enough to
-check for the existence of any headers they may have created the
-first time they have were visited.
+check for the existence of any headers they may have created the first
+time they have were visited.
+
+To get around this the '--remap-host=<host>' or '-r <host>' option may
+be used.  This requires an intermediate loopback remap to be defined which
+handles each slice block request.
+
+This works well with any remap rules that use the url_sig or uri_signing
+plugins.  As the client remap rule is not caching any plugins that
+manipulate the cache key would need to go into the loopback to parent
+remap rule.
+
+NOTE: Requests NOT handled by the slice plugin (ie: HEAD requests) are
+handled as with a typical remap rule.  GET requests intercepted by the
+slice plugin are virtually reissued into ATS and are proxied through
+another remap rule which must contain the ``cache_range_requests`` plugin
+
+Examples::
+
+    map http://ats/ http://parent/ @plugin=slice.so @pparam=--remap-host=loopback
+    map http://loopback/ http://parent/ @plugin=cache_range_requests.so
+
+Alternatively::
+
+    map http://ats/ http://parent/ @plugin=slice.so @pparam=-r @pparam=loopback
+    map http://loopback/ http://parent/ @plugin=cache_range_requests.so
+
+Current Limitations
+===================
 
 Since the Slice plugin is written as an intercept handler it loses the
-ability to use normal state machine hooks and transaction states.
+ability to use normal state machine hooks and transaction states. This
+functionality is handled by using the ``cache_range_requests`` plugin
+to interact with ATS.
diff --git a/plugins/experimental/slice/Config.cc b/plugins/experimental/slice/Config.cc
@@ -29,21 +29,22 @@
 int64_t
 Config::bytesFrom(char const *const valstr)
 {
-  char *endptr       = nullptr;
-  int64_t blockbytes = strtoll(valstr, &endptr, 10);
+  char *endptr          = nullptr;
+  int64_t blockbytes    = strtoll(valstr, &endptr, 10);
+  constexpr int64_t kib = 1024;
 
   if (nullptr != endptr && valstr < endptr) {
     size_t const dist = endptr - valstr;
     if (dist < strlen(valstr) && 0 <= blockbytes) {
       switch (tolower(*endptr)) {
       case 'g':
-        blockbytes *= (static_cast<int64_t>(1024) * static_cast<int64_t>(1024) * static_cast<int64_t>(1024));
+        blockbytes *= (kib * kib * kib);
         break;
       case 'm':
-        blockbytes *= (static_cast<int64_t>(1024) * static_cast<int64_t>(1024));
+        blockbytes *= (kib * kib);
         break;
       case 'k':
-        blockbytes *= static_cast<int64_t>(1024);
+        blockbytes *= kib;
         break;
       default:
         break;
@@ -66,7 +67,7 @@ Config::fromArgs(int const argc, char const *const argv[])
     DEBUG_LOG("args[%d] = %s", index, argv[index]);
   }
 
-  // current "best" blockbytes from configuration
+  // look for lowest priority deprecated blockbytes
   int64_t blockbytes = 0;
 
   // backwards compat: look for blockbytes
@@ -91,19 +92,19 @@ Config::fromArgs(int const argc, char const *const argv[])
   }
 
   // standard parsing
-  constexpr const struct option longopts[] = {
+  constexpr struct option longopts[] = {
     {const_cast<char *>("blockbytes"), required_argument, nullptr, 'b'},
-    {const_cast<char *>("test-blockbytes"), required_argument, nullptr, 't'},
+    {const_cast<char *>("blockbytes-test"), required_argument, nullptr, 't'},
+    {const_cast<char *>("remap-host"), required_argument, nullptr, 'r'},
     {const_cast<char *>("pace-errorlog"), required_argument, nullptr, 'p'},
     {const_cast<char *>("disable-errorlog"), no_argument, nullptr, 'd'},
     {nullptr, 0, nullptr, 0},
   };
 
   // getopt assumes args start at '1' so this hack is needed
   char *const *argvp = (const_cast<char *const *>(argv) - 1);
-
   for (;;) {
-    int const opt = getopt_long(argc + 1, argvp, "b:t:p:d", longopts, nullptr);
+    int const opt = getopt_long(argc + 1, argvp, "b:t:r:p:d", longopts, nullptr);
     if (-1 == opt) {
       break;
     }
@@ -120,25 +121,29 @@ Config::fromArgs(int const argc, char const *const argv[])
         ERROR_LOG("Invalid blockbytes: %s", optarg);
       }
     } break;
-    case 't':
+    case 't': {
       if (0 == blockbytes) {
         int64_t const bytesread = bytesFrom(optarg);
         if (0 < bytesread) {
-          DEBUG_LOG("Using blockbytestest %" PRId64, bytesread);
+          DEBUG_LOG("Using blockbytes-test %" PRId64, bytesread);
           blockbytes = bytesread;
         } else {
-          ERROR_LOG("Invalid blockbytestest: %s", optarg);
+          ERROR_LOG("Invalid blockbytes-test: %s", optarg);
         }
       } else {
-        DEBUG_LOG("Skipping blockbytestest in favor of blockbytes");
+        DEBUG_LOG("Skipping blockbytes-test in favor of blockbytes");
       }
+    } break;
+    case 'r':
+      m_remaphost = optarg;
+      DEBUG_LOG("Using loopback remap host override: %s", m_remaphost.c_str());
       break;
     case 'p': {
       int const secsread = atoi(optarg);
       if (0 < secsread) {
         m_paceerrsecs = std::min(secsread, 60);
       } else {
-        DEBUG_LOG("Ignoring pace-errlog argument");
+        ERROR_LOG("Ignoring pace-errlog argument");
       }
     } break;
     case 'd':
@@ -170,8 +175,6 @@ Config::fromArgs(int const argc, char const *const argv[])
 bool
 Config::canLogError()
 {
-  std::lock_guard<std::mutex> const guard(m_mutex);
-
   if (m_paceerrsecs < 0) {
     return false;
   } else if (0 == m_paceerrsecs) {
@@ -180,13 +183,18 @@ Config::canLogError()
 
 #if !defined(UNITTEST)
   TSHRTime const timenow = TShrtime();
+#endif
+
+  std::lock_guard<std::mutex> const guard(m_mutex);
+
+#if !defined(UNITTEST)
   if (timenow < m_nextlogtime) {
     return false;
   }
 
   m_nextlogtime = timenow + TS_HRTIME_SECONDS(m_paceerrsecs);
 #else
-  m_nextlogtime = 0; // thanks clang
+  m_nextlogtime = 0; // needed by clang
 #endif
 
   return true;

diff --git a/plugins/experimental/slice/Config.h b/plugins/experimental/slice/Config.h
@@ -29,7 +29,8 @@ struct Config {
   static constexpr int64_t const blockbytesdefault = 1024 * 1024;      // 1MB
 
   int64_t m_blockbytes{blockbytesdefault};
-  int m_paceerrsecs{0}; // -1 disable logging, 0 no pacing, max 60s
+  std::string m_remaphost; // remap host to use for loopback slice GET
+  int m_paceerrsecs{0};    // -1 disable logging, 0 no pacing, max 60s
 
   // Convert optarg to bytes
   static int64_t bytesFrom(char const *const valstr);

diff --git a/plugins/experimental/slice/Data.h b/plugins/experimental/slice/Data.h
@@ -39,8 +39,8 @@ struct Data {
 
   sockaddr_storage m_client_ip;
 
-  // for pristine url coming in
-  TSMBuffer m_urlbuffer{nullptr};
+  // for pristine/effective url coming in
+  TSMBuffer m_urlbuf{nullptr};
   TSMLoc m_urlloc{nullptr};
 
   char m_hostname[8192];
@@ -80,7 +80,7 @@ struct Data {
   explicit Data(Config *const config)
     : m_config(config),
       m_client_ip(),
-      m_urlbuffer(nullptr),
+      m_urlbuf(nullptr),
       m_urlloc(nullptr),
       m_hostlen(0),
       m_etaglen(0),
@@ -113,11 +113,11 @@ struct Data {
   ~Data()
   {
     // decrData();
-    if (nullptr != m_urlbuffer) {
+    if (nullptr != m_urlbuf) {
       if (nullptr != m_urlloc) {
-        TSHandleMLocRelease(m_urlbuffer, TS_NULL_MLOC, m_urlloc);
+        TSHandleMLocRelease(m_urlbuf, TS_NULL_MLOC, m_urlloc);
       }
-      TSMBufferDestroy(m_urlbuffer);
+      TSMBufferDestroy(m_urlbuf);
     }
     if (nullptr != m_http_parser) {
       TSHttpParserDestroy(m_http_parser);