Agent report error

include/amarula/dbus/connman/gagent.hpp

@@ -22,8 +22,19 @@ class Agent {
         std::function<GVariant *(const gchar *service, GVariant *fields)>;
     using CancelCallback = std::function<void()>;
     using ReleaseCallback = std::function<void()>;
+    /*
+     * 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<void(const gchar *service, const gchar *error)>;
+        std::function<bool(const gchar *service, const gchar *error)>;
 
     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,

include/amarula/dbus/connman/gmanager.hpp

@@ -75,6 +75,21 @@ class Manager : public DBusProxy<ManaProperties> {
     using OnRequestInputWISPrEnabledCallback =
         std::function<std::pair<std::string, std::string>(
             std::shared_ptr<Service>)>;
+    /*
+     * 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<bool(std::shared_ptr<Service>, const std::string& error)>;
     using OnTechnologiesChangedCallback =
         std::function<void(const Manager::ProxyList<Technology>&)>;
     using OnServicesChangedCallback =
@@ -113,6 +128,11 @@ class Manager : public DBusProxy<ManaProperties> {
         request_input_wispr_enabled_cb_ = std::move(callback);
     }
 
+    void onReportError(OnReportErrorCallback callback) {
+        std::lock_guard<std::mutex> 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<ManaProperties> {
         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<Technology>&) {}};
     OnServicesChangedCallback services_changed_cb_{

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;
     }
 

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<Service> found_service;
+            OnReportErrorCallback callback;
+            {
+                std::lock_guard<std::mutex> 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,

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)

tests/gconnman_agent_test.cpp

@@ -0,0 +1,212 @@
+#include <gtest/gtest.h>
+
+#include <amarula/dbus/connman/gconnman.hpp>
+#include <amarula/dbus/connman/gservice.hpp>
+#include <chrono>
+#include <condition_variable>
+#include <mutex>
+#include <string>
+
+#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<std::mutex> 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<std::mutex> 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<std::mutex> 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<std::mutex> 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);
+}