From patchwork Tue Nov 30 03:38:08 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 14864 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 5373BBF415 for ; Tue, 30 Nov 2021 03:39:08 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id F0DFE605C0; Tue, 30 Nov 2021 04:39:07 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="lB/r2jat"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 05402605B7 for ; Tue, 30 Nov 2021 04:38:53 +0100 (CET) Received: from pendragon.lan (62-78-145-57.bb.dnainternet.fi [62.78.145.57]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 82DEF11FB; Tue, 30 Nov 2021 04:38:52 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1638243532; bh=GrM0K1sdUmF0a281uXKr6sIa76N/CCcYSkKarAvQ33w=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=lB/r2jatZ/yVHszbcj5yTPZtpbCZNCkirZDNWN1o3qSK5qUNr5Y0gNLFVNLahhNN0 YCw/xbdxeg8HSdfsM2NSOls+2DTWwOutZx/q+X4JkS5B58i8U4aWYmxntR3YWhNmzd Wrv6h4UhVirxOTKuGGazPG2KcCGUQ02zsPY53O+Y= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Tue, 30 Nov 2021 05:38:08 +0200 Message-Id: <20211130033820.18235-11-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.32.0 In-Reply-To: <20211130033820.18235-1-laurent.pinchart@ideasonboard.com> References: <20211130033820.18235-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v4 10/22] libcamera: ipc_unixsocket: Use UniqueFD for a file descriptor X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" From: Hirokazu Honda 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 Signed-off-by: Laurent Pinchart Reviewed-by: Hirokazu Honda Reviewed-by: Jacopo Mondi --- 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 #include +#include 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(); - 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(); 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 #include #include #include @@ -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 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 #include -#include #include +#include +#include #include "libcamera/internal/camera_sensor.h" #include "libcamera/internal/control_serializer.h" @@ -122,9 +123,9 @@ public: } } - int init(std::unique_ptr &ipam, int socketfd) + int init(std::unique_ptr &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 ipam = std::make_unique(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";