This is an automated email from the ASF dual-hosted git repository.
brbzull0 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new ebf8a0d6ce Index NetHandler config by named enum (#13543)
ebf8a0d6ce is described below
commit ebf8a0d6ce1f7e28d2af5a8751acd38ab175bdbc
Author: Damian Meden <[email protected]>
AuthorDate: Tue Aug 18 10:16:50 2026 +0200
Index NetHandler config by named enum (#13543)
Config was addressed as an array by advancing a pointer from its first
member, and the update handler recovered the index by subtracting
pointers to distinct members. Both are undefined behavior regardless of
layout. Name the values instead so indexing is well defined, which also
removes the layout assertions and the magic per-thread mask.
The index also arrives from an untyped event cookie, so give the enum a
fixed underlying type: converting an out of range integer to an
enumeration without one is undefined, which would defeat the check in
operator[] before it could run. Scoping the enum keeps an int from
silently becoming an index again.
The compiler checks the new switch for exhaustiveness but not for
correctness, so add a test that every index reaches a distinct member.
A case returning the wrong value would otherwise build cleanly and make
a record update write to the wrong setting.
---
include/iocore/net/NetHandler.h | 49 ++++++++++++++---------
src/iocore/net/CMakeLists.txt | 1 +
src/iocore/net/NetHandler.cc | 27 ++++++++-----
src/iocore/net/UnixNet.cc | 16 ++------
src/iocore/net/unit_tests/test_NetHandler.cc | 60 ++++++++++++++++++++++++++++
5 files changed, 111 insertions(+), 42 deletions(-)
diff --git a/include/iocore/net/NetHandler.h b/include/iocore/net/NetHandler.h
index c55e18be00..992155250e 100644
--- a/include/iocore/net/NetHandler.h
+++ b/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 @@ public:
/// 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 @@ public:
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;
diff --git a/src/iocore/net/CMakeLists.txt b/src/iocore/net/CMakeLists.txt
index b317e26b3e..68fb031ad7 100644
--- a/src/iocore/net/CMakeLists.txt
+++ b/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
diff --git a/src/iocore/net/NetHandler.cc b/src/iocore/net/NetHandler.cc
index 7994bac83a..9a6f8cc298 100644
--- a/src/iocore/net/NetHandler.cc
+++ b/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;
diff --git a/src/iocore/net/UnixNet.cc b/src/iocore/net/UnixNet.cc
index 7c90eeb3d7..b21f21fdce 100644
--- a/src/iocore/net/UnixNet.cc
+++ b/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
{
diff --git a/src/iocore/net/unit_tests/test_NetHandler.cc
b/src/iocore/net/unit_tests/test_NetHandler.cc
new file mode 100644
index 0000000000..fedd16fce0
--- /dev/null
+++ b/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));
+}
