From 5466abe2b6c48c3a6b67f71c73b4527072133175 Mon Sep 17 00:00:00 2001 From: Andrea Ricchi Date: Tue, 28 Jul 2026 10:04:12 +0200 Subject: [PATCH 1/4] gagent: Always complete Cancel and Release invocations The Cancel and Release branches of dispatch_method_call() returned without ever calling g_dbus_method_invocation_return_value(), and were taken only when a callback had been installed. In both cases the D-Bus method invocation was left pending: connman gets an answer only once its own reply timeout expires, and the GDBusMethodInvocation is leaked. Split the callback check from the method match so the invocation is always completed, with or without a registered handler. Signed-off-by: Andrea Ricchi --- src/dbus/gconnman_agent.cpp | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/dbus/gconnman_agent.cpp b/src/dbus/gconnman_agent.cpp index 7d36400..f220e98 100644 --- a/src/dbus/gconnman_agent.cpp +++ b/src/dbus/gconnman_agent.cpp @@ -172,13 +172,19 @@ 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; } From 25a5860771fba14bca92c65fb4ba2796edd04d07 Mon Sep 17 00:00:00 2001 From: Andrea Ricchi Date: Tue, 28 Jul 2026 10:04:26 +0200 Subject: [PATCH 2/4] gagent: Answer ReportError and support connman retry ReportError parsed its arguments, logged them and returned without ever completing the invocation. connman holds the pending Service.Connect() reply until the agent answers, so a failed connection attempt surfaced to the caller only when its own D-Bus reply timeout expired. Always complete the invocation, and let the handler ask for a retry: returning true replies net.connman.Agent.Error.Retry, which makes connman reconnect with the credentials it already has instead of requesting them again. ReportErrorCallback therefore returns bool, and gets the setter it was missing. The GVariant strings are copied into std::string before the children are unref'd, since the borrowed pointers do not outlive them once the callback is invoked after the unref. The callback runs on the D-Bus dispatch thread while connman waits, so it must not block; exceptions escaping it are caught so the invocation is still answered. Signed-off-by: Andrea Ricchi --- include/amarula/dbus/connman/gagent.hpp | 18 ++++++++++++- src/dbus/gconnman_agent.cpp | 34 ++++++++++++++++++++----- 2 files changed, 44 insertions(+), 8 deletions(-) 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/src/dbus/gconnman_agent.cpp b/src/dbus/gconnman_agent.cpp index f220e98..aaa1513 100644 --- a/src/dbus/gconnman_agent.cpp +++ b/src/dbus/gconnman_agent.cpp @@ -189,20 +189,40 @@ void Agent::dispatch_method_call(GDBusMethodInvocation *invocation, } 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; } From 8f4beb7dd54b9499a7440263c2c4312a296b58da Mon Sep 17 00:00:00 2001 From: Andrea Ricchi Date: Tue, 28 Jul 2026 10:04:35 +0200 Subject: [PATCH 3/4] gconnman_manager: Add onReportError callback Expose the agent's ReportError to consumers of Manager, mapping the connman object path to the matching Service and forwarding the connman error string ("invalid-key", "connect-failed", ...). Returning true from the callback asks connman to retry with the credentials it already has. The service lookup and the callback copy are taken under mtx_, and the callback is invoked with the lock released, like the other changed callbacks: it runs on the D-Bus dispatch thread and typically wants to call back into Manager, which locks the same non-recursive mutex. Signed-off-by: Andrea Ricchi --- include/amarula/dbus/connman/gmanager.hpp | 21 +++++++++++++++++++++ src/dbus/gconnman_manager.cpp | 23 +++++++++++++++++++++++ 2 files changed, 44 insertions(+) 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_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, From 7821b1d4662a1eefca2d60a8066f9698474ac1b6 Mon Sep 17 00:00:00 2001 From: Andrea Ricchi Date: Tue, 28 Jul 2026 10:04:35 +0200 Subject: [PATCH 4/4] gconnman_agent_test.cpp: Add agent method reply tests Cover that the agent answers Cancel and ReportError, and that a handler returning true makes the agent reply net.connman.Agent.Error.Retry. The retry test needs a service the agent knows about, since a retry is asked only for a path present in the Manager service list, so it waits for the ServicesChanged callback instead of reading that list right after construction, where the asynchronous GetServices() has not answered yet. The wait happens on the test thread: the callback runs on the same GLib thread that dispatches the agent method calls, so a synchronous call issued from there would never be answered. The calls use a short reply timeout on purpose: an agent that never completes the invocation is otherwise indistinguishable from a slow one until the D-Bus default 25s timeout expires. Like the other connman tests these need a running connmand and skip when it is absent. Signed-off-by: Andrea Ricchi --- tests/CMakeLists.txt | 2 +- tests/gconnman_agent_test.cpp | 212 ++++++++++++++++++++++++++++++++++ 2 files changed, 213 insertions(+), 1 deletion(-) create mode 100644 tests/gconnman_agent_test.cpp 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); +}