diff --git a/include/iocore/net/NetHandler.h b/include/iocore/net/NetHandler.h index c55e18be003..992155250e5 100644 --- a/include/iocore/net/NetHandler.h +++ b/include/iocore/net/NetHandler.h @@ -24,8 +24,11 @@ #pragma once #include -#include -#include +#include +#include +#include + +#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); // 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(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_value_affects_per_thread_value; diff --git a/src/iocore/net/CMakeLists.txt b/src/iocore/net/CMakeLists.txt index b317e26b3ea..68fb031ad77 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 7994bac83a7..b2d5cb0be42 100644 --- a/src/iocore/net/NetHandler.cc +++ b/src/iocore/net/NetHandler.cc @@ -30,6 +30,7 @@ #endif #include +#include 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 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 so it can be passed as an event cookie. - void *idx = reinterpret_cast(static_cast(updated_member - &global_config[0])); + void *idx = reinterpret_cast(static_cast(*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,10 @@ int NetHandler::mainNetEvent(int event, Event *e) { if (TS_EVENT_MGMT_UPDATE == event) { - intptr_t idx = reinterpret_cast(e->cookie); - // Copy to the same offset in the instance struct. + auto idx = static_cast(reinterpret_cast(e->cookie)); + // 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(idx)]) { this->configure_per_thread_values(); } return EVENT_CONT; diff --git a/src/iocore/net/UnixNet.cc b/src/iocore/net/UnixNet.cc index 7c90eeb3d70..b21f21fdceb 100644 --- a/src/iocore/net/UnixNet.cc +++ b/src/iocore/net/UnixNet.cc @@ -41,20 +41,12 @@ std::atomic 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::digits); -static_assert(PER_THREAD_DEPENDENT_CONFIG < (1ULL << NetHandler::CONFIG_ITEM_COUNT)); -} // end anonymous namespace - NetHandler::Config NetHandler::global_config; std::bitset::digits> NetHandler::active_thread_types; -const std::bitset 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_value_affects_per_thread_value{ + (1ULL << static_cast(NetHandler::Config::Index::MAX_CONNECTIONS_IN)) | + (1ULL << static_cast(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 00000000000..0813d3f9c72 --- /dev/null +++ b/src/iocore/net/unit_tests/test_NetHandler.cc @@ -0,0 +1,44 @@ +/** @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 + +#include +#include +#include + +#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("Every Config index maps to a distinct member", "[net][nethandler]") +{ + NetHandler::Config config; + std::set members; + + for (int i = 0; i < NetHandler::CONFIG_ITEM_COUNT; ++i) { + members.insert(&config[static_cast(i)]); + } + CHECK(members.size() == static_cast(NetHandler::CONFIG_ITEM_COUNT)); +}