[libcamera-devel] [RFC PATCH 08/10] libcamera: IPCUnixSocket: Use ScopedFD for a file descriptor
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Jun 6 20:30:16 CEST 2021
Hi Hiro,
Thank you for the patch.
On Thu, Apr 15, 2021 at 05:38:41PM +0900, Hirokazu Honda wrote:
> 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 ScopedFD.
>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
> include/libcamera/internal/ipc_unixsocket.h | 5 +--
> src/libcamera/ipc_pipe_unixsocket.cpp | 8 ++---
> src/libcamera/ipc_unixsocket.cpp | 40 ++++++++++-----------
> test/ipc/unixsocket.cpp | 6 ++--
> 4 files changed, 30 insertions(+), 29 deletions(-)
>
> diff --git a/include/libcamera/internal/ipc_unixsocket.h b/include/libcamera/internal/ipc_unixsocket.h
> index e871b650..c06fee47 100644
> --- a/include/libcamera/internal/ipc_unixsocket.h
> +++ b/include/libcamera/internal/ipc_unixsocket.h
> @@ -12,6 +12,7 @@
> #include <sys/types.h>
> #include <vector>
>
> +#include <libcamera/scoped_file_descriptor.h>
> #include <libcamera/signal.h>
>
> namespace libcamera {
> @@ -29,7 +30,7 @@ public:
> IPCUnixSocket();
> ~IPCUnixSocket();
>
> - int create();
> + ScopedFD create();
> int bind(int fd);
> void close();
> bool isBound() const;
> @@ -50,7 +51,7 @@ private:
>
> void dataNotifier(EventNotifier *notifier);
>
> - int fd_;
> + ScopedFD 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 db0e260f..1997643f 100644
> --- a/src/libcamera/ipc_pipe_unixsocket.cpp
> +++ b/src/libcamera/ipc_pipe_unixsocket.cpp
> @@ -30,14 +30,14 @@ IPCPipeUnixSocket::IPCPipeUnixSocket(const char *ipaModulePath,
> args.push_back(ipaModulePath);
>
> socket_ = std::make_unique<IPCUnixSocket>();
> - int fd = socket_->create();
> - if (fd < 0) {
> + ScopedFD 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 fdb359f7..2da43188 100644
> --- a/src/libcamera/ipc_unixsocket.cpp
> +++ b/src/libcamera/ipc_unixsocket.cpp
> @@ -68,7 +68,7 @@ LOG_DEFINE_CATEGORY(IPCUnixSocket)
> */
>
> IPCUnixSocket::IPCUnixSocket()
> - : fd_(-1), headerReceived_(false), notifier_(nullptr)
> + : headerReceived_(false), notifier_(nullptr)
> {
> }
>
> @@ -86,9 +86,9 @@ IPCUnixSocket::~IPCUnixSocket()
> * 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()
> +ScopedFD IPCUnixSocket::create()
> {
> int sockets[2];
> int ret;
> @@ -98,14 +98,13 @@ int IPCUnixSocket::create()
> ret = -errno;
> LOG(IPCUnixSocket, Error)
> << "Failed to create socket pair: " << strerror(-ret);
> - return ret;
> + return ScopedFD();
> }
>
> - ret = bind(sockets[0]);
> - if (ret)
> - return ret;
> + if (bind(sockets[0]) < 0)
> + return ScopedFD();
>
> - return sockets[1];
> + return ScopedFD(sockets[1]);
> }
>
> /**
> @@ -116,15 +115,18 @@ int IPCUnixSocket::create()
> * by the file descriptor \a fd. The file descriptor is obtained from the
> * IPCUnixSocket::create() method.
> *
> - * \return 0 on success or a negative error code otherwise
> + * \return 0 on success or a negative error code otherwise.
> + *
> + * \todo This argument should be ScopedFD because bind() takes over the
> + * ownership.
Could you address this ?
> */
> int IPCUnixSocket::bind(int fd)
> {
> if (isBound())
> return -EINVAL;
>
> - fd_ = fd;
> - notifier_ = new EventNotifier(fd_, EventNotifier::Read);
> + fd_ = ScopedFD(fd);
> + notifier_ = new EventNotifier(fd_.get(), EventNotifier::Read);
> notifier_->activated.connect(this, &IPCUnixSocket::dataNotifier);
>
> return 0;
> @@ -143,9 +145,7 @@ void IPCUnixSocket::close()
> delete notifier_;
> notifier_ = nullptr;
>
> - ::close(fd_);
> -
> - fd_ = -1;
> + fd_.reset();
> headerReceived_ = false;
> }
>
> @@ -155,7 +155,7 @@ void IPCUnixSocket::close()
> */
> bool IPCUnixSocket::isBound() const
> {
> - return fd_ != -1;
> + return fd_.isValid();
> }
>
> /**
> @@ -182,7 +182,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)
> @@ -262,7 +262,7 @@ int IPCUnixSocket::sendData(const void *buffer, size_t length,
> msg.msg_flags = 0;
> 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);
> @@ -296,7 +296,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)
> @@ -315,7 +315,7 @@ void IPCUnixSocket::dataNotifier([[maybe_unused]] EventNotifier *notifier)
>
> 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)
> @@ -331,7 +331,7 @@ void IPCUnixSocket::dataNotifier([[maybe_unused]] EventNotifier *notifier)
> * readyRead signal. The notifier will be reenabled by the receive()
> * method.
> */
> - 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 80157b34..9ca0467b 100644
> --- a/test/ipc/unixsocket.cpp
> +++ b/test/ipc/unixsocket.cpp
> @@ -359,11 +359,11 @@ protected:
>
> int run()
> {
> - int slavefd = ipc_.create();
> - if (slavefd < 0)
> + ScopedFD slavefd = ipc_.create();
> + if (!slavefd.isValid())
> return TestFail;
>
> - if (slaveStart(slavefd)) {
> + if (slaveStart(slavefd.release())) {
Should the slaveStart() function take a ScopedFD && ?
> cerr << "Failed to start slave" << endl;
> return TestFail;
> }
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list