[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 17:57:12 CET 2021
Hi Jacopo,
On Mon, Nov 29, 2021 at 04:54:36PM +0100, Jacopo Mondi wrote:
> On Mon, Nov 29, 2021 at 04:33:51PM +0100, Jacopo Mondi wrote:
> > On Mon, Nov 29, 2021 at 01:57:45AM +0200, Laurent Pinchart wrote:
> > > 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);
> >
> > Was in the previous version fd leaked ??
It seems to be leaked in this version too actually... I'll test it.
> > > + 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) {
> >
> > Is this bind() IPCUnixSocket::bind(UniqueFD fd) ?
> > do you need to move then ?
>
> Yes you do, sorry.
You're right :-)
> > > 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));
> >
> > Same question
> >
> > > if (ret < 0) {
> > > LOG({{proxy_worker_name}}, Error)
> > > << "Failed to initialize proxy worker";
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list