From d2395594be00204aaf09f135ee84f681e3ea4f3f Mon Sep 17 00:00:00 2001 From: Andrew Beltrano Date: Wed, 24 Jan 2024 22:19:51 +0000 Subject: [PATCH 1/6] Make Nl80211Interface suitable for use in unique collections. --- .../libnl-helpers/Netlink80211Interface.cxx | 6 +++++ .../netlink/nl80211/Netlink80211Interface.hxx | 25 +++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/src/linux/libnl-helpers/Netlink80211Interface.cxx b/src/linux/libnl-helpers/Netlink80211Interface.cxx index 25ee8176..d43710fd 100644 --- a/src/linux/libnl-helpers/Netlink80211Interface.cxx +++ b/src/linux/libnl-helpers/Netlink80211Interface.cxx @@ -160,3 +160,9 @@ Nl80211Interface::GetWiphy() const { return Nl80211Wiphy::FromIndex(WiphyIndex); } + +bool +Nl80211Interface::IsAccessPoint() const noexcept +{ + return (Type == nl80211_iftype::NL80211_IFTYPE_AP); +} diff --git a/src/linux/libnl-helpers/include/microsoft/net/netlink/nl80211/Netlink80211Interface.hxx b/src/linux/libnl-helpers/include/microsoft/net/netlink/nl80211/Netlink80211Interface.hxx index 4529d76b..de30760a 100644 --- a/src/linux/libnl-helpers/include/microsoft/net/netlink/nl80211/Netlink80211Interface.hxx +++ b/src/linux/libnl-helpers/include/microsoft/net/netlink/nl80211/Netlink80211Interface.hxx @@ -28,6 +28,9 @@ struct Nl80211Interface Nl80211Interface() = default; + auto + operator <=>(const Nl80211Interface&) const = default; + /** * @brief Construct a new Nl80211Interface object with the specified attributes. * @@ -73,8 +76,30 @@ struct Nl80211Interface */ std::string ToString() const; + + /** + * @brief Indicates if the interface is an access point. + * + * @return true + * @return false + */ + bool + IsAccessPoint() const noexcept; }; } // namespace Microsoft::Net::Netlink::Nl80211 +namespace std +{ +template <> +struct hash +{ + size_t + operator()(const Microsoft::Net::Netlink::Nl80211::Nl80211Interface& interface) const noexcept + { + return std::hash()(interface.Name); + } +}; +} // namespace std + #endif // NETLINK_82011_INTERFACE_HXX From 41a007e92a50fee600138ec1ab6ae0782a886cd5 Mon Sep 17 00:00:00 2001 From: Andrew Beltrano Date: Wed, 24 Jan 2024 22:20:05 +0000 Subject: [PATCH 2/6] Improve logging. --- .../wifi/apmanager/AccessPointDiscoveryAgent.cxx | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/common/wifi/apmanager/AccessPointDiscoveryAgent.cxx b/src/common/wifi/apmanager/AccessPointDiscoveryAgent.cxx index 4a69b98c..e602b231 100644 --- a/src/common/wifi/apmanager/AccessPointDiscoveryAgent.cxx +++ b/src/common/wifi/apmanager/AccessPointDiscoveryAgent.cxx @@ -1,4 +1,7 @@ +#include + +#include #include #include #include @@ -40,7 +43,7 @@ AccessPointDiscoveryAgent::DevicePresenceChanged(AccessPointPresenceEvent presen { std::shared_lock onDevicePresenceChangedLock{ m_onDevicePresenceChangedGate }; if (m_onDevicePresenceChanged) { - LOGD << "Access point discovery agent detected a device presence change"; + LOGI << std::format("Access Point Discovery Event: Interface {} {}", accessPoint->GetInterfaceName(), magic_enum::enum_name(presence)); m_onDevicePresenceChanged(presence, std::move(accessPoint)); } } @@ -56,11 +59,13 @@ AccessPointDiscoveryAgent::Start() { bool expected = false; if (m_started.compare_exchange_weak(expected, true)) { - LOGD << "Access point discovery agent starting"; + LOGI << "Access point discovery agent starting"; m_operations->Start([weakThis = std::weak_ptr(GetInstance())](auto&& presence, auto&& accessPoint) { // Attempt to promote the weak pointer to a shared pointer to ensure this instance is still valid. if (auto strongThis = weakThis.lock(); strongThis) { strongThis->DevicePresenceChanged(presence, std::move(accessPoint)); + } else { + LOGW << "Access point discovery agent instance no longer valid; ignoring presence change event"; } }); } @@ -71,7 +76,7 @@ AccessPointDiscoveryAgent::Stop() { bool expected = true; if (m_started.compare_exchange_weak(expected, false)) { - LOGD << "Access point discovery agent stopping"; + LOGI << "Access point discovery agent stopping"; m_operations->Stop(); } } From 3e8b2bb5349faa23c988d054fa02bb4e3175af7f Mon Sep 17 00:00:00 2001 From: Andrew Beltrano Date: Wed, 24 Jan 2024 22:20:37 +0000 Subject: [PATCH 3/6] Use unordered_set and eliminate use of utility struct. --- ...ssPointDiscoveryAgentOperationsNetlink.cxx | 54 +++++++++++-------- ...ssPointDiscoveryAgentOperationsNetlink.hxx | 10 +--- 2 files changed, 33 insertions(+), 31 deletions(-) diff --git a/src/linux/wifi/apmanager/AccessPointDiscoveryAgentOperationsNetlink.cxx b/src/linux/wifi/apmanager/AccessPointDiscoveryAgentOperationsNetlink.cxx index 01fdd384..3c558a97 100644 --- a/src/linux/wifi/apmanager/AccessPointDiscoveryAgentOperationsNetlink.cxx +++ b/src/linux/wifi/apmanager/AccessPointDiscoveryAgentOperationsNetlink.cxx @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -193,31 +194,36 @@ AccessPointDiscoveryAgentOperationsNetlink::ProcessNetlinkMessage(struct nl_msg return NL_SKIP; } - int interfaceIndex{ -1 }; - const char *interfaceName{ nullptr }; - nl80211_iftype interfaceType{ NL80211_IFTYPE_UNSPECIFIED }; - AccessPointPresenceEvent accessPointPresenceEvent; + if (genlMessageHeader->cmd != NL80211_CMD_NEW_INTERFACE && + genlMessageHeader->cmd != NL80211_CMD_DEL_INTERFACE && + genlMessageHeader->cmd != NL80211_CMD_SET_INTERFACE) { + LOGV << std::format("Ignoring {} nl80211 command message", nl80211CommandName); + } - LOG_VERBOSE << std::format("Received {} nl80211 command message", nl80211CommandName); + LOGD << std::format("Received {} nl80211 command message", nl80211CommandName); + + // Parse the message into an Nl80211Interface object. + auto nl80211InterfaceOpt = Nl80211Interface::Parse(netlinkMessage); + if (!nl80211InterfaceOpt.has_value()) { + LOGE << "Failed to parse nl80211 interface dump response"; + return NL_OK; + } + + AccessPointPresenceEvent accessPointPresenceEvent; + auto &nl80211Interface = nl80211InterfaceOpt.value(); switch (genlMessageHeader->cmd) { case NL80211_CMD_NEW_INTERFACE: case NL80211_CMD_DEL_INTERFACE: { - interfaceIndex = static_cast(nla_get_u32(netlinkMessageAttributes[NL80211_ATTR_IFINDEX])); - interfaceName = static_cast(nla_data(netlinkMessageAttributes[NL80211_ATTR_IFNAME])); - interfaceType = static_cast(nla_get_u32(netlinkMessageAttributes[NL80211_ATTR_IFTYPE])); - if (interfaceType != NL80211_IFTYPE_AP) { - LOG_VERBOSE << std::format("Ignoring interface presence change nl80211 message for non-ap wi-fi interface (type={})", Nl80211InterfaceTypeToString(interfaceType)); - return NL_SKIP; + if (!nl80211Interface.IsAccessPoint()) { + LOGV << std::format("Ignoring interface presence change nl80211 message for non-ap wi-fi interface (type={})", Nl80211InterfaceTypeToString(nl80211Interface.Type)); + return NL_OK; } accessPointPresenceEvent = (genlMessageHeader->cmd == NL80211_CMD_NEW_INTERFACE) ? AccessPointPresenceEvent::Arrived : AccessPointPresenceEvent::Departed; break; } case NL80211_CMD_SET_INTERFACE: { - interfaceIndex = static_cast(nla_get_u32(netlinkMessageAttributes[NL80211_ATTR_IFINDEX])); - interfaceName = static_cast(nla_data(netlinkMessageAttributes[NL80211_ATTR_IFNAME])); - interfaceType = static_cast(nla_get_u32(netlinkMessageAttributes[NL80211_ATTR_IFTYPE])); - accessPointPresenceEvent = (interfaceType == NL80211_IFTYPE_AP) ? AccessPointPresenceEvent::Arrived : AccessPointPresenceEvent::Departed; + accessPointPresenceEvent = nl80211Interface.IsAccessPoint() ? AccessPointPresenceEvent::Arrived : AccessPointPresenceEvent::Departed; break; } default: { @@ -226,24 +232,26 @@ AccessPointDiscoveryAgentOperationsNetlink::ProcessNetlinkMessage(struct nl_msg } } - // Update map tracking interface information. Handle the case where the - // interface is already present. This happens for NL80211_CMD_SET_INTERFACE - // when the interface type did not change. - auto interfaceInfo = m_interfaceInfo.find(interfaceIndex); - if (interfaceInfo == std::cend(m_interfaceInfo)) { + // Update interface seen cache, handling the case where the interface has already been seen. This happens for + // NL80211_CMD_SET_INTERFACE whern the interface type did not change. + auto interfaceSeenIterator = m_interfacesSeen.find(nl80211Interface); + if (interfaceSeenIterator == std::cend(m_interfacesSeen)) { if (accessPointPresenceEvent == AccessPointPresenceEvent::Arrived) { - m_interfaceInfo[interfaceIndex] = { interfaceName, interfaceType }; + m_interfacesSeen.insert(nl80211Interface); } } else { if (accessPointPresenceEvent == AccessPointPresenceEvent::Departed) { - m_interfaceInfo.erase(interfaceInfo); + m_interfacesSeen.erase(interfaceSeenIterator); } } // Invoke presence event callback if present. if (accessPointPresenceEventCallback != nullptr) { + const auto interfaceName{ nl80211Interface.Name }; LOGV << std::format("Invoking access point presence event callback with event args 'interface={}, presence={}'", interfaceName, magic_enum::enum_name(accessPointPresenceEvent)); - auto accessPoint = m_accessPointFactory->Create(interfaceName); + auto accessPointCreateArgs = std::make_unique(std::move(nl80211Interface)); + auto accessPoint = m_accessPointFactory->Create(interfaceName, std::move(accessPointCreateArgs)); + accessPointPresenceEventCallback(accessPointPresenceEvent, accessPoint); } diff --git a/src/linux/wifi/apmanager/include/microsoft/net/wifi/AccessPointDiscoveryAgentOperationsNetlink.hxx b/src/linux/wifi/apmanager/include/microsoft/net/wifi/AccessPointDiscoveryAgentOperationsNetlink.hxx index 704354e6..c912b003 100644 --- a/src/linux/wifi/apmanager/include/microsoft/net/wifi/AccessPointDiscoveryAgentOperationsNetlink.hxx +++ b/src/linux/wifi/apmanager/include/microsoft/net/wifi/AccessPointDiscoveryAgentOperationsNetlink.hxx @@ -7,7 +7,7 @@ #include #include #include -#include +#include #include #include @@ -120,13 +120,7 @@ private: int m_eventLoopStopFd{ -1 }; std::jthread m_netlinkMessageProcessingThread; - struct WifiInterfaceInfo - { - std::string Name; - nl80211_iftype Type; - }; - - std::unordered_map m_interfaceInfo; + std::unordered_set m_interfacesSeen; Microsoft::Net::Netlink::Nl80211::Nl80211ProtocolState &m_netlink80211ProtocolState; }; } // namespace Microsoft::Net::Wifi From 888c42f1b9ddae8e95af0d4fc5b34d9bf84de249 Mon Sep 17 00:00:00 2001 From: Andrew Beltrano Date: Thu, 25 Jan 2024 01:59:07 +0000 Subject: [PATCH 4/6] Disable file logging by default. Add cli option to enable. --- src/common/server/NetRemoteServerConfiguration.cxx | 5 +++++ .../net/remote/NetRemoteServerConfiguration.hxx | 5 +++++ src/linux/server/Main.cxx | 9 +++++---- 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/common/server/NetRemoteServerConfiguration.cxx b/src/common/server/NetRemoteServerConfiguration.cxx index 54332e36..ff6f1601 100644 --- a/src/common/server/NetRemoteServerConfiguration.cxx +++ b/src/common/server/NetRemoteServerConfiguration.cxx @@ -26,6 +26,11 @@ ConfigureCliAppOptions(CLI::App& app, NetRemoteServerConfiguration& config) config.LogVerbosity, "The log verbosity level. Supply multiple times to increase verbosity (0=warnings, errors, and fatal messages, 1=info messages, 2=debug messages, 3=verbose messages)"); + app.add_flag( + "--enable-file-logging", + config.EnableFileLogging, + "Enable logging to file (disabled by default)"); + return app; } diff --git a/src/common/server/include/microsoft/net/remote/NetRemoteServerConfiguration.hxx b/src/common/server/include/microsoft/net/remote/NetRemoteServerConfiguration.hxx index e6d90722..64fdc75f 100644 --- a/src/common/server/include/microsoft/net/remote/NetRemoteServerConfiguration.hxx +++ b/src/common/server/include/microsoft/net/remote/NetRemoteServerConfiguration.hxx @@ -64,6 +64,11 @@ struct NetRemoteServerConfiguration * and a level of 3 or above will show all verbose messages. */ uint32_t LogVerbosity{ 0 }; + + /** + * @brief Whether to enable logging to file or not. + */ + bool EnableFileLogging{ false }; /** * @brief Access point manager instance. diff --git a/src/linux/server/Main.cxx b/src/linux/server/Main.cxx index cefd44aa..23f2f193 100644 --- a/src/linux/server/Main.cxx +++ b/src/linux/server/Main.cxx @@ -41,10 +41,11 @@ main(int argc, char *argv[]) // Configure logging, appending all loggers to the default instance. plog::init(logSeverity, &colorConsoleAppender); - plog::init(logSeverity, &rollingFileAppender); - plog::init(logSeverity) - .addAppender(plog::get()) - .addAppender(plog::get()); + plog::init(logSeverity).addAppender(plog::get()); + if (configuration.EnableFileLogging) { + plog::init(logSeverity, &rollingFileAppender); + plog::init(logSeverity).addAppender(plog::get()); + } // Create an access point manager and discovery agent. { From e827ad1c3f62f91478655173eec8aea58b090b29 Mon Sep 17 00:00:00 2001 From: Andrew Beltrano Date: Thu, 25 Jan 2024 03:06:27 +0000 Subject: [PATCH 5/6] Ignore cmake test temporary directory. --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 15de8754..754a65b6 100644 --- a/.gitignore +++ b/.gitignore @@ -8,6 +8,7 @@ obj/ # CMake build/ out/ +Testing/ # vcpkg vcpkg_installed*/ From 74e216590cc1c219a3fc840ba023a25bebdfceff Mon Sep 17 00:00:00 2001 From: Andrew Beltrano Date: Thu, 25 Jan 2024 03:31:52 +0000 Subject: [PATCH 6/6] Annotate root tests and add cmake test preset to filter them out. --- CMakePresets.json | 19 ++++++------------- .../unit/linux/wpa-controller/TestHostapd.cxx | 16 ++++++++-------- .../wpa-controller/TestWpaController.cxx | 2 +- 3 files changed, 15 insertions(+), 22 deletions(-) diff --git a/CMakePresets.json b/CMakePresets.json index 06e3c8eb..b5295066 100644 --- a/CMakePresets.json +++ b/CMakePresets.json @@ -203,7 +203,8 @@ "name": "test-common", "hidden": true, "output": { - "outputOnFailure": true + "outputOnFailure": true, + "shortProgress": true } }, { @@ -214,24 +215,16 @@ ] }, { - "name": "local-base", - "hidden": true, - "description": "Run tests that don't require a remote connection", + "name": "non-root", + "configurePreset": "dev-linux", "inherits": [ "test-common" ], "filter": { - "include": { - "label": "[local]" + "exclude": { + "name": "(root)" } } - }, - { - "name": "local", - "configurePreset": "dev-linux", - "inherits": [ - "local-base" - ] } ] } \ No newline at end of file diff --git a/tests/unit/linux/wpa-controller/TestHostapd.cxx b/tests/unit/linux/wpa-controller/TestHostapd.cxx index d6382bc7..85b00b44 100644 --- a/tests/unit/linux/wpa-controller/TestHostapd.cxx +++ b/tests/unit/linux/wpa-controller/TestHostapd.cxx @@ -8,7 +8,7 @@ #include "detail/WpaDaemonManager.hxx" -TEST_CASE("Create a Hostapd instance", "[wpa][hostapd][client][remote]") +TEST_CASE("Create a Hostapd instance (root)", "[wpa][hostapd][client][remote]") { using namespace Wpa; @@ -40,7 +40,7 @@ TEST_CASE("Create a Hostapd instance", "[wpa][hostapd][client][remote]") } } -TEST_CASE("Send Ping() command", "[wpa][hostapd][client][remote]") +TEST_CASE("Send Ping() command (root)", "[wpa][hostapd][client][remote]") { using namespace Wpa; @@ -58,7 +58,7 @@ TEST_CASE("Send Ping() command", "[wpa][hostapd][client][remote]") } } -TEST_CASE("Send command: GetStatus()", "[wpa][hostapd][client][remote]") +TEST_CASE("Send command: GetStatus() (root)", "[wpa][hostapd][client][remote]") { using namespace Wpa; @@ -96,7 +96,7 @@ TEST_CASE("Send command: GetStatus()", "[wpa][hostapd][client][remote]") } } -TEST_CASE("Send GetProperty() command", "[wpa][hostapd][client][remote]") +TEST_CASE("Send GetProperty() command (root)", "[wpa][hostapd][client][remote]") { using namespace Wpa; @@ -135,7 +135,7 @@ TEST_CASE("Send GetProperty() command", "[wpa][hostapd][client][remote]") } } -TEST_CASE("Send SetProperty() command", "[wpa][hostapd][client][remote]") +TEST_CASE("Send SetProperty() command (root)", "[wpa][hostapd][client][remote]") { using namespace Wpa; @@ -159,7 +159,7 @@ TEST_CASE("Send SetProperty() command", "[wpa][hostapd][client][remote]") // TODO: validate that the property was actually set. Need to find a property whose value is retrievable. } -TEST_CASE("Send control commands: Enable(), Disable()", "[wpa][hostapd][client][remote]") +TEST_CASE("Send control commands: Enable(), Disable() (root)", "[wpa][hostapd][client][remote]") { using namespace Wpa; @@ -211,7 +211,7 @@ TEST_CASE("Send control commands: Enable(), Disable()", "[wpa][hostapd][client][ // Also, keep Terminate() test cases at end-of-file since Catch2 will run // tests in declaration order by default, which minimizes the possibility // of a test case being run after the daemon has been terminated. -TEST_CASE("Send command: Terminate() doesn't throw", "[wpa][hostapd][client][remote]") +TEST_CASE("Send command: Terminate() doesn't throw (root)", "[wpa][hostapd][client][remote]") { using namespace Wpa; @@ -219,7 +219,7 @@ TEST_CASE("Send command: Terminate() doesn't throw", "[wpa][hostapd][client][rem REQUIRE_NOTHROW(hostapd.Terminate()); } -TEST_CASE("Send command: Terminate() ping failure", "[wpa][hostapd][client][remote]") +TEST_CASE("Send command: Terminate() ping failure (root)", "[wpa][hostapd][client][remote]") { using namespace Wpa; using namespace std::chrono_literals; diff --git a/tests/unit/linux/wpa-controller/TestWpaController.cxx b/tests/unit/linux/wpa-controller/TestWpaController.cxx index c4d1cf46..23bcd233 100644 --- a/tests/unit/linux/wpa-controller/TestWpaController.cxx +++ b/tests/unit/linux/wpa-controller/TestWpaController.cxx @@ -15,7 +15,7 @@ constexpr auto WpaTypesSupported = { }; } // namespace TestDetail -TEST_CASE("Send/receive WpaController request/response", "[wpa][hostapd][client][remote]") +TEST_CASE("Send/receive WpaController request/response (root)", "[wpa][hostapd][client][remote]") { using namespace TestDetail; using namespace Wpa;