diff --git a/include/amarula/dbus/connman/gagent.hpp b/include/amarula/dbus/connman/gagent.hpp index 9ce0707..5a461e1 100644 --- a/include/amarula/dbus/connman/gagent.hpp +++ b/include/amarula/dbus/connman/gagent.hpp @@ -22,8 +22,19 @@ class Agent { std::function; using CancelCallback = std::function; using ReleaseCallback = std::function; + /* + * Returning true asks connman to retry the connection with the credentials + * it already has, by replying net.connman.Agent.Error.Retry. The callback + * runs on the D-Bus dispatch thread while connman waits for the reply, so + * it must not block. + * + * The retry budget belongs to this callback: connman reconnects + * immediately, with no backoff and no attempt limit of its own, so a + * callback that always returns true against a service that keeps failing + * retries forever. Count the attempts and return false once you give up. + */ using ReportErrorCallback = - std::function; + std::function; void set_request_input_handler(RequestInputCallback callback) { request_input_cb_ = std::move(callback); @@ -37,9 +48,14 @@ class Agent { release_cb_ = std::move(callback); } + void set_report_error_handler(ReportErrorCallback callback) { + report_error_cb_ = std::move(callback); + } + RequestInputCallback request_input_cb_; CancelCallback cancel_cb_; ReleaseCallback release_cb_; + ReportErrorCallback report_error_cb_; static void on_method_call(GDBusConnection *connection, const gchar *sender, const gchar *object_path, diff --git a/include/amarula/dbus/connman/gmanager.hpp b/include/amarula/dbus/connman/gmanager.hpp index 8db7c4f..b0c4a98 100644 --- a/include/amarula/dbus/connman/gmanager.hpp +++ b/include/amarula/dbus/connman/gmanager.hpp @@ -75,6 +75,21 @@ class Manager : public DBusProxy { using OnRequestInputWISPrEnabledCallback = std::function( std::shared_ptr)>; + /* + * Called when connman reports that a connection attempt failed, with the + * connman error string ("invalid-key", "connect-failed", ...). Return true + * to have connman retry with the credentials it already has. + * + * connman holds the pending Service.Connect() reply until this returns, so + * the callback runs on the D-Bus dispatch thread and must not block. + * + * The retry budget belongs to this callback: connman reconnects + * immediately, with no backoff and no attempt limit of its own, so a + * callback that always returns true against a service that keeps failing + * retries forever. Count the attempts and return false once you give up. + */ + using OnReportErrorCallback = + std::function, const std::string& error)>; using OnTechnologiesChangedCallback = std::function&)>; using OnServicesChangedCallback = @@ -113,6 +128,11 @@ class Manager : public DBusProxy { request_input_wispr_enabled_cb_ = std::move(callback); } + void onReportError(OnReportErrorCallback callback) { + std::lock_guard const lock(mtx_); + report_error_cb_ = std::move(callback); + } + void registerAgent(const std::string& object_path, PropertiesSetCallback callback = nullptr); void unregisterAgent(const std::string& object_path, @@ -151,6 +171,7 @@ class Manager : public DBusProxy { request_input_hidden_network_name_cb_; OnRequestInputWPAEnterpriseCallback request_input_wpa_enterprise_cb_; OnRequestInputWISPrEnabledCallback request_input_wispr_enabled_cb_; + OnReportErrorCallback report_error_cb_; OnTechnologiesChangedCallback technologies_changed_cb_{ [](const Manager::ProxyList&) {}}; OnServicesChangedCallback services_changed_cb_{ diff --git a/src/dbus/gconnman_agent.cpp b/src/dbus/gconnman_agent.cpp index 7d36400..aaa1513 100644 --- a/src/dbus/gconnman_agent.cpp +++ b/src/dbus/gconnman_agent.cpp @@ -172,31 +172,57 @@ void Agent::dispatch_method_call(GDBusMethodInvocation *invocation, return; } - if (g_strcmp0(method_name, "Cancel") == 0 && cancel_cb_) { - cancel_cb_(); + if (g_strcmp0(method_name, "Cancel") == 0) { + if (cancel_cb_) { + cancel_cb_(); + } + g_dbus_method_invocation_return_value(invocation, nullptr); return; } - if (g_strcmp0(method_name, "Release") == 0 && release_cb_) { - release_cb_(); + if (g_strcmp0(method_name, "Release") == 0) { + if (release_cb_) { + release_cb_(); + } + g_dbus_method_invocation_return_value(invocation, nullptr); return; } if (g_strcmp0(method_name, "ReportError") == 0) { - const gchar *service = nullptr; - const gchar *error_str = nullptr; - GVariant *child_service = g_variant_get_child_value(parameters, 0); GVariant *child_error = g_variant_get_child_value(parameters, 1); - service = g_variant_get_string(child_service, nullptr); - error_str = g_variant_get_string(child_error, nullptr); - - LCM_LOG("ReportError:" << service << " " << error_str << '\n'); + std::string const service(g_variant_get_string(child_service, nullptr)); + std::string const error_str(g_variant_get_string(child_error, nullptr)); g_variant_unref(child_service); g_variant_unref(child_error); + LCM_LOG("ReportError:" << service << " " << error_str << '\n'); + + /* + * connman holds the pending Service.Connect() reply until this call is + * answered, so the invocation must always be completed - otherwise the + * caller only gets an answer when its own D-Bus reply timeout expires. + * Replying net.connman.Agent.Error.Retry makes connman reconnect using + * the credentials it already has, without asking for them again. + */ + bool retry = false; + if (report_error_cb_) { + try { + retry = report_error_cb_(service.c_str(), error_str.c_str()); + } catch (...) { + LCM_LOG("Exception in ReportError callback"); + } + } + + if (retry) { + g_dbus_method_invocation_return_dbus_error( + invocation, "net.connman.Agent.Error.Retry", "Retry"); + } else { + g_dbus_method_invocation_return_value(invocation, nullptr); + } + return; } diff --git a/src/dbus/gconnman_manager.cpp b/src/dbus/gconnman_manager.cpp index 4a37ac7..0a565e1 100644 --- a/src/dbus/gconnman_manager.cpp +++ b/src/dbus/gconnman_manager.cpp @@ -198,6 +198,29 @@ void Manager::setup_agent() { } return g_variant_builder_end(&builder); }); + + agent_->set_report_error_handler( + [this](const gchar* service_path, const gchar* error) -> bool { + std::shared_ptr found_service; + OnReportErrorCallback callback; + { + std::lock_guard const lock(mtx_); + auto service_it = std::ranges::find_if( + services_, [&service_path](const auto& service) { + return service->objPath() == service_path; + }); + if (service_it != services_.end()) { + found_service = *service_it; + } + callback = report_error_cb_; + } + + if (!found_service || callback == nullptr) { + return false; + } + + return callback(found_service, error); + }); } void Manager::setOfflineMode(bool offline_mode, diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 4d7a8dc..2b226cb 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -19,7 +19,7 @@ install( if(BUILD_CONNMAN) foreach(connman_test gconnman_clock_test gconnman_tech_test - gconnman_serv_test) + gconnman_serv_test gconnman_agent_test) add_executable(${connman_test} ${connman_test}.cpp thread_bundle.hpp logging_init.cpp) target_link_libraries(${connman_test} PRIVATE GConnmanDbus gtest_main) diff --git a/tests/gconnman_agent_test.cpp b/tests/gconnman_agent_test.cpp new file mode 100644 index 0000000..3fd89db --- /dev/null +++ b/tests/gconnman_agent_test.cpp @@ -0,0 +1,212 @@ +#include + +#include +#include +#include +#include +#include +#include + +#include "thread_bundle.hpp" + +using Amarula::DBus::G::Connman::Connman; + +namespace { + +/* + * Short on purpose: connman answers a Service.Connect() call only once the + * agent has answered ReportError, so an agent that never completes the + * invocation is indistinguishable from a hang. These calls must come back + * well within the D-Bus default reply timeout of 25s. + */ +constexpr int CALL_TIMEOUT_MS = 2000; + +/* + * The service list is filled by the asynchronous GetServices() issued when the + * Manager is built, so a test that needs a service has to wait for the + * ServicesChanged callback instead of reading Manager::services() right away. + */ +constexpr int SERVICES_TIMEOUT_MS = 5000; + +constexpr const char* AGENT_INTERFACE = "net.connman.Agent"; +constexpr const char* RETRY_ERROR = "net.connman.Agent.Error.Retry"; + +auto call_agent(GDBusConnection* bus, const std::string& path, + const gchar* method, GVariant* args, GError** error) + -> GVariant* { + return g_dbus_connection_call_sync( + bus, g_dbus_connection_get_unique_name(bus), path.c_str(), + AGENT_INTERFACE, method, args, nullptr, G_DBUS_CALL_FLAGS_NONE, + CALL_TIMEOUT_MS, nullptr, error); +} + +/* + * The Agent constructor exports the agent object on our own bus connection + * when the Manager is built, so these tests call it directly and never go + * through connmand - Manager::registerAgent() is a separate step that only + * tells connmand which path to call. Like the other connman tests they still + * need a running connmand, because constructing a Manager does. Skip rather + * than fail so the suite stays usable on a host without it. + */ +auto connman_available(GDBusConnection* bus) -> bool { + GVariant* reply = g_dbus_connection_call_sync( + bus, "org.freedesktop.DBus", "/org/freedesktop/DBus", + "org.freedesktop.DBus", "NameHasOwner", + g_variant_new("(s)", "net.connman"), G_VARIANT_TYPE("(b)"), + G_DBUS_CALL_FLAGS_NONE, CALL_TIMEOUT_MS, nullptr, nullptr); + if (reply == nullptr) { + return false; + } + + gboolean has_owner = FALSE; + g_variant_get(reply, "(b)", &has_owner); + g_variant_unref(reply); + + return has_owner == TRUE; +} + +auto system_bus_or_skip() -> GDBusConnection* { + GDBusConnection* bus = g_bus_get_sync(G_BUS_TYPE_SYSTEM, nullptr, nullptr); + if (bus == nullptr) { + return nullptr; + } + return connman_available(bus) ? bus : nullptr; +} + +} // namespace + +TEST(ConnmanAgent, ReportErrorIsAnswered) { + GDBusConnection* bus = system_bus_or_skip(); + if (bus == nullptr) { + GTEST_SKIP() << "connmand not available on the system bus"; + } + + const ThreadBundle thread_bundle; + const Connman connman; + const auto manager = connman.manager(); + + GError* error = nullptr; + + // No handler installed: the agent must still answer, and must not ask + // connman to retry. + GVariant* reply = + call_agent(bus, manager->internalAgentPath(), "ReportError", + g_variant_new("(os)", "/net/connman/service/does_not_exist", + "invalid-key"), + &error); + + ASSERT_NE(reply, nullptr) + << "ReportError was not answered within " << CALL_TIMEOUT_MS + << "ms: " << (error != nullptr ? error->message : ""); + g_variant_unref(reply); +} + +TEST(ConnmanAgent, ReportErrorRequestsRetry) { + GDBusConnection* bus = system_bus_or_skip(); + if (bus == nullptr) { + GTEST_SKIP() << "connmand not available on the system bus"; + } + + /* + * Declared before the Connman instance so that they outlive the callbacks + * capturing them: ~Connman() waits for the pending asynchronous calls, + * whose callbacks run on the library GLib thread. + */ + std::mutex mtx; + std::condition_variable services_cv; + std::string discovered_service_path; + std::string reported_service_path; + std::string reported_error; + + const ThreadBundle thread_bundle; + const Connman connman; + const auto manager = connman.manager(); + + manager->onServicesChanged( + [&mtx, &services_cv, &discovered_service_path](const auto& services) { + { + std::lock_guard const lock(mtx); + if (discovered_service_path.empty() && !services.empty()) { + discovered_service_path = services.front()->objPath(); + } + } + services_cv.notify_all(); + }); + + /* + * The agent answers Retry only for a service it knows about, so wait for + * the Manager service list to be populated. The waiting has to happen here + * and not inside the callback: the callback runs on the GLib thread that + * also dispatches the agent method calls, so a synchronous call from there + * would never be answered. + */ + std::string service_path; + { + std::unique_lock lock(mtx); + services_cv.wait_for(lock, + std::chrono::milliseconds(SERVICES_TIMEOUT_MS), + [&discovered_service_path] { + return !discovered_service_path.empty(); + }); + service_path = discovered_service_path; + } + + // The callback is missed when GetServices() completes before it is + // installed, so fall back to what the Manager already holds. + if (service_path.empty()) { + const auto services = manager->services(); + if (!services.empty()) { + service_path = services.front()->objPath(); + } + } + + if (service_path.empty()) { + GTEST_SKIP() << "No connman services available"; + } + + manager->onReportError([&mtx, &reported_service_path, &reported_error]( + const auto& service, const std::string& error) { + std::lock_guard const lock(mtx); + reported_service_path = + service != nullptr ? service->objPath() : std::string(); + reported_error = error; + return true; + }); + + GError* error = nullptr; + GVariant* reply = call_agent( + bus, manager->internalAgentPath(), "ReportError", + g_variant_new("(os)", service_path.c_str(), "invalid-key"), &error); + + ASSERT_EQ(reply, nullptr) << "Expected " << RETRY_ERROR; + ASSERT_NE(error, nullptr); + + gchar* remote_error = g_dbus_error_get_remote_error(error); + EXPECT_STREQ(remote_error, RETRY_ERROR); + g_free(remote_error); + g_error_free(error); + + std::lock_guard const lock(mtx); + EXPECT_EQ(reported_service_path, service_path); + EXPECT_EQ(reported_error, "invalid-key"); +} + +TEST(ConnmanAgent, CancelIsAnswered) { + GDBusConnection* bus = system_bus_or_skip(); + if (bus == nullptr) { + GTEST_SKIP() << "connmand not available on the system bus"; + } + + const ThreadBundle thread_bundle; + const Connman connman; + const auto manager = connman.manager(); + + GError* error = nullptr; + GVariant* reply = call_agent(bus, manager->internalAgentPath(), "Cancel", + nullptr, &error); + + ASSERT_NE(reply, nullptr) + << "Cancel was not answered within " << CALL_TIMEOUT_MS + << "ms: " << (error != nullptr ? error->message : ""); + g_variant_unref(reply); +}