You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by zw...@apache.org on 2016/06/27 18:07:48 UTC

[trafficserver] 02/02: TS-4449 Better errors and debug output

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

zwoop pushed a commit to branch master
in repository https://git-dual.apache.org/repos/asf/trafficserver.git

commit b905a52a4a7a0a3d05966bc1a74ee6407cf94c39
Author: Leif Hedstrom <zw...@apache.org>
AuthorDate: Tue May 17 13:12:19 2016 -0600

    TS-4449 Better errors and debug output
    
    This does a few things:
    
       1. Better error reports when parsing a config file
       2. Better diagnostics when debugging expressions
    
    I also added an example to the docs, for a common use case that
    turns out to be non-obvious.
---
 doc/admin-guide/plugins/header_rewrite.en.rst |  12 +++
 plugins/header_rewrite/conditions.cc          | 140 ++++++++++++++------------
 plugins/header_rewrite/conditions.h           |  26 +++++
 plugins/header_rewrite/header_rewrite.cc      |   6 +-
 plugins/header_rewrite/matcher.h              |  47 ++++++---
 plugins/header_rewrite/operators.cc           |   8 +-
 plugins/header_rewrite/regex_helper.h         |   9 +-
 plugins/header_rewrite/ruleset.cc             |  10 +-
 plugins/header_rewrite/ruleset.h              |   4 +-
 9 files changed, 163 insertions(+), 99 deletions(-)

diff --git a/doc/admin-guide/plugins/header_rewrite.en.rst b/doc/admin-guide/plugins/header_rewrite.en.rst
index 8b76e82..0a78ac3 100644
--- a/doc/admin-guide/plugins/header_rewrite.en.rst
+++ b/doc/admin-guide/plugins/header_rewrite.en.rst
@@ -1017,3 +1017,15 @@ Query string when the Origin server times out or the connection is refused::
     cond %{STATUS} =502 [OR]
     cond %{STATUS} =504
     set-redirect 302 http://different_origin.example.com/%{PATH} [QSA]
+
+Check for existence of a header
+-------------------------------
+
+This rule will modify the ``Cache-Control`` header, but only if it is not
+already set to some value, and the status code is a 2xx::
+
+    cond %{READ_RESPONSE_HDR_HOOK} [AND]
+    cond %{HEADER:Cache-Control} ="" [AND]
+    cond %{STATUS} >199 [AND]
+    cond %{STATUS} <300
+    set-header Cache-Control "max-age=600, public"
diff --git a/plugins/header_rewrite/conditions.cc b/plugins/header_rewrite/conditions.cc
index d3e4afa..5f9685a 100644
--- a/plugins/header_rewrite/conditions.cc
+++ b/plugins/header_rewrite/conditions.cc
@@ -35,7 +35,7 @@ void
 ConditionStatus::initialize(Parser &p)
 {
   Condition::initialize(p);
-  Matchers<TSHttpStatus> *match = new Matchers<TSHttpStatus>(_cond_op);
+  MatcherType *match = new MatcherType(_cond_op);
 
   match->set(static_cast<TSHttpStatus>(strtol(p.get_arg().c_str(), NULL, 10)));
   _matcher = match;
@@ -55,14 +55,15 @@ ConditionStatus::initialize_hooks()
 bool
 ConditionStatus::eval(const Resources &res)
 {
-  TSDebug(PLUGIN_NAME, "Evaluating STATUS()"); // TODO: It'd be nice to get the args here ...
-  return static_cast<const Matchers<TSHttpStatus> *>(_matcher)->test(res.resp_status);
+  TSDebug(PLUGIN_NAME, "Evaluating STATUS()");
+  return static_cast<MatcherType *>(_matcher)->test(res.resp_status);
 }
 
 void
 ConditionStatus::append_value(std::string &s, const Resources &res)
 {
   std::ostringstream oss;
+
   oss << res.resp_status;
   s += oss.str();
   TSDebug(PLUGIN_NAME, "Appending STATUS(%d) to evaluation value -> %s", res.resp_status, s.c_str());
@@ -73,10 +74,9 @@ void
 ConditionMethod::initialize(Parser &p)
 {
   Condition::initialize(p);
-  Matchers<std::string> *match = new Matchers<std::string>(_cond_op);
+  MatcherType *match = new MatcherType(_cond_op);
 
   match->set(p.get_arg());
-
   _matcher = match;
 }
 
@@ -86,9 +86,9 @@ ConditionMethod::eval(const Resources &res)
   std::string s;
 
   append_value(s, res);
-  bool rval = static_cast<const Matchers<std::string> *>(_matcher)->test(s);
-  TSDebug(PLUGIN_NAME, "Evaluating METHOD(): %s - rval: %d", s.c_str(), rval);
-  return rval;
+  TSDebug(PLUGIN_NAME, "Evaluating METHOD()");
+
+  return static_cast<const MatcherType *>(_matcher)->test(s);
 }
 
 void
@@ -115,7 +115,7 @@ ConditionRandom::initialize(Parser &p)
 {
   struct timeval tv;
   Condition::initialize(p);
-  Matchers<unsigned int> *match = new Matchers<unsigned int>(_cond_op);
+  MatcherType *match = new MatcherType(_cond_op);
 
   gettimeofday(&tv, NULL);
   _seed = getpid() * tv.tv_usec;
@@ -128,8 +128,8 @@ ConditionRandom::initialize(Parser &p)
 bool
 ConditionRandom::eval(const Resources & /* res ATS_UNUSED */)
 {
-  TSDebug(PLUGIN_NAME, "Evaluating RANDOM(%d)", _max);
-  return static_cast<const Matchers<unsigned int> *>(_matcher)->test(rand_r(&_seed) % _max);
+  TSDebug(PLUGIN_NAME, "Evaluating RANDOM()");
+  return static_cast<const MatcherType *>(_matcher)->test(rand_r(&_seed) % _max);
 }
 
 void
@@ -171,8 +171,6 @@ ConditionAccess::eval(const Resources & /* res ATS_UNUSED */)
   struct timeval tv;
 
   gettimeofday(&tv, NULL);
-
-  TSDebug(PLUGIN_NAME, "Evaluating ACCESS(%s)", _qualifier.c_str());
   if (tv.tv_sec > _next) {
     // There is a small "race" here, where we could end up calling access() a few times extra. I think
     // that is OK, and not worth protecting with a lock.
@@ -183,6 +181,7 @@ ConditionAccess::eval(const Resources & /* res ATS_UNUSED */)
     _next = tv.tv_sec; // I hope this is an atomic "set"...
     _last = check;     // This sure ought to be
   }
+  TSDebug(PLUGIN_NAME, "Evaluating ACCESS(%s) -> %d", _qualifier.c_str(), _last);
 
   return _last;
 }
@@ -192,7 +191,7 @@ void
 ConditionHeader::initialize(Parser &p)
 {
   Condition::initialize(p);
-  Matchers<std::string> *match = new Matchers<std::string>(_cond_op);
+  MatcherType *match = new MatcherType(_cond_op);
 
   match->set(p.get_arg());
   _matcher = match;
@@ -246,9 +245,9 @@ ConditionHeader::eval(const Resources &res)
   std::string s;
 
   append_value(s, res);
-  bool rval = static_cast<const Matchers<std::string> *>(_matcher)->test(s);
-  TSDebug(PLUGIN_NAME, "Evaluating HEADER(): %s - rval: %d", s.c_str(), rval);
-  return rval;
+  TSDebug(PLUGIN_NAME, "Evaluating HEADER()");
+
+  return static_cast<const MatcherType *>(_matcher)->test(s);
 }
 
 // ConditionPath
@@ -256,7 +255,7 @@ void
 ConditionPath::initialize(Parser &p)
 {
   Condition::initialize(p);
-  Matchers<std::string> *match = new Matchers<std::string>(_cond_op);
+  MatcherType *match = new MatcherType(_cond_op);
 
   match->set(p.get_arg());
   _matcher = match;
@@ -286,9 +285,9 @@ ConditionPath::eval(const Resources &res)
   std::string s;
 
   append_value(s, res);
-  TSDebug(PLUGIN_NAME, "Evaluating PATH");
+  TSDebug(PLUGIN_NAME, "Evaluating PATH()");
 
-  return static_cast<const Matchers<std::string> *>(_matcher)->test(s);
+  return static_cast<MatcherType *>(_matcher)->test(s);
 }
 
 // ConditionQuery
@@ -296,7 +295,7 @@ void
 ConditionQuery::initialize(Parser &p)
 {
   Condition::initialize(p);
-  Matchers<std::string> *match = new Matchers<std::string>(_cond_op);
+  MatcherType *match = new MatcherType(_cond_op);
 
   match->set(p.get_arg());
   _matcher = match;
@@ -315,15 +314,17 @@ ConditionQuery::append_value(std::string &s, const Resources &res)
 bool
 ConditionQuery::eval(const Resources &res)
 {
-  std::string s;
+  if (NULL != res._rri) {
+    std::string s;
+
+    append_value(s, res);
+    TSDebug(PLUGIN_NAME, "Evaluating QUERY()");
 
-  if (NULL == res._rri) {
-    TSDebug(PLUGIN_NAME, "QUERY requires remap initialization! Evaluating to false!");
-    return false;
+    return static_cast<const MatcherType *>(_matcher)->test(s);
   }
-  append_value(s, res);
-  TSDebug(PLUGIN_NAME, "Evaluating QUERY - %s", s.c_str());
-  return static_cast<const Matchers<std::string> *>(_matcher)->test(s);
+
+  TSDebug(PLUGIN_NAME, "\tQUERY requires remap initialization! Evaluating to false!");
+  return false;
 }
 
 // ConditionUrl: request or response header. TODO: This is not finished, at all!!!
@@ -332,7 +333,7 @@ ConditionUrl::initialize(Parser &p)
 {
   Condition::initialize(p);
 
-  Matchers<std::string> *match = new Matchers<std::string>(_cond_op);
+  MatcherType *match = new MatcherType(_cond_op);
   match->set(p.get_arg());
   _matcher = match;
 }
@@ -410,7 +411,7 @@ ConditionDBM::initialize(Parser &p)
 {
   Condition::initialize(p);
 
-  Matchers<std::string> *match = new Matchers<std::string>(_cond_op);
+  MatcherType *match = new MatcherType(_cond_op);
   match->set(p.get_arg());
   _matcher = match;
 
@@ -463,9 +464,9 @@ ConditionDBM::eval(const Resources &res)
   std::string s;
 
   append_value(s, res);
-  TSDebug(PLUGIN_NAME, "Evaluating DBM(%s, \"%s\")", _file.c_str(), s.c_str());
+  TSDebug(PLUGIN_NAME, "Evaluating DBM()");
 
-  return static_cast<const Matchers<std::string> *>(_matcher)->test(s);
+  return static_cast<const MatcherType *>(_matcher)->test(s);
 }
 
 // ConditionCookie: request or response header
@@ -474,9 +475,9 @@ ConditionCookie::initialize(Parser &p)
 {
   Condition::initialize(p);
 
-  Matchers<std::string> *match = new Matchers<std::string>(_cond_op);
-  match->set(p.get_arg());
+  MatcherType *match = new MatcherType(_cond_op);
 
+  match->set(p.get_arg());
   _matcher = match;
 
   require_resources(RSRC_CLIENT_REQUEST_HEADERS);
@@ -533,15 +534,19 @@ ConditionCookie::eval(const Resources &res)
   std::string s;
 
   append_value(s, res);
-  bool rval = static_cast<const Matchers<std::string> *>(_matcher)->test(s);
-  TSDebug(PLUGIN_NAME, "Evaluating COOKIE(%s): %s: rval: %d", _qualifier.c_str(), s.c_str(), rval);
-  return rval;
+  TSDebug(PLUGIN_NAME, "Evaluating COOKIE()");
+
+  return static_cast<const MatcherType *>(_matcher)->test(s);
 }
 
+// ConditionInternalTxn: Is the txn internal?
 bool
 ConditionInternalTxn::eval(const Resources &res)
 {
-  return TSHttpTxnIsInternal(res.txnp) == TS_SUCCESS;
+  bool ret = (TSHttpTxnIsInternal(res.txnp) == TS_SUCCESS);
+
+  TSDebug(PLUGIN_NAME, "Evaluating INTERNAL-TRANSACTION() -> %d", ret);
+  return ret;
 }
 
 void
@@ -549,9 +554,9 @@ ConditionClientIp::initialize(Parser &p)
 {
   Condition::initialize(p);
 
-  Matchers<std::string> *match = new Matchers<std::string>(_cond_op);
-  match->set(p.get_arg());
+  MatcherType *match = new MatcherType(_cond_op);
 
+  match->set(p.get_arg());
   _matcher = match;
 }
 
@@ -561,9 +566,9 @@ ConditionClientIp::eval(const Resources &res)
   std::string s;
 
   append_value(s, res);
-  bool rval = static_cast<const Matchers<std::string> *>(_matcher)->test(s);
-  TSDebug(PLUGIN_NAME, "Evaluating CLIENT-IP(): %s: rval: %d", s.c_str(), rval);
-  return rval;
+  TSDebug(PLUGIN_NAME, "Evaluating CLIENT-IP()");
+
+  return static_cast<MatcherType *>(_matcher)->test(s);
 }
 
 void
@@ -581,7 +586,8 @@ ConditionIncomingPort::initialize(Parser &p)
 {
   Condition::initialize(p);
 
-  Matchers<uint16_t> *match = new Matchers<uint16_t>(_cond_op);
+  MatcherType *match = new MatcherType(_cond_op);
+
   match->set(static_cast<uint16_t>(strtoul(p.get_arg().c_str(), NULL, 10)));
   _matcher = match;
 }
@@ -590,9 +596,9 @@ bool
 ConditionIncomingPort::eval(const Resources &res)
 {
   uint16_t port = getPort(TSHttpTxnIncomingAddrGet(res.txnp));
-  bool rval     = static_cast<const Matchers<uint16_t> *>(_matcher)->test(port);
-  TSDebug(PLUGIN_NAME, "Evaluating INCOMING-PORT(): %d: rval: %d", port, rval);
-  return rval;
+
+  TSDebug(PLUGIN_NAME, "Evaluating INCOMING-PORT()");
+  return static_cast<MatcherType *>(_matcher)->test(port);
 }
 
 void
@@ -600,6 +606,7 @@ ConditionIncomingPort::append_value(std::string &s, const Resources &res)
 {
   std::ostringstream oss;
   uint16_t port = getPort(TSHttpTxnIncomingAddrGet(res.txnp));
+
   oss << port;
   s += oss.str();
   TSDebug(PLUGIN_NAME, "Appending %d to evaluation value -> %s", port, s.c_str());
@@ -610,11 +617,10 @@ void
 ConditionTransactCount::initialize(Parser &p)
 {
   Condition::initialize(p);
-
   MatcherType *match     = new MatcherType(_cond_op);
   std::string const &arg = p.get_arg();
-  match->set(strtol(arg.c_str(), NULL, 10));
 
+  match->set(strtol(arg.c_str(), NULL, 10));
   _matcher = match;
 }
 
@@ -622,15 +628,16 @@ bool
 ConditionTransactCount::eval(const Resources &res)
 {
   TSHttpSsn ssn = TSHttpTxnSsnGet(res.txnp);
-  bool rval     = false;
+
   if (ssn) {
     int n = TSHttpSsnTransactionCount(ssn);
-    rval  = static_cast<MatcherType *>(_matcher)->test(n);
-    TSDebug(PLUGIN_NAME, "Evaluating TXN-COUNT(): %d: rval: %s", n, rval ? "true" : "false");
-  } else {
-    TSDebug(PLUGIN_NAME, "Evaluation TXN-COUNT(): No session found, returning false");
+
+    TSDebug(PLUGIN_NAME, "Evaluating TXN-COUNT()");
+    return static_cast<MatcherType *>(_matcher)->test(n);
   }
-  return rval;
+
+  TSDebug(PLUGIN_NAME, "\tNo session found, returning false");
+  return false;
 }
 
 void
@@ -642,6 +649,7 @@ ConditionTransactCount::append_value(std::string &s, Resources const &res)
     char value[32]; // enough for UINT64_MAX
     int count  = TSHttpSsnTransactionCount(ssn);
     int length = ink_fast_itoa(count, value, sizeof(value));
+
     if (length > 0) {
       TSDebug(PLUGIN_NAME, "Appending TXN-COUNT %s to evaluation value %.*s", _qualifier.c_str(), length, value);
       s.append(value, length);
@@ -699,7 +707,8 @@ void
 ConditionNow::initialize(Parser &p)
 {
   Condition::initialize(p);
-  Matchers<int64_t> *match = new Matchers<int64_t>(_cond_op);
+
+  MatcherType *match = new MatcherType(_cond_op);
 
   match->set(static_cast<int64_t>(strtol(p.get_arg().c_str(), NULL, 10)));
   _matcher = match;
@@ -748,9 +757,8 @@ ConditionNow::eval(const Resources &res)
 {
   int64_t now = get_now_qualified(_now_qual);
 
-  TSDebug(PLUGIN_NAME, "Evaluating NOW() -> %" PRId64, now);
-
-  return static_cast<const Matchers<int64_t> *>(_matcher)->test(now);
+  TSDebug(PLUGIN_NAME, "Evaluating NOW()");
+  return static_cast<const MatcherType *>(_matcher)->test(now);
 }
 
 // ConditionGeo: Geo-based information (integer). See ConditionGeoCountry for the string version.
@@ -969,19 +977,19 @@ ConditionGeo::append_value(std::string &s, const Resources &res)
 bool
 ConditionGeo::eval(const Resources &res)
 {
+  bool ret = false;
+
+  TSDebug(PLUGIN_NAME, "Evaluating GEO()");
   if (is_int_type()) {
     int64_t geo = get_geo_int(TSHttpTxnClientAddrGet(res.txnp));
 
-    TSDebug(PLUGIN_NAME, "Evaluating GEO() -> %" PRId64, geo);
-
-    return static_cast<const Matchers<int64_t> *>(_matcher)->test(geo);
+    ret = static_cast<const Matchers<int64_t> *>(_matcher)->test(geo);
   } else {
     std::string s;
 
     append_value(s, res);
-    bool rval = static_cast<const Matchers<std::string> *>(_matcher)->test(s);
-
-    TSDebug(PLUGIN_NAME, "Evaluating GEO(): %s - rval: %d", s.c_str(), rval);
-    return rval;
+    ret = static_cast<const Matchers<std::string> *>(_matcher)->test(s);
   }
+
+  return ret;
 }
diff --git a/plugins/header_rewrite/conditions.h b/plugins/header_rewrite/conditions.h
index e733147..32915c8 100644
--- a/plugins/header_rewrite/conditions.h
+++ b/plugins/header_rewrite/conditions.h
@@ -87,6 +87,8 @@ private:
 // Check the HTTP return status
 class ConditionStatus : public Condition
 {
+  typedef Matchers<TSHttpStatus> MatcherType;
+
 public:
   ConditionStatus() { TSDebug(PLUGIN_NAME_DBG, "Calling CTOR for ConditionStatus"); }
   void initialize(Parser &p);
@@ -103,6 +105,8 @@ private:
 // Check the HTTP method
 class ConditionMethod : public Condition
 {
+  typedef Matchers<std::string> MatcherType;
+
 public:
   ConditionMethod() { TSDebug(PLUGIN_NAME_DBG, "Calling CTOR for ConditionMethod"); }
   void initialize(Parser &p);
@@ -118,6 +122,8 @@ private:
 // Random 0 to (N-1)
 class ConditionRandom : public Condition
 {
+  typedef Matchers<unsigned int> MatcherType;
+
 public:
   ConditionRandom() : _seed(0), _max(0) { TSDebug(PLUGIN_NAME_DBG, "Calling CTOR for ConditionRandom"); }
   void initialize(Parser &p);
@@ -154,6 +160,8 @@ private:
 // cookie(name)
 class ConditionCookie : public Condition
 {
+  typedef Matchers<std::string> MatcherType;
+
 public:
   ConditionCookie() { TSDebug(PLUGIN_NAME_DBG, "Calling CTOR for ConditionCookie"); }
   void initialize(Parser &p);
@@ -220,6 +228,8 @@ private:
 // header
 class ConditionHeader : public Condition
 {
+  typedef Matchers<std::string> MatcherType;
+
 public:
   explicit ConditionHeader(bool client = false) : _client(client)
   {
@@ -241,6 +251,8 @@ private:
 // path
 class ConditionPath : public Condition
 {
+  typedef Matchers<std::string> MatcherType;
+
 public:
   explicit ConditionPath() { TSDebug(PLUGIN_NAME_DBG, "Calling CTOR for ConditionPath"); }
   void initialize(Parser &p);
@@ -256,6 +268,8 @@ private:
 // query
 class ConditionQuery : public Condition
 {
+  typedef Matchers<std::string> MatcherType;
+
 public:
   explicit ConditionQuery() { TSDebug(PLUGIN_NAME_DBG, "Calling CTOR for ConditionQuery"); }
   void initialize(Parser &p);
@@ -271,6 +285,8 @@ private:
 // url
 class ConditionUrl : public Condition
 {
+  typedef Matchers<std::string> MatcherType;
+
 public:
   enum UrlType { CLIENT, URL, FROM, TO };
 
@@ -296,6 +312,8 @@ private:
 // DBM lookups
 class ConditionDBM : public Condition
 {
+  typedef Matchers<std::string> MatcherType;
+
 public:
   ConditionDBM()
     : //_dbm(NULL),
@@ -329,6 +347,8 @@ private:
 
 class ConditionInternalTxn : public Condition
 {
+  typedef Matchers<std::string> MatcherType;
+
 public:
   void
   append_value(std::string & /* s ATS_UNUSED */, const Resources & /* res ATS_UNUSED */)
@@ -341,6 +361,8 @@ protected:
 
 class ConditionClientIp : public Condition
 {
+  typedef Matchers<std::string> MatcherType;
+
 public:
   void initialize(Parser &p);
   void append_value(std::string &s, const Resources &res);
@@ -351,6 +373,8 @@ protected:
 
 class ConditionIncomingPort : public Condition
 {
+  typedef Matchers<uint16_t> MatcherType;
+
 public:
   ConditionIncomingPort() { TSDebug(PLUGIN_NAME_DBG, "Calling CTOR for ConditionIncomingPort"); }
   void initialize(Parser &p);
@@ -383,6 +407,8 @@ private:
 // now: Keeping track of current time / day / hour etc.
 class ConditionNow : public Condition
 {
+  typedef Matchers<int64_t> MatcherType;
+
 public:
   explicit ConditionNow() : _now_qual(NOW_QUAL_EPOCH) { TSDebug(PLUGIN_NAME_DBG, "Calling CTOR for ConditionNow"); }
   void initialize(Parser &p);
diff --git a/plugins/header_rewrite/header_rewrite.cc b/plugins/header_rewrite/header_rewrite.cc
index b5e2edb..e07c163 100644
--- a/plugins/header_rewrite/header_rewrite.cc
+++ b/plugins/header_rewrite/header_rewrite.cc
@@ -231,9 +231,9 @@ RulesConfig::parse_config(const std::string fname, TSHttpHookID default_hook)
     }
 
     if (p.is_cond()) {
-      rule->add_condition(p);
+      rule->add_condition(p, filename.c_str());
     } else {
-      rule->add_operator(p);
+      rule->add_operator(p, filename.c_str());
     }
   }
 
@@ -357,7 +357,7 @@ TSPluginInit(int argc, const char *argv[])
     }
   } else {
     // Didn't get anything, nuke it.
-    TSError("[%s] failed to parse configuration file", PLUGIN_NAME);
+    TSError("[%s] failed to parse any configuration file", PLUGIN_NAME);
     conf->release();
   }
 }
diff --git a/plugins/header_rewrite/matcher.h b/plugins/header_rewrite/matcher.h
index a388955..95af794 100644
--- a/plugins/header_rewrite/matcher.h
+++ b/plugins/header_rewrite/matcher.h
@@ -23,7 +23,7 @@
 #define __MATCHER_H__ 1
 
 #include <string>
-#include <iostream> // For debugging
+#include <sstream>
 
 #include "ts/ts.h"
 
@@ -56,13 +56,11 @@ public:
   {
     _pdata = pdata;
   }
-
   void *
   get_pdata() const
   {
     return _pdata;
   }
-
   virtual void
   free_pdata()
   {
@@ -84,7 +82,7 @@ template <class T> class Matchers : public Matcher
 public:
   explicit Matchers<T>(const MatcherOps op) : Matcher(op), _data() {}
   // Getters / setters
-  const T
+  const T &
   get() const
   {
     return _data;
@@ -94,7 +92,9 @@ public:
   setRegex(const std::string /* data ATS_UNUSED */)
   {
     if (!helper.setRegexMatch(_data)) {
-      std::cout << "Invalid regex:failed to precompile" << std::endl;
+      std::stringstream ss;
+      ss << _data;
+      TSError("[%s] Invalid regex: failed to precompile: %s", PLUGIN_NAME, ss.str().c_str());
       abort();
     }
     TSDebug(PLUGIN_NAME, "Regex precompiled successfully");
@@ -105,7 +105,6 @@ public:
   {
     return;
   }
-
   void
   setRegex(const TSHttpStatus /* t ATS_UNUSED */)
   {
@@ -146,26 +145,47 @@ public:
   }
 
 private:
+  void
+  debug_helper(const T t, const char *op, bool r) const
+  {
+    std::stringstream ss;
+
+    ss << '"' << t << '"' << op << '"' << _data << '"' << " -> " << r;
+    TSDebug(PLUGIN_NAME, "\ttesting: %s", ss.str().c_str());
+  }
+
   // For basic types
   bool
   test_eq(const T t) const
   {
-    // std::cout << "Testing: " << t << " == " << _data << std::endl;
-    return t == _data;
+    bool r = (t == _data);
+
+    if (TSIsDebugTagSet(PLUGIN_NAME)) {
+      debug_helper(t, " == ", r);
+    }
+    return r;
   }
 
   bool
   test_lt(const T t) const
   {
-    // std::cout << "Testing: " << t << " < " << _data << std::endl;
-    return t < _data;
+    bool r = (t < _data);
+
+    if (TSIsDebugTagSet(PLUGIN_NAME)) {
+      debug_helper(t, " < ", r);
+    }
+    return r;
   }
 
   bool
   test_gt(const T t) const
   {
-    // std::cout << "Testing: " << t << " > " << _data << std::endl;
-    return t > _data;
+    bool r = t > _data;
+
+    if (TSIsDebugTagSet(PLUGIN_NAME)) {
+      debug_helper(t, " > ", r);
+    }
+    return r;
   }
 
   bool
@@ -185,8 +205,9 @@ private:
   bool
   test_reg(const std::string t) const
   {
-    TSDebug(PLUGIN_NAME, "Test regular expression %s : %s", _data.c_str(), t.c_str());
     int ovector[OVECCOUNT];
+
+    TSDebug(PLUGIN_NAME, "Test regular expression %s : %s", _data.c_str(), t.c_str());
     if (helper.regexMatch(t.c_str(), t.length(), ovector) > 0) {
       TSDebug(PLUGIN_NAME, "Successfully found regular expression match");
       return true;
diff --git a/plugins/header_rewrite/operators.cc b/plugins/header_rewrite/operators.cc
index 3920797..271edfa 100644
--- a/plugins/header_rewrite/operators.cc
+++ b/plugins/header_rewrite/operators.cc
@@ -470,7 +470,7 @@ OperatorRMHeader::exec(const Resources &res) const
   TSMLoc field_loc, tmp;
 
   if (res.bufp && res.hdr_loc) {
-    TSDebug(PLUGIN_NAME, "OperatorRMHeader::exec() invoked on header %s", _header.c_str());
+    TSDebug(PLUGIN_NAME, "OperatorRMHeader::exec() invoked on %s", _header.c_str());
     field_loc = TSMimeHdrFieldFind(res.bufp, res.hdr_loc, _header.c_str(), _header.size());
     while (field_loc) {
       TSDebug(PLUGIN_NAME, "   Deleting header %s", _header.c_str());
@@ -511,7 +511,7 @@ OperatorAddHeader::exec(const Resources &res) const
   }
 
   if (res.bufp && res.hdr_loc) {
-    TSDebug(PLUGIN_NAME, "OperatorAddHeader::exec() invoked on header %s: %s", _header.c_str(), value.c_str());
+    TSDebug(PLUGIN_NAME, "OperatorAddHeader::exec() invoked on %s: %s", _header.c_str(), value.c_str());
     TSMLoc field_loc;
 
     if (TS_SUCCESS == TSMimeHdrFieldCreateNamed(res.bufp, res.hdr_loc, _header.c_str(), _header.size(), &field_loc)) {
@@ -549,7 +549,7 @@ OperatorSetHeader::exec(const Resources &res) const
   if (res.bufp && res.hdr_loc) {
     TSMLoc field_loc = TSMimeHdrFieldFind(res.bufp, res.hdr_loc, _header.c_str(), _header.size());
 
-    TSDebug(PLUGIN_NAME, "OperatorSetHeader::exec() invoked on header %s: %s", _header.c_str(), value.c_str());
+    TSDebug(PLUGIN_NAME, "OperatorSetHeader::exec() invoked on %s: %s", _header.c_str(), value.c_str());
 
     if (!field_loc) {
       // No existing header, so create one
@@ -616,7 +616,7 @@ OperatorCounter::exec(const Resources & /* ATS_UNUSED res */) const
     return;
   }
 
-  TSDebug(PLUGIN_NAME, "OperatorCounter::exec() invoked on counter %s", _counter_name.c_str());
+  TSDebug(PLUGIN_NAME, "OperatorCounter::exec() invoked on %s", _counter_name.c_str());
   TSStatIntIncrement(_counter, 1);
 }
 
diff --git a/plugins/header_rewrite/regex_helper.h b/plugins/header_rewrite/regex_helper.h
index b4dcc04..caad006 100644
--- a/plugins/header_rewrite/regex_helper.h
+++ b/plugins/header_rewrite/regex_helper.h
@@ -36,13 +36,8 @@ public:
   regexHelper() : regex(NULL), regexExtra(NULL), regexCcount(0) {}
   ~regexHelper()
   {
-    if (regex) {
-      pcre_free(regex);
-    }
-
-    if (regexExtra) {
-      pcre_free(regexExtra);
-    }
+    pcre_free(regex);
+    pcre_free(regexExtra);
   }
 
   bool setRegexMatch(const std::string &s);
diff --git a/plugins/header_rewrite/ruleset.cc b/plugins/header_rewrite/ruleset.cc
index e7cf82e..e92cf56 100644
--- a/plugins/header_rewrite/ruleset.cc
+++ b/plugins/header_rewrite/ruleset.cc
@@ -41,7 +41,7 @@ RuleSet::append(RuleSet *rule)
 }
 
 void
-RuleSet::add_condition(Parser &p)
+RuleSet::add_condition(Parser &p, const char *filename)
 {
   Condition *c = condition_factory(p.get_op());
 
@@ -49,7 +49,8 @@ RuleSet::add_condition(Parser &p)
     TSDebug(PLUGIN_NAME, "   Adding condition: %%{%s} with arg: %s", p.get_op().c_str(), p.get_arg().c_str());
     c->initialize(p);
     if (!c->set_hook(_hook)) {
-      TSError("[%s] can't use this condition in this hook", PLUGIN_NAME);
+      TSError("[%s] in %s: can't use this condition in hook=%d: %%{%s} with arg: %s", PLUGIN_NAME, filename, _hook,
+              p.get_op().c_str(), p.get_arg().c_str());
       return;
     }
     if (NULL == _cond) {
@@ -65,7 +66,7 @@ RuleSet::add_condition(Parser &p)
 }
 
 void
-RuleSet::add_operator(Parser &p)
+RuleSet::add_operator(Parser &p, const char *filename)
 {
   Operator *o = operator_factory(p.get_op());
 
@@ -74,7 +75,8 @@ RuleSet::add_operator(Parser &p)
     TSDebug(PLUGIN_NAME, "   Adding operator: %s(%s)", p.get_op().c_str(), p.get_arg().c_str());
     o->initialize(p);
     if (!o->set_hook(_hook)) {
-      TSError("[%s] can't use this operator in this hook", PLUGIN_NAME);
+      TSError("[%s] in %s: can't use this operator in hook=%d:  %s(%s)", PLUGIN_NAME, filename, _hook, p.get_op().c_str(),
+              p.get_arg().c_str());
       return;
     }
     if (NULL == _oper) {
diff --git a/plugins/header_rewrite/ruleset.h b/plugins/header_rewrite/ruleset.h
index 514abe6..3ff3f14 100644
--- a/plugins/header_rewrite/ruleset.h
+++ b/plugins/header_rewrite/ruleset.h
@@ -47,8 +47,8 @@ public:
 
   // No reason to inline these
   void append(RuleSet *rule);
-  void add_condition(Parser &p);
-  void add_operator(Parser &p);
+  void add_condition(Parser &p, const char *filename);
+  void add_operator(Parser &p, const char *filename);
 
   bool
   has_operator() const

-- 
To stop receiving notification emails like this one, please contact
"commits@trafficserver.apache.org" <co...@trafficserver.apache.org>.