[libcamera-devel] [PATCH v3 10/17] libcamera: ipc_unixsocket: Use UniqueFD for a file descriptor
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Nov 29 00:57:45 CET 2021
From: Hirokazu Honda <hiroh at chromium.org>
IPCUnixSocket::create() creates two file descriptors. One of
them is stored in IPCUnixSocket and the other is returned to a
caller. This clarifies the ownership using UniqueFD.
Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
---
Changes since v2:
- Pass UniqueFD to IPCUnixSocket::bind()
- Return {}
---
include/libcamera/internal/ipc_unixsocket.h | 7 +--
src/libcamera/ipc_pipe_unixsocket.cpp | 8 ++--
src/libcamera/ipc_unixsocket.cpp | 43 ++++++++++---------
test/ipc/unixsocket.cpp | 14 +++---
test/ipc/unixsocket_ipc.cpp | 8 ++--
.../module_ipa_proxy_worker.cpp.tmpl | 13 +++---
6 files changed, 49 insertions(+), 44 deletions(-)
diff --git a/include/libcamera/internal/ipc_unixsocket.h b/include/libcamera/internal/ipc_unixsocket.h
index 5010b66a2bda..3963d182ffa6 100644
--- a/include/libcamera/internal/ipc_unixsocket.h
+++ b/include/libcamera/internal/ipc_unixsocket.h
@@ -12,6 +12,7 @@
#include <vector>
#include <libcamera/base/signal.h>
+#include <libcamera/base/unique_fd.h>
namespace libcamera {
@@ -28,8 +29,8 @@ public:
IPCUnixSocket();
~IPCUnixSocket();
- int create();
- int bind(int fd);
+ UniqueFD create();
+ int bind(UniqueFD fd);
void close();
bool isBound() const;
@@ -49,7 +50,7 @@ private:
void dataNotifier();
- int fd_;
+ UniqueFD fd_;
bool headerReceived_;
struct Header header_;
EventNotifier *notifier_;
diff --git a/src/libcamera/ipc_pipe_unixsocket.cpp b/src/libcamera/ipc_pipe_unixsocket.cpp
index 533560cf95d3..65277500ff42 100644
--- a/src/libcamera/ipc_pipe_unixsocket.cpp
+++ b/src/libcamera/ipc_pipe_unixsocket.cpp
@@ -31,14 +31,14 @@ IPCPipeUnixSocket::IPCPipeUnixSocket(const char *ipaModulePath,
args.push_back(ipaModulePath);
socket_ = std::make_unique<IPCUnixSocket>();
- int fd = socket_->create();
- if (fd < 0) {
+ UniqueFD fd = socket_->create();
+ if (!fd.isValid()) {
LOG(IPCPipe, Error) << "Failed to create socket";
return;
}
socket_->readyRead.connect(this, &IPCPipeUnixSocket::readyRead);
- args.push_back(std::to_string(fd));
- fds.push_back(fd);
+ args.push_back(std::to_string(fd.get()));
+ fds.push_back(fd.release());
proc_ = std::make_unique<Process>();
int ret = proc_->start(ipaProxyWorkerPath, args, fds);
diff --git a/src/libcamera/ipc_unixsocket.cpp b/src/libcamera/ipc_unixsocket.cpp
index bd32fca3a678..1980d374cea8 100644
--- a/src/libcamera/ipc_unixsocket.cpp
+++ b/src/libcamera/ipc_unixsocket.cpp
@@ -7,6 +7,7 @@
#include "libcamera/internal/ipc_unixsocket.h"
+#include <array>
#include <poll.h>
#include <string.h>
#include <sys/socket.h>
@@ -68,7 +69,7 @@ LOG_DEFINE_CATEGORY(IPCUnixSocket)
*/
IPCUnixSocket::IPCUnixSocket()
- : fd_(-1), headerReceived_(false), notifier_(nullptr)
+ : headerReceived_(false), notifier_(nullptr)
{
}
@@ -86,9 +87,9 @@ IPCUnixSocket::~IPCUnixSocket()
* to the remote process, where it can be used with IPCUnixSocket::bind() to
* bind the remote side socket.
*
- * \return A file descriptor on success, negative error code on failure
+ * \return A file descriptor. It is valid on success or invalid otherwise.
*/
-int IPCUnixSocket::create()
+UniqueFD IPCUnixSocket::create()
{
int sockets[2];
int ret;
@@ -98,14 +99,18 @@ int IPCUnixSocket::create()
ret = -errno;
LOG(IPCUnixSocket, Error)
<< "Failed to create socket pair: " << strerror(-ret);
- return ret;
+ return {};
}
- ret = bind(sockets[0]);
- if (ret)
- return ret;
+ std::array<UniqueFD, 2> socketFds{
+ UniqueFD(sockets[0]),
+ UniqueFD(sockets[1]),
+ };
- return sockets[1];
+ if (bind(std::move(socketFds[0])) < 0)
+ return {};
+
+ return std::move(socketFds[1]);
}
/**
@@ -118,13 +123,13 @@ int IPCUnixSocket::create()
*
* \return 0 on success or a negative error code otherwise
*/
-int IPCUnixSocket::bind(int fd)
+int IPCUnixSocket::bind(UniqueFD fd)
{
if (isBound())
return -EINVAL;
- fd_ = fd;
- notifier_ = new EventNotifier(fd_, EventNotifier::Read);
+ fd_ = std::move(fd);
+ notifier_ = new EventNotifier(fd_.get(), EventNotifier::Read);
notifier_->activated.connect(this, &IPCUnixSocket::dataNotifier);
return 0;
@@ -143,9 +148,7 @@ void IPCUnixSocket::close()
delete notifier_;
notifier_ = nullptr;
- ::close(fd_);
-
- fd_ = -1;
+ fd_.reset();
headerReceived_ = false;
}
@@ -155,7 +158,7 @@ void IPCUnixSocket::close()
*/
bool IPCUnixSocket::isBound() const
{
- return fd_ != -1;
+ return fd_.isValid();
}
/**
@@ -182,7 +185,7 @@ int IPCUnixSocket::send(const Payload &payload)
if (!hdr.data && !hdr.fds)
return -EINVAL;
- ret = ::send(fd_, &hdr, sizeof(hdr), 0);
+ ret = ::send(fd_.get(), &hdr, sizeof(hdr), 0);
if (ret < 0) {
ret = -errno;
LOG(IPCUnixSocket, Error)
@@ -263,7 +266,7 @@ int IPCUnixSocket::sendData(const void *buffer, size_t length,
if (fds)
memcpy(CMSG_DATA(cmsg), fds, num * sizeof(uint32_t));
- if (sendmsg(fd_, &msg, 0) < 0) {
+ if (sendmsg(fd_.get(), &msg, 0) < 0) {
int ret = -errno;
LOG(IPCUnixSocket, Error)
<< "Failed to sendmsg: " << strerror(-ret);
@@ -297,7 +300,7 @@ int IPCUnixSocket::recvData(void *buffer, size_t length,
msg.msg_controllen = cmsg->cmsg_len;
msg.msg_flags = 0;
- if (recvmsg(fd_, &msg, 0) < 0) {
+ if (recvmsg(fd_.get(), &msg, 0) < 0) {
int ret = -errno;
if (ret != -EAGAIN)
LOG(IPCUnixSocket, Error)
@@ -317,7 +320,7 @@ void IPCUnixSocket::dataNotifier()
if (!headerReceived_) {
/* Receive the header. */
- ret = ::recv(fd_, &header_, sizeof(header_), 0);
+ ret = ::recv(fd_.get(), &header_, sizeof(header_), 0);
if (ret < 0) {
ret = -errno;
LOG(IPCUnixSocket, Error)
@@ -333,7 +336,7 @@ void IPCUnixSocket::dataNotifier()
* readyRead signal. The notifier will be reenabled by the receive()
* function.
*/
- struct pollfd fds = { fd_, POLLIN, 0 };
+ struct pollfd fds = { fd_.get(), POLLIN, 0 };
ret = poll(&fds, 1, 0);
if (ret < 0)
return;
diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp
index 7270bf4d2fe7..414f1bfc9d12 100644
--- a/test/ipc/unixsocket.cpp
+++ b/test/ipc/unixsocket.cpp
@@ -52,9 +52,9 @@ public:
ipc_.readyRead.connect(this, &UnixSocketTestSlave::readyRead);
}
- int run(int fd)
+ int run(UniqueFD fd)
{
- if (ipc_.bind(fd)) {
+ if (ipc_.bind(std::move(fd))) {
cerr << "Failed to connect to IPC channel" << endl;
return EXIT_FAILURE;
}
@@ -360,11 +360,11 @@ protected:
int run()
{
- int slavefd = ipc_.create();
- if (slavefd < 0)
+ UniqueFD slavefd = ipc_.create();
+ if (!slavefd.isValid())
return TestFail;
- if (slaveStart(slavefd)) {
+ if (slaveStart(slavefd.release())) {
cerr << "Failed to start slave" << endl;
return TestFail;
}
@@ -496,9 +496,9 @@ private:
int main(int argc, char **argv)
{
if (argc == 2) {
- int ipcfd = std::stoi(argv[1]);
+ UniqueFD ipcfd = UniqueFD(std::stoi(argv[1]));
UnixSocketTestSlave slave;
- return slave.run(ipcfd);
+ return slave.run(std::move(ipcfd));
}
return UnixSocketTest().execute();
diff --git a/test/ipc/unixsocket_ipc.cpp b/test/ipc/unixsocket_ipc.cpp
index ab5d25572d83..178ee1870056 100644
--- a/test/ipc/unixsocket_ipc.cpp
+++ b/test/ipc/unixsocket_ipc.cpp
@@ -49,9 +49,9 @@ public:
ipc_.readyRead.connect(this, &UnixSocketTestIPCSlave::readyRead);
}
- int run(int fd)
+ int run(UniqueFD fd)
{
- if (ipc_.bind(fd)) {
+ if (ipc_.bind(std::move(fd))) {
cerr << "Failed to connect to IPC channel" << endl;
return EXIT_FAILURE;
}
@@ -222,9 +222,9 @@ int main(int argc, char **argv)
{
/* IPCPipeUnixSocket passes IPA module path in argv[1] */
if (argc == 3) {
- int ipcfd = std::stoi(argv[2]);
+ UniqueFD ipcfd = UniqueFD(std::stoi(argv[2]));
UnixSocketTestIPCSlave slave;
- return slave.run(ipcfd);
+ return slave.run(std::move(ipcfd));
}
return UnixSocketTestIPC().execute();
diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
index 764e7a3af63a..b65dc4cf31c5 100644
--- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
+++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
@@ -27,8 +27,9 @@
#include <libcamera/logging.h>
#include <libcamera/base/event_dispatcher.h>
-#include <libcamera/base/thread.h>
#include <libcamera/base/log.h>
+#include <libcamera/base/thread.h>
+#include <libcamera/base/unique_fd.h>
#include "libcamera/internal/camera_sensor.h"
#include "libcamera/internal/control_serializer.h"
@@ -122,9 +123,9 @@ public:
}
}
- int init(std::unique_ptr<IPAModule> &ipam, int socketfd)
+ int init(std::unique_ptr<IPAModule> &ipam, UniqueFD socketfd)
{
- if (socket_.bind(socketfd) < 0) {
+ if (socket_.bind(std::move(socketfd)) < 0) {
LOG({{proxy_worker_name}}, Error)
<< "IPC socket binding failed";
return EXIT_FAILURE;
@@ -203,10 +204,10 @@ int main(int argc, char **argv)
return EXIT_FAILURE;
}
- int fd = std::stoi(argv[2]);
+ UniqueFD fd(std::stoi(argv[2]));
LOG({{proxy_worker_name}}, Info)
<< "Starting worker for IPA module " << argv[1]
- << " with IPC fd = " << fd;
+ << " with IPC fd = " << fd.get();
std::unique_ptr<IPAModule> ipam = std::make_unique<IPAModule>(argv[1]);
if (!ipam->isValid() || !ipam->load()) {
@@ -228,7 +229,7 @@ int main(int argc, char **argv)
}
{{proxy_worker_name}} proxyWorker;
- int ret = proxyWorker.init(ipam, fd);
+ int ret = proxyWorker.init(ipam, std::move(fd));
if (ret < 0) {
LOG({{proxy_worker_name}}, Error)
<< "Failed to initialize proxy worker";
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list