Agent report error - #44
Merged
Merged
Conversation
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 <andrea.ricchi@amarulasolutions.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughConnMan agent error reporting now supports registered callbacks that return a retry decision. Agent methods always complete their D-Bus replies. Manager wiring forwards service errors, and integration tests validate retry and response behavior. ChangesConnMan agent reporting
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ConnMan
participant Agent
participant Manager
participant Service
participant OnReportErrorCallback
ConnMan->>Agent: ReportError(service, error)
Agent->>Manager: invoke registered report-error handler
Manager->>Service: locate service
Manager->>OnReportErrorCallback: invoke service and error
OnReportErrorCallback-->>Manager: return retry decision
Manager-->>Agent: return retry decision
Agent-->>ConnMan: retry D-Bus error or empty response
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
🧩 Build Artifacts ✅ The following build artifacts were produced: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/gconnman_agent_test.cpp (1)
107-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate the forwarded
Serviceargument.The callback discards
service. Assertservice->objPath()equalsservice_path. This test then detects a regression in the ReportError service lookup.Proposed test update
std::string reported_error; +std::string reported_service_path; manager->onReportError( - [&reported_error](const auto& /*service*/, const std::string& error) { + [&reported_error, &reported_service_path]( + const auto& service, const std::string& error) { + reported_service_path = service->objPath(); reported_error = error; return true; }); ... EXPECT_EQ(reported_error, "invalid-key"); +EXPECT_EQ(reported_service_path, service_path);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/gconnman_agent_test.cpp` around lines 107 - 112, Update the onReportError callback setup in gconnman_agent_test.cpp so the lambda validates the forwarded Service argument instead of discarding it; use the existing service_path expectation and assert that service->objPath() matches it before capturing the error string. Keep the reported_error assignment intact so the test still verifies the error text while also catching regressions in the ReportError service lookup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/gconnman_agent_test.cpp`:
- Around line 107-112: Update the onReportError callback setup in
gconnman_agent_test.cpp so the lambda validates the forwarded Service argument
instead of discarding it; use the existing service_path expectation and assert
that service->objPath() matches it before capturing the error string. Keep the
reported_error assignment intact so the test still verifies the error text while
also catching regressions in the ReportError service lookup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0235d0db-a1cf-431d-bba1-813994fd8354
📒 Files selected for processing (6)
include/amarula/dbus/connman/gagent.hppinclude/amarula/dbus/connman/gmanager.hppsrc/dbus/gconnman_agent.cppsrc/dbus/gconnman_manager.cpptests/CMakeLists.txttests/gconnman_agent_test.cpp
AndreaRicchi
force-pushed
the
agent-report-error
branch
from
August 5, 2026 13:17
eeb55b7 to
9b01398
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
🧩 Build Artifacts ✅ The following build artifacts were produced: |
EddyTheCo
requested changes
Aug 5, 2026
EddyTheCo
left a comment
EddyTheCo
left a comment
Contributor
There was a problem hiding this comment.
The invocation release for other methods was missing. Thanks.
In general, I like the changes; just please double-check my comments.
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 <andrea.ricchi@amarulasolutions.com>
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 <andrea.ricchi@amarulasolutions.com>
AndreaRicchi
force-pushed
the
agent-report-error
branch
from
August 5, 2026 14:15
9b01398 to
7833015
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/gconnman_agent_test.cpp (1)
112-118: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the callback false-result branch.
The tests cover no handler and a handler that returns
true. They do not cover a handler that returnsfalse. A change that retries wheneverreport_error_cb_exists would pass both current tests.Add a test that registers a callback returning
false, then assert a normalReportErrorreply and the callback payload.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/gconnman_agent_test.cpp` around lines 112 - 118, Add a test in the existing gconnman agent test flow that registers an onReportError callback returning false, invokes the normal ReportError request, and asserts both the successful reply and the callback’s reported service path and error payload. Keep the existing no-handler and true-result coverage unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/gconnman_agent_test.cpp`:
- Around line 112-118: Add a test in the existing gconnman agent test flow that
registers an onReportError callback returning false, invokes the normal
ReportError request, and asserts both the successful reply and the callback’s
reported service path and error payload. Keep the existing no-handler and
true-result coverage unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 85987d38-1099-426b-a1d2-d3f5ea85c398
📒 Files selected for processing (6)
include/amarula/dbus/connman/gagent.hppinclude/amarula/dbus/connman/gmanager.hppsrc/dbus/gconnman_agent.cppsrc/dbus/gconnman_manager.cpptests/CMakeLists.txttests/gconnman_agent_test.cpp
🚧 Files skipped from review as they are similar to previous changes (5)
- src/dbus/gconnman_manager.cpp
- include/amarula/dbus/connman/gmanager.hpp
- tests/CMakeLists.txt
- src/dbus/gconnman_agent.cpp
- include/amarula/dbus/connman/gagent.hpp
|
🧩 Build Artifacts ✅ The following build artifacts were produced: |
EddyTheCo
reviewed
Aug 5, 2026
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 <andrea.ricchi@amarulasolutions.com>
AndreaRicchi
force-pushed
the
agent-report-error
branch
from
August 5, 2026 14:49
7833015 to
7821b1d
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
🧩 Build Artifacts ✅ The following build artifacts were produced: |
EddyTheCo
approved these changes
Aug 5, 2026
EddyTheCo
left a comment
EddyTheCo
left a comment
Contributor
There was a problem hiding this comment.
Very nice. Thanks.
After these changes, there is no need to call remove when the service is in invalid-key, connman calls request input again.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests