feat: Add function for sending fd#1561
Conversation
Summary of ChangesHello @ComixHe, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces core functionality for inter-process communication (IPC) by enabling the transfer of file descriptors (FDs) between processes using Unix domain sockets. This new capability, accompanied by a robust testing suite, enhances the system's ability to securely and efficiently share resources and data across process boundaries. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces new functionality for sending and receiving file descriptors over sockets, along with corresponding tests. The implementation is solid and covers important aspects like handling EINTR. The tests are also quite comprehensive, covering normal operation, edge cases, and even signal interruptions.
My review includes a few suggestions for improvement:
- Using thread-safe methods for error string retrieval in
socket.cpp. - Improving test robustness in
socket_test.cppby usingwaitpidinstead ofwait. - Using
sigactioninstead ofsignalfor better portability in tests.
Overall, this is a good addition to the common library.
| }(); | ||
|
|
||
| if (n < 0) { | ||
| return tl::make_unexpected(std::string{ "Failed to receive message" } + ::strerror(errno)); |
There was a problem hiding this comment.
The ::strerror function is not thread-safe, which can lead to race conditions and incorrect error messages in a multithreaded environment. A better, thread-safe alternative in C++ is to use std::system_category().message(errno), which requires including the <system_error> header. This also provides an opportunity to make the error message more readable.
| return tl::make_unexpected(std::string{ "Failed to receive message" } + ::strerror(errno)); | |
| return tl::make_unexpected("Failed to receive message: " + std::system_category().message(errno)); |
| }(); | ||
|
|
||
| if (n < 0) { | ||
| return tl::make_unexpected("sendmsg failed: " + std::string(strerror(errno))); |
There was a problem hiding this comment.
As in recvFdWithPayload, strerror is not thread-safe. It's recommended to use a thread-safe alternative like std::system_category().message(errno) from the <system_error> header.
| return tl::make_unexpected("sendmsg failed: " + std::string(strerror(errno))); | |
| return tl::make_unexpected("sendmsg failed: " + std::system_category().message(errno)); |
| close(sv[0]); | ||
| auto res = recvFdWithPayload(sv[1], 1024); | ||
| EXPECT_FALSE(res.has_value()); | ||
| wait(nullptr); |
There was a problem hiding this comment.
Using wait(nullptr) waits for any child process to terminate, which can be imprecise. It's better practice to use waitpid() with the specific child's process ID. This allows you to verify that the correct child process is being waited for and to check its exit status, making the test more robust. This advice applies to other tests in this file that use fork() and wait(nullptr).
|
|
||
| TEST_F(SocketFdTest, SignalInterruptionRecovery) | ||
| { | ||
| auto handler = signal(SIGUSR1, [](int) { }); |
There was a problem hiding this comment.
The signal() function has implementation-defined semantics across different systems, which can make its behavior non-portable. For reliable and portable signal handling, it is strongly recommended to use sigaction() instead.
There was a problem hiding this comment.
Pull request overview
This pull request adds functionality for sending and receiving file descriptors over Unix domain sockets using the SCM_RIGHTS mechanism. The implementation provides two main functions: sendFdWithPayload for sending a file descriptor with an associated payload string, and recvFdWithPayload for receiving them.
Changes:
- Added
socket.handsocket.cppto implement file descriptor passing over Unix sockets - Added comprehensive test suite with 8 test cases covering normal operation, error conditions, edge cases, and signal handling
- Updated CMake build files to include the new source and test files
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| libs/common/src/linglong/common/socket.h | Defines the public API for socket fd passing with SocketData struct and two main functions |
| libs/common/src/linglong/common/socket.cpp | Implements fd passing using sendmsg/recvmsg with ancillary data and EINTR handling |
| libs/linglong/tests/ll-tests/src/linglong/common/socket_test.cpp | Comprehensive test suite covering cross-process transfer, error handling, truncation, signals, and edge cases |
| libs/common/CMakeLists.txt | Adds new socket source files to the build |
| libs/linglong/tests/ll-tests/CMakeLists.txt | Adds new socket test file to the test build |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| tl::expected<SocketData, std::string> recvFdWithPayload(int fd, std::size_t bufSize = 4096); | ||
|
|
There was a problem hiding this comment.
The public API functions recvFdWithPayload and sendFdWithPayload lack documentation. Add docstrings explaining their purpose, parameters, return values, and error conditions. This is especially important for API functions that will be used by other components.
| tl::expected<SocketData, std::string> recvFdWithPayload(int fd, std::size_t bufSize = 4096); | |
| /** | |
| * @brief Receives a file descriptor and an optional payload from a socket. | |
| * | |
| * This function expects the peer to send one file descriptor along with an | |
| * associated payload (typically a small control message) over a socket | |
| * that supports passing file descriptors (e.g. a UNIX domain socket). | |
| * | |
| * @param fd | |
| * A valid, connected socket file descriptor to receive from. | |
| * @param bufSize | |
| * Maximum number of bytes to read for the payload. If the incoming | |
| * payload is larger than this value, the behavior is implementation | |
| * defined (e.g. it may be truncated or treated as an error). The | |
| * default buffer size is 4096 bytes. | |
| * | |
| * @return tl::expected<SocketData, std::string> | |
| * - On success, contains a SocketData instance with the received file | |
| * descriptor and payload string. The caller is responsible for | |
| * managing (and eventually closing) the received file descriptor. | |
| * - On failure, contains a human-readable error message describing the | |
| * problem (for example, an invalid socket, protocol error, or system | |
| * call failure). | |
| */ | |
| tl::expected<SocketData, std::string> recvFdWithPayload(int fd, std::size_t bufSize = 4096); | |
| /** | |
| * @brief Sends a file descriptor and an associated payload over a socket. | |
| * | |
| * This function sends the given file descriptor together with the provided | |
| * payload (typically a small control message) over a socket that supports | |
| * passing file descriptors (e.g. a UNIX domain socket). | |
| * | |
| * @param socketFd | |
| * A valid, connected socket file descriptor to send on. | |
| * @param fd | |
| * The file descriptor to send to the peer. Ownership of this file | |
| * descriptor is not transferred by this function; the caller remains | |
| * responsible for closing it unless otherwise specified by the | |
| * surrounding protocol. | |
| * @param payload | |
| * A string payload to send alongside the file descriptor. Very large | |
| * payloads may fail depending on the underlying implementation or OS | |
| * limits. | |
| * | |
| * @return tl::expected<void, std::string> | |
| * - On success, contains no value (i.e. an engaged expected<void>). | |
| * - On failure, contains a human-readable error message describing the | |
| * problem (for example, an invalid argument, protocol error, or | |
| * system call failure). | |
| */ |
| if (fork() == 0) { | ||
| close(sv[1]); | ||
| std::string data = "just-text-no-fd"; | ||
| ASSERT_NE(write(sv[0], data.data(), data.size()), -1); | ||
| close(sv[0]); | ||
| _exit(0); |
There was a problem hiding this comment.
The child process doesn't check the return value of sendFdWithPayload before exiting. If the send operation fails, the test may not accurately detect the failure. The child should exit with EXIT_FAILURE if sendFdWithPayload returns an error.
| close(sv[0]); | ||
| auto res = recvFdWithPayload(sv[1], 1024); | ||
| EXPECT_FALSE(res.has_value()); | ||
| wait(nullptr); |
There was a problem hiding this comment.
The test doesn't verify the child process exit status. If the child process encounters an error, the test may still pass. Consider using waitpid with a status variable and checking WEXITSTATUS.
| close(res->fd); | ||
| } | ||
|
|
||
| wait(nullptr); |
There was a problem hiding this comment.
The test doesn't verify the child process exit status. If the child process encounters an assertion failure with EXPECT_TRUE, the test may still pass because wait(nullptr) doesn't check the exit status. Consider using waitpid with a status variable and checking WEXITSTATUS.
| close(res->fd); | ||
| } | ||
|
|
||
| wait(nullptr); |
There was a problem hiding this comment.
The test doesn't verify the child process exit status. If the child process encounters an error, the test may still pass. Consider using waitpid with a status variable and checking WEXITSTATUS.
|
|
||
| struct SocketData | ||
| { | ||
| int fd; | ||
| std::string payload; | ||
| }; | ||
|
|
||
| tl::expected<SocketData, std::string> recvFdWithPayload(int fd, std::size_t bufSize = 4096); | ||
|
|
There was a problem hiding this comment.
The SocketData struct and the public functions lack documentation. Consider adding docstrings to explain:
- The purpose of SocketData and ownership semantics of the fd member
- What recvFdWithPayload and sendFdWithPayload do
- Parameter descriptions (what each parameter represents)
- Return value semantics (when errors occur, what the caller should do with the fd)
- Caller responsibilities (e.g., closing the received fd)
| struct SocketData | |
| { | |
| int fd; | |
| std::string payload; | |
| }; | |
| tl::expected<SocketData, std::string> recvFdWithPayload(int fd, std::size_t bufSize = 4096); | |
| /** | |
| * @brief Container for a file descriptor and an associated payload. | |
| * | |
| * The @c fd member represents a file descriptor that is sent or received | |
| * over a socket together with the textual @c payload. | |
| * | |
| * Ownership of @c fd is not managed automatically by this struct. When | |
| * a @c SocketData instance is returned from @c recvFdWithPayload on | |
| * success, ownership of the contained file descriptor is transferred | |
| * to the caller. The caller is then responsible for closing the | |
| * descriptor when it is no longer needed. | |
| * | |
| * A value of @c -1 for @c fd indicates that there is no valid file | |
| * descriptor associated with this instance. | |
| */ | |
| struct SocketData | |
| { | |
| int fd; | |
| std::string payload; | |
| }; | |
| /** | |
| * @brief Receive a file descriptor and an associated payload over a socket. | |
| * | |
| * This function reads from a socket that supports passing file descriptors | |
| * (for example, a UNIX domain socket) and attempts to receive a single file | |
| * descriptor along with a textual payload. | |
| * | |
| * @param fd | |
| * The socket file descriptor to read from. | |
| * @param bufSize | |
| * The maximum number of bytes to read for the payload. The payload | |
| * may be truncated to this size. | |
| * | |
| * @return | |
| * On success, returns a @c SocketData value containing the received | |
| * file descriptor and payload. Ownership of the returned file descriptor | |
| * is transferred to the caller, who is responsible for closing it when | |
| * it is no longer needed. | |
| * | |
| * On failure, returns an @c unexpected containing a human-readable | |
| * error message. In that case, no ownership of any file descriptor | |
| * is transferred to the caller, and the caller MUST NOT attempt to | |
| * close any descriptor that would have been received. | |
| */ | |
| tl::expected<SocketData, std::string> recvFdWithPayload(int fd, std::size_t bufSize = 4096); | |
| /** | |
| * @brief Send a file descriptor and an associated payload over a socket. | |
| * | |
| * This function sends a single file descriptor along with a textual payload | |
| * over a socket that supports passing file descriptors (for example, a | |
| * UNIX domain socket). | |
| * | |
| * @param socketFd | |
| * The socket file descriptor to send the data over. | |
| * @param fd | |
| * The file descriptor to send. The caller retains ownership of this | |
| * descriptor regardless of success or failure and is responsible for | |
| * closing it when appropriate. | |
| * @param payload | |
| * The textual payload to be sent alongside the file descriptor. | |
| * | |
| * @return | |
| * On success, returns an empty (engaged) @c tl::expected<void, std::string>. | |
| * | |
| * On failure, returns an @c unexpected containing a human-readable error | |
| * message. In that case, the caller still owns @p fd and must decide | |
| * whether to retry the operation or close the descriptor. | |
| */ |
| }(); | ||
|
|
||
| if (n < 0) { | ||
| return tl::make_unexpected(std::string{ "Failed to receive message" } + ::strerror(errno)); |
There was a problem hiding this comment.
The error message concatenation is missing a separator or proper formatting. The error message will appear as "Failed to receive messageBad file descriptor" (or similar) without spacing. Consider using std::format or adding proper spacing, e.g., "Failed to receive message: " with a colon and space.
| return tl::make_unexpected(std::string{ "Failed to receive message" } + ::strerror(errno)); | |
| return tl::make_unexpected(std::string{ "Failed to receive message: " } + ::strerror(errno)); |
|
|
||
| int received_fd{ -1 }; | ||
| struct cmsghdr *cmsg = CMSG_FIRSTHDR(&msg); | ||
|
|
There was a problem hiding this comment.
If cmsg is nullptr, the function returns an error but the received_fd is not closed. However, in this specific case, if there's no control message header, no file descriptor was received, so received_fd remains -1 and doesn't need to be closed. This is correct but could benefit from a code comment explaining this for maintainability.
| // If there is no control message header, no file descriptor was received. | |
| // In this case received_fd remains -1 and does not need to be closed. |
| const std::string large_payload(65536, 'B'); | ||
| if (fork() == 0) { | ||
| close(sv[1]); | ||
| auto ret = sendFdWithPayload(sv[0], STDIN_FILENO, large_payload); |
There was a problem hiding this comment.
The test doesn't check the return value of sendFdWithPayload. If the send fails, the child process exits with status 0 anyway, which could lead to false positives in the test. Consider checking the result and exiting with a failure status if the send operation fails.
| auto ret = sendFdWithPayload(sv[0], STDIN_FILENO, large_payload); | |
| auto ret = sendFdWithPayload(sv[0], STDIN_FILENO, large_payload); | |
| if (!ret.has_value()) { | |
| close(sv[0]); | |
| _exit(1); | |
| } |
| } | ||
|
|
||
| if (cmsg->cmsg_level != SOL_SOCKET || cmsg->cmsg_type != SCM_RIGHTS) { | ||
| return tl::make_unexpected("Not ancillary data with file descriptor"); |
There was a problem hiding this comment.
The error message "Not ancillary data with file descriptor" is grammatically incorrect. It should be "No ancillary data with file descriptor" or "Not an ancillary message with file descriptor" or "Invalid ancillary data: expected file descriptor".
| return tl::make_unexpected("Not ancillary data with file descriptor"); | |
| return tl::make_unexpected("Invalid ancillary data: expected file descriptor"); |
Signed-off-by: ComixHe <ComixHe1895@outlook.com>
Signed-off-by: ComixHe <ComixHe1895@outlook.com>
deepin pr auto review这份代码 diff 主要进行了以下变更：
以下是对代码的详细审查意见： 1. 语法与逻辑
2. 代码质量
3. 代码性能
4. 代码安全
5. 其他建议
总结整体代码质量很高，逻辑清晰，错误处理完善，安全性考虑周全（特别是资源泄漏防护）。将 主要改进点：
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ComixHe, 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 |
No description provided.