libcamera: ipc_unixsocket: Use UniqueFD for a file descriptor

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@chromium.org>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

6 files changed, 49 insertions, 44 deletions

diff --git a/include/libcamera/internal/ipc_unixsocket.h b/include/libcamera/internal/ipc_unixsocket.h
index 5010b66a..3963d182 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 533560cf..65277500 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 bd32fca3..1980d374 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 b3568c06..7e90e629 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;
 		}
@@ -359,11 +359,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;
 		}
@@ -495,9 +495,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));
 	}
 
 	UnixSocketTest test;
diff --git a/test/ipc/unixsocket_ipc.cpp b/test/ipc/unixsocket_ipc.cpp
index 1a8d06a1..3ee6017e 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));
 	}
 
 	UnixSocketTestIPC test;
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 764e7a3a..b65dc4cf 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";