[libcamera-devel,v3,10/17] libcamera: ipc_unixsocket: Use UniqueFD for a file descriptor
diff mbox series

Message ID 20211128235752.10836-11-laurent.pinchart@ideasonboard.com
State Superseded
Headers show
Series
  • libcamera: Introduce UniqueFD
Related show

Commit Message

Laurent Pinchart Nov. 28, 2021, 11:57 p.m. UTC 
From: Hirokazu Honda <hiroh@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@chromium.org>
Signed-off-by: Laurent Pinchart <laurent.pinchart@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(-)

Comments

Hirokazu Honda Nov. 29, 2021, 1:56 p.m. UTC | #1 
Hi Laurent,

On Mon, Nov 29, 2021 at 8:58 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> From: Hirokazu Honda <hiroh@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@chromium.org>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Looks good to me.
Reviewed-by: Hirokazu Honda <hiroh@chromium.org> if applicable.
> ---
> 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
>
Jacopo Mondi Nov. 29, 2021, 3:33 p.m. UTC | #2 
On Mon, Nov 29, 2021 at 01:57:45AM +0200, Laurent Pinchart wrote:
> From: Hirokazu Honda <hiroh@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@chromium.org>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@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 ??

> +	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 ?

>  			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

Thanks
  j
>  	if (ret < 0) {
>  		LOG({{proxy_worker_name}}, Error)
>  			<< "Failed to initialize proxy worker";
> --
> Regards,
>
> Laurent Pinchart
>
Jacopo Mondi Nov. 29, 2021, 3:54 p.m. UTC | #3 
Hi again

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@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@chromium.org>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@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 ??
>
> > +	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.

> >  			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
>
> Thanks
>   j
> >  	if (ret < 0) {
> >  		LOG({{proxy_worker_name}}, Error)
> >  			<< "Failed to initialize proxy worker";
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
Laurent Pinchart Nov. 29, 2021, 4:57 p.m. UTC | #4 
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@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@chromium.org>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@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";

Patch
diff mbox series 

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";