fix: fix fd marshaling in QtDBus#1716
Conversation
Call Ping() to trigger QtDBus's file descriptor passing capability update. Signed-off-by: reddevillg <reddevillg@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dengbo11 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Cli::getPkgMan method to consolidate the D-Bus peer connection setup and PackageManager initialization, reducing duplication. It also adds a check in Cli::installFromFile to ensure the D-Bus connection supports Unix File Descriptor passing before proceeding. The review feedback suggests two improvements: first, avoiding a redundant and inefficient initialization of pkgManConn by using its default constructor; second, generalizing the error message when Unix FD passing is unsupported, as the connection may be via the system bus rather than a peer connection.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| } | ||
|
|
||
| QString service; | ||
| QDBusConnection pkgManConn{ "ll-package-manager" }; |
There was a problem hiding this comment.
Initializing pkgManConn with "ll-package-manager" is redundant and inefficient because it is immediately overwritten in both branches of the if-else statement (either by QDBusConnection::connectToPeer or QDBusConnection::systemBus). Additionally, constructing a QDBusConnection with a name that has not been registered yet triggers an unnecessary connection lookup in the Qt D-Bus registry.\n\nUsing the default constructor QDBusConnection pkgManConn; is cleaner, more efficient, and avoids any confusion.
QDBusConnection pkgManConn;|
|
||
| auto caps = (*pkgMan)->connection().connectionCapabilities(); | ||
| if (!(caps & QDBusConnection::UnixFileDescriptorPassing)) { | ||
| this->printer.printErr(LINGLONG_ERRV("peer connection does not support Unix FD passing")); |
There was a problem hiding this comment.
The error message refers specifically to a "peer connection", but this function can also be called when connected via the system bus (when peerMode is false). To avoid confusion, it is better to use a more general error message such as "D-Bus connection does not support Unix FD passing".
this->printer.printErr(LINGLONG_ERRV("D-Bus connection does not support Unix FD passing"));
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
3 participants
Call Ping() to trigger QtDBus's file descriptor passing capability update.