block_errors: Fix plugin message handling

[bneradt]

The msg_hook handler was not verifying the event type and was incorrectly logging errors for messages intended for other plugins. ATS core broadcasts all plugin messages to all registered handlers, so each plugin must filter for its own messages. Added TS_EVENT_LIFECYCLE_MSG check, filter messages by 'block_errors' target prefix, and used TextView to parse target/command.

[bryancall]

Thanks for fixing this bug! The message filtering logic is correct and the test coverage is comprehensive.

Suggestion: We should prefer std::string and std::string_view instead of swoc::TextView. Per PR/issue scrub discussion, if we can easily use STL types we should prefer them over adding dependencies that aren't as well maintained.

Something like:

#include <string>
#include <string_view>

static int
msg_hook(TSCont * /* contp ATS_UNUSED */, TSEvent event, void *edata)
{
  if (event != TS_EVENT_LIFECYCLE_MSG) {
    TSError("block_errors: unexpected event %d", event);
    return TS_EVENT_NONE;
  }

  TSPluginMsg *msg = static_cast<TSPluginMsg *>(edata);
  std::string_view tag{msg->tag};

  // Filter for messages meant for this plugin
  static constexpr std::string_view PREFIX{"block_errors."};
  if (tag.substr(0, PREFIX.size()) != PREFIX) {
    Dbg(dbg_ctl, "msg_hook: message for a different plugin: %.*s",
        static_cast<int>(tag.size()), tag.data());
    return TS_EVENT_NONE;
  }

  std::string_view command = tag.substr(PREFIX.size());
  std::string data{static_cast<const char *>(msg->data), msg->data_size};

  Dbg(dbg_ctl, "msg_hook: command=%.*s data=%s",
      static_cast<int>(command.size()), command.data(), data.c_str());

  if (command == "enabled") {
    enabled = static_cast<bool>(std::stoi(data));
  } else if (command == "limit") {
    RESET_LIMIT = std::stoi(data);
  } else if (command == "cycles") {
    TIMEOUT_CYCLES = std::stoi(data);
  } else if (command == "shutdown") {
    shutdown_connection = static_cast<bool>(std::stoi(data));
  } else {
    Dbg(dbg_ctl, "msg_hook: unknown command '%.*s'",
        static_cast<int>(command.size()), command.data());
    TSError("block_errors: unknown command '%.*s'",
            static_cast<int>(command.size()), command.data());
  }

  Dbg(dbg_ctl, "reset limit: %d per minute, timeout limit: %d minutes, shutdown connection: %d enabled: %d",
      RESET_LIMIT, TIMEOUT_CYCLES, shutdown_connection, enabled);

  return TS_EVENT_NONE;
}

[bryancall]

Diff with master:

diff --git a/plugins/experimental/block_errors/block_errors.cc b/plugins/experimental/block_errors/block_errors.cc
index 88fe3bf57af..xxxxx 100644
--- a/plugins/experimental/block_errors/block_errors.cc
+++ b/plugins/experimental/block_errors/block_errors.cc
@@ -17,6 +17,7 @@
 
 #include <ts/ts.h>
 #include <unordered_map>
+#include <string>
 #include <limits>
 #include <swoc/IPAddr.h>
 #include <swoc/bwf_ip.h>
@@ -37,27 +38,42 @@ static bool     enabled             = true;
 
 //-------------------------------------------------------------------------
 static int
-msg_hook(TSCont * /* contp ATS_UNUSED */, TSEvent /* event ATS_UNUSED */, void *edata)
+msg_hook(TSCont * /* contp ATS_UNUSED */, TSEvent event, void *edata)
 {
-  TSPluginMsg     *msg = static_cast<TSPluginMsg *>(edata);
-  std::string_view tag(static_cast<const char *>(msg->tag));
-  std::string_view data(static_cast<const char *>(msg->data));
+  if (event != TS_EVENT_LIFECYCLE_MSG) {
+    TSError("block_errors: unexpected event %d", event);
+    return TS_EVENT_NONE;
+  }
+
+  TSPluginMsg *msg = static_cast<TSPluginMsg *>(edata);
+  std::string_view tag{msg->tag};
+
+  // Filter for messages meant for this plugin
+  static constexpr std::string_view PREFIX{"block_errors."};
+  if (tag.substr(0, PREFIX.size()) != PREFIX) {
+    Dbg(dbg_ctl, "msg_hook: message for a different plugin: %.*s",
+        static_cast<int>(tag.size()), tag.data());
+    return TS_EVENT_NONE;
+  }
 
-  Dbg(dbg_ctl, "msg_hook: tag=%s data=%s", tag.data(), data.data());
+  std::string_view command = tag.substr(PREFIX.size());
+  std::string data{static_cast<const char *>(msg->data), msg->data_size};
 
-  if (tag == "block_errors.enabled") {
-    enabled = static_cast<bool>(atoi(data.data()));
-  } else if (tag == "block_errors.limit") {
-    RESET_LIMIT = atoi(data.data());
-  } else if (tag == "block_errors.cycles") {
-    TIMEOUT_CYCLES = atoi(data.data());
-  } else if (tag == "block_errors.shutdown") {
-    shutdown_connection = static_cast<bool>(atoi(data.data()));
+  Dbg(dbg_ctl, "msg_hook: command=%.*s data=%s",
+      static_cast<int>(command.size()), command.data(), data.c_str());
+
+  if (command == "enabled") {
+    enabled = static_cast<bool>(std::stoi(data));
+  } else if (command == "limit") {
+    RESET_LIMIT = std::stoi(data);
+  } else if (command == "cycles") {
+    TIMEOUT_CYCLES = std::stoi(data);
+  } else if (command == "shutdown") {
+    shutdown_connection = static_cast<bool>(std::stoi(data));
   } else {
-    Dbg(dbg_ctl, "msg_hook: unknown message tag '%s'", tag.data());
-    TSError("block_errors: unknown message tag '%s'", tag.data());
+    Dbg(dbg_ctl, "msg_hook: unknown command '%.*s'",
+        static_cast<int>(command.size()), command.data());
+    TSError("block_errors: unknown command '%.*s'",
+        static_cast<int>(command.size()), command.data());
   }
 
   Dbg(dbg_ctl, "reset limit: %d per minute, timeout limit: %d minutes, shutdown connection: %d enabled: %d", RESET_LIMIT,
       TIMEOUT_CYCLES, shutdown_connection, enabled);
 
-  return 0;
+  return TS_EVENT_NONE;
 }

[bryancall]

Please take a look at the comments on the PR

[bneradt]

But TextView simplifies the logic. It uses message.take_prefix_at('.');. I typically only use TextView when the added API over string_view is helpful.

Are we deprecating TextView? If not, we should make use of the API.