Index NetHandler config by named enum

include/iocore/net/NetHandler.h

@@ -24,8 +24,11 @@
 #pragma once
 
 #include <atomic>
-#include <cstddef>
-#include <type_traits>
+#include <bitset>
+#include <cstdint>
+#include <limits>
+
+#include "tscore/ink_assert.h"
 
 #include "iocore/eventsystem/Continuation.h"
 #include "iocore/eventsystem/EThread.h"
@@ -113,30 +116,38 @@ class NetHandler : public Continuation, public EThread::LoopTailHandler
 
   /// configuration settings for managing the active and keep-alive queues
   struct Config {
+    /// Identifies a config value, so an update can name it instead of passing a pointer.
+    /// @note The underlying type is fixed and must not be narrowed. The index arrives as
+    /// an untyped event cookie, and a value that wrapped would look like a valid index.
+    enum class Index : int {
+      MAX_CONNECTIONS_IN,
+      MAX_REQUESTS_IN,
+      DEFAULT_INACTIVITY_TIMEOUT,
+      COUNT ///< Number of config values, not a valid index.
+    };
+
     uint32_t max_connections_in         = 0;
     uint32_t max_requests_in            = 0;
     uint32_t default_inactivity_timeout = 0;
 
-    /** Return the address of the first value in this struct.
-
-        Doing updates is much easier if we treat this config struct as an array.
-        Making it a method means the knowledge of which member is the first one
-        is localized to this struct, not scattered about.
-     */
+    /// The config value identified by @a idx.
     uint32_t &
-    operator[](int n)
+    operator[](Index idx)
     {
-      return *(&max_connections_in + n);
+      switch (idx) {
+      case Index::MAX_CONNECTIONS_IN:
+        return max_connections_in;
+      case Index::MAX_REQUESTS_IN:
+        return max_requests_in;
+      case Index::DEFAULT_INACTIVITY_TIMEOUT:
+        return default_inactivity_timeout;
+      case Index::COUNT:
+        break;
+      }
+      ink_release_assert(!"invalid NetHandler::Config index");
+      return max_connections_in;
     }
   };
-  // Config is addressed as an array of uint32_t through operator[], and
-  // config_value_affects_per_thread_value is a bitset indexed by field
-  // position, so the offset of each member is part of the interface.
-  static_assert(std::is_standard_layout_v<Config>);    // required for offsetof below to be well defined
-  static_assert(alignof(Config) == alignof(uint32_t)); // a member of wider type would break operator[]
-  static_assert(offsetof(Config, max_connections_in) == 0 * sizeof(uint32_t));
-  static_assert(offsetof(Config, max_requests_in) == 1 * sizeof(uint32_t));
-  static_assert(offsetof(Config, default_inactivity_timeout) == 2 * sizeof(uint32_t));
 
   /** Static global config, set and updated per process.
 
@@ -154,7 +165,7 @@ class NetHandler : public Continuation, public EThread::LoopTailHandler
   uint32_t max_connections_per_thread_in = 0;
   uint32_t max_requests_per_thread_in    = 0;
   /// Number of configuration items in @c Config.
-  static constexpr int CONFIG_ITEM_COUNT = sizeof(Config) / sizeof(uint32_t);
+  static constexpr int CONFIG_ITEM_COUNT = static_cast<int>(Config::Index::COUNT);
   /// Which members of @c Config the per thread values depend on.
   /// If one of these is updated, the per thread values must also be updated.
   static const std::bitset<CONFIG_ITEM_COUNT> config_value_affects_per_thread_value;

src/iocore/net/CMakeLists.txt

@@ -144,6 +144,7 @@ if(BUILD_TESTING)
     test_net
     libinknet_stub.cc
     NetVCTest.cc
+    unit_tests/test_NetHandler.cc
     unit_tests/test_ProxyProtocol.cc
     unit_tests/test_SSLCertLookup.cc
     unit_tests/test_SSLNetVConnectionAsyncEp.cc

src/iocore/net/NetHandler.cc

@@ -30,6 +30,7 @@
 #endif
 
 #include <atomic>
+#include <optional>
 
 using namespace std::literals;
 
@@ -119,17 +120,17 @@ NetHandler::stopCop(NetEvent *ne)
 int
 NetHandler::update_nethandler_config(const char *str, RecDataT, RecData data, void *)
 {
-  uint32_t        *updated_member = nullptr; // direct pointer to config member for update.
-  std::string_view name{str};
+  std::optional<Config::Index> updated_index; // which config value the record maps to, if any.
+  std::string_view             name{str};
 
   if (name == "proxy.config.net.max_connections_in"sv) {
-    updated_member = &NetHandler::global_config.max_connections_in;
+    updated_index = Config::Index::MAX_CONNECTIONS_IN;
     Dbg(dbg_ctl_net_queue, "proxy.config.net.max_connections_in updated to %" PRId64, data.rec_int);
   } else if (name == "proxy.config.net.max_requests_in"sv) {
-    updated_member = &NetHandler::global_config.max_requests_in;
+    updated_index = Config::Index::MAX_REQUESTS_IN;
     Dbg(dbg_ctl_net_queue, "proxy.config.net.max_requests_in updated to %" PRId64, data.rec_int);
   } else if (name == "proxy.config.net.default_inactivity_timeout"sv) {
-    updated_member = &NetHandler::global_config.default_inactivity_timeout;
+    updated_index = Config::Index::DEFAULT_INACTIVITY_TIMEOUT;
     Dbg(dbg_ctl_net_queue, "proxy.config.net.default_inactivity_timeout updated to %" PRId64, data.rec_int);
   } else if (name == "proxy.config.net.additional_accepts"sv) {
     NetHandler::additional_accepts.store(data.rec_int, std::memory_order_relaxed);
@@ -139,10 +140,10 @@ NetHandler::update_nethandler_config(const char *str, RecDataT, RecData data, vo
     Dbg(dbg_ctl_net_queue, "proxy.config.net.per_client.max_connections_in updated to %" PRId64, data.rec_int);
   }
 
-  if (updated_member) {
-    *updated_member = data.rec_int; // do the actual update.
+  if (updated_index) {
+    global_config[*updated_index] = data.rec_int; // do the actual update.
     // portable form of the update, an index converted to <void*> so it can be passed as an event cookie.
-    void *idx = reinterpret_cast<void *>(static_cast<intptr_t>(updated_member - &global_config[0]));
+    void *idx = reinterpret_cast<void *>(static_cast<intptr_t>(*updated_index));
     // Signal the NetHandler instances, passing the index of the updated config value.
     for (int i = 0; i < eventProcessor.n_thread_groups; ++i) {
       if (!active_thread_types[i]) {
@@ -304,10 +305,14 @@ int
 NetHandler::mainNetEvent(int event, Event *e)
 {
   if (TS_EVENT_MGMT_UPDATE == event) {
-    intptr_t idx = reinterpret_cast<intptr_t>(e->cookie);
-    // Copy to the same offset in the instance struct.
+    // Check the raw value before converting: narrowing to the underlying type
+    // could otherwise wrap a bad cookie onto a valid index.
+    auto raw = reinterpret_cast<intptr_t>(e->cookie);
+    ink_release_assert(raw >= 0 && raw < CONFIG_ITEM_COUNT);
+    auto idx = static_cast<Config::Index>(raw);
+    // Copy the updated value to the instance struct.
     config[idx] = global_config[idx];
-    if (config_value_affects_per_thread_value[idx]) {
+    if (config_value_affects_per_thread_value[static_cast<size_t>(idx)]) {
       this->configure_per_thread_values();
     }
     return EVENT_CONT;

src/iocore/net/UnixNet.cc

@@ -41,20 +41,12 @@ std::atomic<bool> net_memory_throttle = false;
 int               fds_throttle;
 ink_hrtime        last_transient_accept_error;
 
-namespace
-{
-/// Config members that @c NetHandler::configure_per_thread_values reads.
-constexpr unsigned long long PER_THREAD_DEPENDENT_CONFIG{0x3};
-// std::bitset silently discards bits at or above its width, which would drop a
-// member from the set without any diagnostic if Config ever shrinks. The first
-// assertion keeps the shift in the second one well defined.
-static_assert(NetHandler::CONFIG_ITEM_COUNT < std::numeric_limits<unsigned long long>::digits);
-static_assert(PER_THREAD_DEPENDENT_CONFIG < (1ULL << NetHandler::CONFIG_ITEM_COUNT));
-} // end anonymous namespace
-
 NetHandler::Config                                     NetHandler::global_config;
 std::bitset<std::numeric_limits<unsigned int>::digits> NetHandler::active_thread_types;
-const std::bitset<NetHandler::CONFIG_ITEM_COUNT> NetHandler::config_value_affects_per_thread_value{PER_THREAD_DEPENDENT_CONFIG};
+/// The values @c NetHandler::configure_per_thread_values reads.
+const std::bitset<NetHandler::CONFIG_ITEM_COUNT> NetHandler::config_value_affects_per_thread_value{
+  (1ULL << static_cast<unsigned>(NetHandler::Config::Index::MAX_CONNECTIONS_IN)) |
+  (1ULL << static_cast<unsigned>(NetHandler::Config::Index::MAX_REQUESTS_IN))};
 
 namespace
 {

src/iocore/net/unit_tests/test_NetHandler.cc

@@ -0,0 +1,60 @@
+/** @file
+
+  Catch based unit tests for NetHandler
+
+  @section license License
+
+  Licensed to the Apache Software Foundation (ASF) under one
+  or more contributor license agreements.  See the NOTICE file
+  distributed with this work for additional information
+  regarding copyright ownership.  The ASF licenses this file
+  to you under the Apache License, Version 2.0 (the
+  "License"); you may not use this file except in compliance
+  with the License.  You may obtain a copy of the License at
+
+      http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+ */
+
+#include <catch2/catch_test_macros.hpp>
+
+#include <cstddef>
+#include <cstdint>
+#include <set>
+
+#include "iocore/net/NetHandler.h"
+
+// The switch in Config::operator[] is checked for exhaustiveness by the
+// compiler, but not for correctness: a case returning the wrong member still
+// builds. That would make a record update write to the wrong config value.
+TEST_CASE("Config indexing reaches the intended member", "[net][nethandler]")
+{
+  using Index = NetHandler::Config::Index;
+  NetHandler::Config config;
+
+  config[Index::MAX_CONNECTIONS_IN]         = 11;
+  config[Index::MAX_REQUESTS_IN]            = 22;
+  config[Index::DEFAULT_INACTIVITY_TIMEOUT] = 33;
+
+  CHECK(config.max_connections_in == 11);
+  CHECK(config.max_requests_in == 22);
+  CHECK(config.default_inactivity_timeout == 33);
+}
+
+// Distinct addresses alone would not catch two cases returning each other's
+// member, but this covers any index added later without updating the test.
+TEST_CASE("Every Config index maps to a distinct member", "[net][nethandler]")
+{
+  NetHandler::Config         config;
+  std::set<uint32_t const *> members;
+
+  for (int i = 0; i < NetHandler::CONFIG_ITEM_COUNT; ++i) {
+    members.insert(&config[static_cast<NetHandler::Config::Index>(i)]);
+  }
+  CHECK(members.size() == static_cast<std::size_t>(NetHandler::CONFIG_ITEM_COUNT));
+}