[{"id":17414,"web_url":"https://patchwork.libcamera.org/comment/17414/","msgid":"","date":"2021-06-06T18:30:16","subject":"Re: [libcamera-devel] [RFC PATCH 08/10] libcamera: IPCUnixSocket:\n\tUse ScopedFD for a file descriptor","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Thu, Apr 15, 2021 at 05:38:41PM +0900, Hirokazu Honda wrote:\n> IPCUnixSocket::create() creates two file descriptors. One of\n> them is stored in IPCUnixSocket and the other is returned to a\n> caller. This clarifies the ownership using ScopedFD.\n> \n> Signed-off-by: Hirokazu Honda \n> ---\n> include/libcamera/internal/ipc_unixsocket.h | 5 +--\n> src/libcamera/ipc_pipe_unixsocket.cpp | 8 ++---\n> src/libcamera/ipc_unixsocket.cpp | 40 ++++++++++-----------\n> test/ipc/unixsocket.cpp | 6 ++--\n> 4 files changed, 30 insertions(+), 29 deletions(-)\n> \n> diff --git a/include/libcamera/internal/ipc_unixsocket.h b/include/libcamera/internal/ipc_unixsocket.h\n> index e871b650..c06fee47 100644\n> --- a/include/libcamera/internal/ipc_unixsocket.h\n> +++ b/include/libcamera/internal/ipc_unixsocket.h\n> @@ -12,6 +12,7 @@\n> #include \n> #include \n> \n> +#include \n> #include \n> \n> namespace libcamera {\n> @@ -29,7 +30,7 @@ public:\n> \tIPCUnixSocket();\n> \t~IPCUnixSocket();\n> \n> -\tint create();\n> +\tScopedFD create();\n> \tint bind(int fd);\n> \tvoid close();\n> \tbool isBound() const;\n> @@ -50,7 +51,7 @@ private:\n> \n> \tvoid dataNotifier(EventNotifier *notifier);\n> \n> -\tint fd_;\n> +\tScopedFD fd_;\n> \tbool headerReceived_;\n> \tstruct Header header_;\n> \tEventNotifier *notifier_;\n> diff --git a/src/libcamera/ipc_pipe_unixsocket.cpp b/src/libcamera/ipc_pipe_unixsocket.cpp\n> index db0e260f..1997643f 100644\n> --- a/src/libcamera/ipc_pipe_unixsocket.cpp\n> +++ b/src/libcamera/ipc_pipe_unixsocket.cpp\n> @@ -30,14 +30,14 @@ IPCPipeUnixSocket::IPCPipeUnixSocket(const char *ipaModulePath,\n> \targs.push_back(ipaModulePath);\n> \n> \tsocket_ = std::make_unique();\n> -\tint fd = socket_->create();\n> -\tif (fd < 0) {\n> +\tScopedFD fd = socket_->create();\n> +\tif (!fd.isValid()) {\n> \t\tLOG(IPCPipe, Error) << \"Failed to create socket\";\n> \t\treturn;\n> \t}\n> \tsocket_->readyRead.connect(this, &IPCPipeUnixSocket::readyRead);\n> -\targs.push_back(std::to_string(fd));\n> -\tfds.push_back(fd);\n> +\targs.push_back(std::to_string(fd.get()));\n> +\tfds.push_back(fd.release());\n> \n> \tproc_ = std::make_unique();\n> \tint ret = proc_->start(ipaProxyWorkerPath, args, fds);\n> diff --git a/src/libcamera/ipc_unixsocket.cpp b/src/libcamera/ipc_unixsocket.cpp\n> index fdb359f7..2da43188 100644\n> --- a/src/libcamera/ipc_unixsocket.cpp\n> +++ b/src/libcamera/ipc_unixsocket.cpp\n> @@ -68,7 +68,7 @@ LOG_DEFINE_CATEGORY(IPCUnixSocket)\n> */\n> \n> IPCUnixSocket::IPCUnixSocket()\n> -\t: fd_(-1), headerReceived_(false), notifier_(nullptr)\n> +\t: headerReceived_(false), notifier_(nullptr)\n> {\n> }\n> \n> @@ -86,9 +86,9 @@ IPCUnixSocket::~IPCUnixSocket()\n> * the remote process, where it can be used with IPCUnixSocket::bind() to bind\n> * the remote side socket.\n> *\n> - * \\return A file descriptor on success, negative error code on failure\n> + * \\return A file descriptor. It is valid on success or invalid otherwise.\n> */\n> -int IPCUnixSocket::create()\n> +ScopedFD IPCUnixSocket::create()\n> {\n> \tint sockets[2];\n> \tint ret;\n> @@ -98,14 +98,13 @@ int IPCUnixSocket::create()\n> \t\tret = -errno;\n> \t\tLOG(IPCUnixSocket, Error)\n> \t\t\t<< \"Failed to create socket pair: \" << strerror(-ret);\n> -\t\treturn ret;\n> +\t\treturn ScopedFD();\n> \t}\n> \n> -\tret = bind(sockets[0]);\n> -\tif (ret)\n> -\t\treturn ret;\n> +\tif (bind(sockets[0]) < 0)\n> +\t\treturn ScopedFD();\n> \n> -\treturn sockets[1];\n> +\treturn ScopedFD(sockets[1]);\n> }\n> \n> /**\n> @@ -116,15 +115,18 @@ int IPCUnixSocket::create()\n> * by the file descriptor \\a fd. The file descriptor is obtained from the\n> * IPCUnixSocket::create() method.\n> *\n> - * \\return 0 on success or a negative error code otherwise\n> + * \\return 0 on success or a negative error code otherwise.\n> + *\n> + * \\todo This argument should be ScopedFD because bind() takes over the\n> + * ownership.\n\nCould you address this ?\n\n> */\n> int IPCUnixSocket::bind(int fd)\n> {\n> \tif (isBound())\n> \t\treturn -EINVAL;\n> \n> -\tfd_ = fd;\n> -\tnotifier_ = new EventNotifier(fd_, EventNotifier::Read);\n> +\tfd_ = ScopedFD(fd);\n> +\tnotifier_ = new EventNotifier(fd_.get(), EventNotifier::Read);\n> \tnotifier_->activated.connect(this, &IPCUnixSocket::dataNotifier);\n> \n> \treturn 0;\n> @@ -143,9 +145,7 @@ void IPCUnixSocket::close()\n> \tdelete notifier_;\n> \tnotifier_ = nullptr;\n> \n> -\t::close(fd_);\n> -\n> -\tfd_ = -1;\n> +\tfd_.reset();\n> \theaderReceived_ = false;\n> }\n> \n> @@ -155,7 +155,7 @@ void IPCUnixSocket::close()\n> */\n> bool IPCUnixSocket::isBound() const\n> {\n> -\treturn fd_ != -1;\n> +\treturn fd_.isValid();\n> }\n> \n> /**\n> @@ -182,7 +182,7 @@ int IPCUnixSocket::send(const Payload &payload)\n> \tif (!hdr.data && !hdr.fds)\n> \t\treturn -EINVAL;\n> \n> -\tret = ::send(fd_, &hdr, sizeof(hdr), 0);\n> +\tret = ::send(fd_.get(), &hdr, sizeof(hdr), 0);\n> \tif (ret < 0) {\n> \t\tret = -errno;\n> \t\tLOG(IPCUnixSocket, Error)\n> @@ -262,7 +262,7 @@ int IPCUnixSocket::sendData(const void *buffer, size_t length,\n> \tmsg.msg_flags = 0;\n> \tmemcpy(CMSG_DATA(cmsg), fds, num * sizeof(uint32_t));\n> \n> -\tif (sendmsg(fd_, &msg, 0) < 0) {\n> +\tif (sendmsg(fd_.get(), &msg, 0) < 0) {\n> \t\tint ret = -errno;\n> \t\tLOG(IPCUnixSocket, Error)\n> \t\t\t<< \"Failed to sendmsg: \" << strerror(-ret);\n> @@ -296,7 +296,7 @@ int IPCUnixSocket::recvData(void *buffer, size_t length,\n> \tmsg.msg_controllen = cmsg->cmsg_len;\n> \tmsg.msg_flags = 0;\n> \n> -\tif (recvmsg(fd_, &msg, 0) < 0) {\n> +\tif (recvmsg(fd_.get(), &msg, 0) < 0) {\n> \t\tint ret = -errno;\n> \t\tif (ret != -EAGAIN)\n> \t\t\tLOG(IPCUnixSocket, Error)\n> @@ -315,7 +315,7 @@ void IPCUnixSocket::dataNotifier([[maybe_unused]] EventNotifier *notifier)\n> \n> \tif (!headerReceived_) {\n> \t\t/* Receive the header. */\n> -\t\tret = ::recv(fd_, &header_, sizeof(header_), 0);\n> +\t\tret = ::recv(fd_.get(), &header_, sizeof(header_), 0);\n> \t\tif (ret < 0) {\n> \t\t\tret = -errno;\n> \t\t\tLOG(IPCUnixSocket, Error)\n> @@ -331,7 +331,7 @@ void IPCUnixSocket::dataNotifier([[maybe_unused]] EventNotifier *notifier)\n> \t * readyRead signal. The notifier will be reenabled by the receive()\n> \t * method.\n> \t */\n> -\tstruct pollfd fds = { fd_, POLLIN, 0 };\n> +\tstruct pollfd fds = { fd_.get(), POLLIN, 0 };\n> \tret = poll(&fds, 1, 0);\n> \tif (ret < 0)\n> \t\treturn;\n> diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp\n> index 80157b34..9ca0467b 100644\n> --- a/test/ipc/unixsocket.cpp\n> +++ b/test/ipc/unixsocket.cpp\n> @@ -359,11 +359,11 @@ protected:\n> \n> \tint run()\n> \t{\n> -\t\tint slavefd = ipc_.create();\n> -\t\tif (slavefd < 0)\n> +\t\tScopedFD slavefd = ipc_.create();\n> +\t\tif (!slavefd.isValid())\n> \t\t\treturn TestFail;\n> \n> -\t\tif (slaveStart(slavefd)) {\n> +\t\tif (slaveStart(slavefd.release())) {\n\nShould the slaveStart() function take a ScopedFD && ?\n\n> \t\t\tcerr << \"Failed to start slave\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 82554C3206\n\tfor ;\n\tSun, 6 Jun 2021 18:30:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0666668926;\n\tSun, 6 Jun 2021 20:30:31 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 06E5C602A7\n\tfor ;\n\tSun, 6 Jun 2021 20:30:30 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7BD54EF;\n\tSun, 6 Jun 2021 20:30:29 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"FkpnjIdV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623004229;\n\tbh=jEQXEiM027LMub3QGoyAqUICbpVqaqg06TaMA+4eJwk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FkpnjIdVcG6o6NWolR5mj2KTKi9KLfYQzmujbEkj66COay+sR2GyK+GxmlBXYl3uf\n\tyPmpKZmunwg+QRHhtUxDMXWOh7fta2DE28QxemknzONNJv8hRdgqcolanTpUF1XUe1\n\t5RaLzpJzUPkjTxR2rnll82G3iCLLFNvELRgSH/G8=","Date":"Sun, 6 Jun 2021 21:30:16 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210415083843.3399502-1-hiroh@chromium.org>\n\t<20210415083843.3399502-8-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210415083843.3399502-8-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [RFC PATCH 08/10] libcamera: IPCUnixSocket:\n\tUse ScopedFD for a file descriptor","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17434,"web_url":"https://patchwork.libcamera.org/comment/17434/","msgid":"","date":"2021-06-07T09:15:31","subject":"Re: [libcamera-devel] [RFC PATCH 08/10] libcamera: IPCUnixSocket:\n\tUse ScopedFD for a file descriptor","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Mon, Jun 7, 2021 at 3:30 AM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> On Thu, Apr 15, 2021 at 05:38:41PM +0900, Hirokazu Honda wrote:\n> > IPCUnixSocket::create() creates two file descriptors. One of\n> > them is stored in IPCUnixSocket and the other is returned to a\n> > caller. This clarifies the ownership using ScopedFD.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > include/libcamera/internal/ipc_unixsocket.h | 5 +--\n> > src/libcamera/ipc_pipe_unixsocket.cpp | 8 ++---\n> > src/libcamera/ipc_unixsocket.cpp | 40 ++++++++++-----------\n> > test/ipc/unixsocket.cpp | 6 ++--\n> > 4 files changed, 30 insertions(+), 29 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/ipc_unixsocket.h\n> b/include/libcamera/internal/ipc_unixsocket.h\n> > index e871b650..c06fee47 100644\n> > --- a/include/libcamera/internal/ipc_unixsocket.h\n> > +++ b/include/libcamera/internal/ipc_unixsocket.h\n> > @@ -12,6 +12,7 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > #include \n> >\n> > namespace libcamera {\n> > @@ -29,7 +30,7 @@ public:\n> > IPCUnixSocket();\n> > ~IPCUnixSocket();\n> >\n> > - int create();\n> > + ScopedFD create();\n> > int bind(int fd);\n> > void close();\n> > bool isBound() const;\n> > @@ -50,7 +51,7 @@ private:\n> >\n> > void dataNotifier(EventNotifier *notifier);\n> >\n> > - int fd_;\n> > + ScopedFD fd_;\n> > bool headerReceived_;\n> > struct Header header_;\n> > EventNotifier *notifier_;\n> > diff --git a/src/libcamera/ipc_pipe_unixsocket.cpp\n> b/src/libcamera/ipc_pipe_unixsocket.cpp\n> > index db0e260f..1997643f 100644\n> > --- a/src/libcamera/ipc_pipe_unixsocket.cpp\n> > +++ b/src/libcamera/ipc_pipe_unixsocket.cpp\n> > @@ -30,14 +30,14 @@ IPCPipeUnixSocket::IPCPipeUnixSocket(const char\n> *ipaModulePath,\n> > args.push_back(ipaModulePath);\n> >\n> > socket_ = std::make_unique();\n> > - int fd = socket_->create();\n> > - if (fd < 0) {\n> > + ScopedFD fd = socket_->create();\n> > + if (!fd.isValid()) {\n> > LOG(IPCPipe, Error) << \"Failed to create socket\";\n> > return;\n> > }\n> > socket_->readyRead.connect(this, &IPCPipeUnixSocket::readyRead);\n> > - args.push_back(std::to_string(fd));\n> > - fds.push_back(fd);\n> > + args.push_back(std::to_string(fd.get()));\n> > + fds.push_back(fd.release());\n> >\n> > proc_ = std::make_unique();\n> > int ret = proc_->start(ipaProxyWorkerPath, args, fds);\n> > diff --git a/src/libcamera/ipc_unixsocket.cpp\n> b/src/libcamera/ipc_unixsocket.cpp\n> > index fdb359f7..2da43188 100644\n> > --- a/src/libcamera/ipc_unixsocket.cpp\n> > +++ b/src/libcamera/ipc_unixsocket.cpp\n> > @@ -68,7 +68,7 @@ LOG_DEFINE_CATEGORY(IPCUnixSocket)\n> > */\n> >\n> > IPCUnixSocket::IPCUnixSocket()\n> > - : fd_(-1), headerReceived_(false), notifier_(nullptr)\n> > + : headerReceived_(false), notifier_(nullptr)\n> > {\n> > }\n> >\n> > @@ -86,9 +86,9 @@ IPCUnixSocket::~IPCUnixSocket()\n> > * the remote process, where it can be used with IPCUnixSocket::bind()\n> to bind\n> > * the remote side socket.\n> > *\n> > - * \\return A file descriptor on success, negative error code on failure\n> > + * \\return A file descriptor. It is valid on success or invalid\n> otherwise.\n> > */\n> > -int IPCUnixSocket::create()\n> > +ScopedFD IPCUnixSocket::create()\n> > {\n> > int sockets[2];\n> > int ret;\n> > @@ -98,14 +98,13 @@ int IPCUnixSocket::create()\n> > ret = -errno;\n> > LOG(IPCUnixSocket, Error)\n> > << \"Failed to create socket pair: \" <<\n> strerror(-ret);\n> > - return ret;\n> > + return ScopedFD();\n> > }\n> >\n> > - ret = bind(sockets[0]);\n> > - if (ret)\n> > - return ret;\n> > + if (bind(sockets[0]) < 0)\n> > + return ScopedFD();\n> >\n> > - return sockets[1];\n> > + return ScopedFD(sockets[1]);\n> > }\n> >\n> > /**\n> > @@ -116,15 +115,18 @@ int IPCUnixSocket::create()\n> > * by the file descriptor \\a fd. The file descriptor is obtained from\n> the\n> > * IPCUnixSocket::create() method.\n> > *\n> > - * \\return 0 on success or a negative error code otherwise\n> > + * \\return 0 on success or a negative error code otherwise.\n> > + *\n> > + * \\todo This argument should be ScopedFD because bind() takes over the\n> > + * ownership.\n>\n> Could you address this ?\n>\n> > */\n> > int IPCUnixSocket::bind(int fd)\n> > {\n> > if (isBound())\n> > return -EINVAL;\n> >\n> > - fd_ = fd;\n> > - notifier_ = new EventNotifier(fd_, EventNotifier::Read);\n> > + fd_ = ScopedFD(fd);\n> > + notifier_ = new EventNotifier(fd_.get(), EventNotifier::Read);\n> > notifier_->activated.connect(this, &IPCUnixSocket::dataNotifier);\n> >\n> > return 0;\n> > @@ -143,9 +145,7 @@ void IPCUnixSocket::close()\n> > delete notifier_;\n> > notifier_ = nullptr;\n> >\n> > - ::close(fd_);\n> > -\n> > - fd_ = -1;\n> > + fd_.reset();\n> > headerReceived_ = false;\n> > }\n> >\n> > @@ -155,7 +155,7 @@ void IPCUnixSocket::close()\n> > */\n> > bool IPCUnixSocket::isBound() const\n> > {\n> > - return fd_ != -1;\n> > + return fd_.isValid();\n> > }\n> >\n> > /**\n> > @@ -182,7 +182,7 @@ int IPCUnixSocket::send(const Payload &payload)\n> > if (!hdr.data && !hdr.fds)\n> > return -EINVAL;\n> >\n> > - ret = ::send(fd_, &hdr, sizeof(hdr), 0);\n> > + ret = ::send(fd_.get(), &hdr, sizeof(hdr), 0);\n> > if (ret < 0) {\n> > ret = -errno;\n> > LOG(IPCUnixSocket, Error)\n> > @@ -262,7 +262,7 @@ int IPCUnixSocket::sendData(const void *buffer,\n> size_t length,\n> > msg.msg_flags = 0;\n> > memcpy(CMSG_DATA(cmsg), fds, num * sizeof(uint32_t));\n> >\n> > - if (sendmsg(fd_, &msg, 0) < 0) {\n> > + if (sendmsg(fd_.get(), &msg, 0) < 0) {\n> > int ret = -errno;\n> > LOG(IPCUnixSocket, Error)\n> > << \"Failed to sendmsg: \" << strerror(-ret);\n> > @@ -296,7 +296,7 @@ int IPCUnixSocket::recvData(void *buffer, size_t\n> length,\n> > msg.msg_controllen = cmsg->cmsg_len;\n> > msg.msg_flags = 0;\n> >\n> > - if (recvmsg(fd_, &msg, 0) < 0) {\n> > + if (recvmsg(fd_.get(), &msg, 0) < 0) {\n> > int ret = -errno;\n> > if (ret != -EAGAIN)\n> > LOG(IPCUnixSocket, Error)\n> > @@ -315,7 +315,7 @@ void IPCUnixSocket::dataNotifier([[maybe_unused]]\n> EventNotifier *notifier)\n> >\n> > if (!headerReceived_) {\n> > /* Receive the header. */\n> > - ret = ::recv(fd_, &header_, sizeof(header_), 0);\n> > + ret = ::recv(fd_.get(), &header_, sizeof(header_), 0);\n> > if (ret < 0) {\n> > ret = -errno;\n> > LOG(IPCUnixSocket, Error)\n> > @@ -331,7 +331,7 @@ void IPCUnixSocket::dataNotifier([[maybe_unused]]\n> EventNotifier *notifier)\n> > * readyRead signal. The notifier will be reenabled by the\n> receive()\n> > * method.\n> > */\n> > - struct pollfd fds = { fd_, POLLIN, 0 };\n> > + struct pollfd fds = { fd_.get(), POLLIN, 0 };\n> > ret = poll(&fds, 1, 0);\n> > if (ret < 0)\n> > return;\n> > diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp\n> > index 80157b34..9ca0467b 100644\n> > --- a/test/ipc/unixsocket.cpp\n> > +++ b/test/ipc/unixsocket.cpp\n> > @@ -359,11 +359,11 @@ protected:\n> >\n> > int run()\n> > {\n> > - int slavefd = ipc_.create();\n> > - if (slavefd < 0)\n> > + ScopedFD slavefd = ipc_.create();\n> > + if (!slavefd.isValid())\n> > return TestFail;\n> >\n> > - if (slaveStart(slavefd)) {\n> > + if (slaveStart(slavefd.release())) {\n>\n> Should the slaveStart() function take a ScopedFD && ?\n>\n>\nWhich is correct, slaveStart() should take the ownership of a file\ndescriptor, or the ownership should still be owned by a caller?\n\n-Hiro\n\n\n> > cerr << \"Failed to start slave\" << endl;\n> > return TestFail;\n> > }\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 883D4C320B\n\tfor ;\n\tMon, 7 Jun 2021 09:15:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C3EFC6892E;\n\tMon, 7 Jun 2021 11:15:44 +0200 (CEST)","from mail-ed1-x534.google.com (mail-ed1-x534.google.com\n\t[IPv6:2a00:1450:4864:20::534])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B995B68925\n\tfor ;\n\tMon, 7 Jun 2021 11:15:42 +0200 (CEST)","by mail-ed1-x534.google.com with SMTP id r11so19396386edt.13\n\tfor ;\n\tMon, 07 Jun 2021 02:15:42 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"blXMfZOA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=PTGR77VZOI+tmlvWVyvQubOye7MrexmeCWXEMcrUewc=;\n\tb=blXMfZOAFjEb/xBXeWWmXIIRcMBSJ5NaSyBn+zKbvppIad6MphykDdCMz8A7Y4QkuN\n\tt1cgHi9tBBCmUKMPn/q7rRW4GIchb03gJpaETwWK9KhJMY+mTcl2Pn+aqvYxYDhdpdY3\n\tFNN9HsKb5BvS+4sARWXCYgngyumWtJUy6J0nA=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=PTGR77VZOI+tmlvWVyvQubOye7MrexmeCWXEMcrUewc=;\n\tb=aAT2Mxwd/Ru1x1Pub7ai+HzxBbREJvl4JeTUO4E4oiKJkBrmmr46oAWyrmbWxvjF2R\n\tmz7oxjG1/pa/MgmpTTsji9oA07kdcffHVqvIYoqcj22SMImK5bsxRDj6JY66z6zf/waf\n\tgG7t5dnkLrfJOhH+nIlbxanW6b3rebD5Kvy9K7KfBErgnVz+8EcO4GeSzyI3i/X8Asz1\n\tDa5sfafRx50fkKaQ7zeunj7zzWyqEy9j4O4ALuK2aLX3CW4N6pFpvf/7uXc5pcybPRrI\n\t+DL4nigGc+CRdpSpS2QsD/klJbGnxX9uPitYQYOT8rrLdZcULVdi8oHuMVjG+QxqOO4i\n\tGdDQ==","X-Gm-Message-State":"AOAM531zHd2d2b/y0SsFt3QJGD7eVD1/qSfs7ygVGWHFEdtRaircWSy7\n\tDeJBKbbEd1sJ45GNySw3NF+FEDn6gu5Tp6XqyMwsXw==","X-Google-Smtp-Source":"ABdhPJxP/KaCFx4M6eaWzycahe0FawrluuRnPUJCUOPtV/oqOAR1eTfLzoLkaBgYMa3QS/0hJGxq63EL+X+lnwLmNgA=","X-Received":"by 2002:a50:afa3:: with SMTP id\n\th32mr18895056edd.202.1623057342284; \n\tMon, 07 Jun 2021 02:15:42 -0700 (PDT)","MIME-Version":"1.0","References":"<20210415083843.3399502-1-hiroh@chromium.org>\n\t<20210415083843.3399502-8-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 7 Jun 2021 18:15:31 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"00000000000056567205c4297ce6\"","Subject":"Re: [libcamera-devel] [RFC PATCH 08/10] libcamera: IPCUnixSocket:\n\tUse ScopedFD for a file descriptor","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17436,"web_url":"https://patchwork.libcamera.org/comment/17436/","msgid":"","date":"2021-06-07T09:58:09","subject":"Re: [libcamera-devel] [RFC PATCH 08/10] libcamera: IPCUnixSocket:\n\tUse ScopedFD for a file descriptor","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Mon, Jun 7, 2021 at 6:15 PM Hirokazu Honda wrote:\n\n> Hi Laurent,\n>\n> On Mon, Jun 7, 2021 at 3:30 AM Laurent Pinchart <\n> laurent.pinchart@ideasonboard.com> wrote:\n>\n>> Hi Hiro,\n>>\n>> Thank you for the patch.\n>>\n>> On Thu, Apr 15, 2021 at 05:38:41PM +0900, Hirokazu Honda wrote:\n>> > IPCUnixSocket::create() creates two file descriptors. One of\n>> > them is stored in IPCUnixSocket and the other is returned to a\n>> > caller. This clarifies the ownership using ScopedFD.\n>> >\n>> > Signed-off-by: Hirokazu Honda \n>> > ---\n>> > include/libcamera/internal/ipc_unixsocket.h | 5 +--\n>> > src/libcamera/ipc_pipe_unixsocket.cpp | 8 ++---\n>> > src/libcamera/ipc_unixsocket.cpp | 40 ++++++++++-----------\n>> > test/ipc/unixsocket.cpp | 6 ++--\n>> > 4 files changed, 30 insertions(+), 29 deletions(-)\n>> >\n>> > diff --git a/include/libcamera/internal/ipc_unixsocket.h\n>> b/include/libcamera/internal/ipc_unixsocket.h\n>> > index e871b650..c06fee47 100644\n>> > --- a/include/libcamera/internal/ipc_unixsocket.h\n>> > +++ b/include/libcamera/internal/ipc_unixsocket.h\n>> > @@ -12,6 +12,7 @@\n>> > #include \n>> > #include \n>> >\n>> > +#include \n>> > #include \n>> >\n>> > namespace libcamera {\n>> > @@ -29,7 +30,7 @@ public:\n>> > IPCUnixSocket();\n>> > ~IPCUnixSocket();\n>> >\n>> > - int create();\n>> > + ScopedFD create();\n>> > int bind(int fd);\n>> > void close();\n>> > bool isBound() const;\n>> > @@ -50,7 +51,7 @@ private:\n>> >\n>> > void dataNotifier(EventNotifier *notifier);\n>> >\n>> > - int fd_;\n>> > + ScopedFD fd_;\n>> > bool headerReceived_;\n>> > struct Header header_;\n>> > EventNotifier *notifier_;\n>> > diff --git a/src/libcamera/ipc_pipe_unixsocket.cpp\n>> b/src/libcamera/ipc_pipe_unixsocket.cpp\n>> > index db0e260f..1997643f 100644\n>> > --- a/src/libcamera/ipc_pipe_unixsocket.cpp\n>> > +++ b/src/libcamera/ipc_pipe_unixsocket.cpp\n>> > @@ -30,14 +30,14 @@ IPCPipeUnixSocket::IPCPipeUnixSocket(const char\n>> *ipaModulePath,\n>> > args.push_back(ipaModulePath);\n>> >\n>> > socket_ = std::make_unique();\n>> > - int fd = socket_->create();\n>> > - if (fd < 0) {\n>> > + ScopedFD fd = socket_->create();\n>> > + if (!fd.isValid()) {\n>> > LOG(IPCPipe, Error) << \"Failed to create socket\";\n>> > return;\n>> > }\n>> > socket_->readyRead.connect(this, &IPCPipeUnixSocket::readyRead);\n>> > - args.push_back(std::to_string(fd));\n>> > - fds.push_back(fd);\n>> > + args.push_back(std::to_string(fd.get()));\n>> > + fds.push_back(fd.release());\n>> >\n>> > proc_ = std::make_unique();\n>> > int ret = proc_->start(ipaProxyWorkerPath, args, fds);\n>> > diff --git a/src/libcamera/ipc_unixsocket.cpp\n>> b/src/libcamera/ipc_unixsocket.cpp\n>> > index fdb359f7..2da43188 100644\n>> > --- a/src/libcamera/ipc_unixsocket.cpp\n>> > +++ b/src/libcamera/ipc_unixsocket.cpp\n>> > @@ -68,7 +68,7 @@ LOG_DEFINE_CATEGORY(IPCUnixSocket)\n>> > */\n>> >\n>> > IPCUnixSocket::IPCUnixSocket()\n>> > - : fd_(-1), headerReceived_(false), notifier_(nullptr)\n>> > + : headerReceived_(false), notifier_(nullptr)\n>> > {\n>> > }\n>> >\n>> > @@ -86,9 +86,9 @@ IPCUnixSocket::~IPCUnixSocket()\n>> > * the remote process, where it can be used with IPCUnixSocket::bind()\n>> to bind\n>> > * the remote side socket.\n>> > *\n>> > - * \\return A file descriptor on success, negative error code on failure\n>> > + * \\return A file descriptor. It is valid on success or invalid\n>> otherwise.\n>> > */\n>> > -int IPCUnixSocket::create()\n>> > +ScopedFD IPCUnixSocket::create()\n>> > {\n>> > int sockets[2];\n>> > int ret;\n>> > @@ -98,14 +98,13 @@ int IPCUnixSocket::create()\n>> > ret = -errno;\n>> > LOG(IPCUnixSocket, Error)\n>> > << \"Failed to create socket pair: \" <<\n>> strerror(-ret);\n>> > - return ret;\n>> > + return ScopedFD();\n>> > }\n>> >\n>> > - ret = bind(sockets[0]);\n>> > - if (ret)\n>> > - return ret;\n>> > + if (bind(sockets[0]) < 0)\n>> > + return ScopedFD();\n>> >\n>> > - return sockets[1];\n>> > + return ScopedFD(sockets[1]);\n>> > }\n>> >\n>> > /**\n>> > @@ -116,15 +115,18 @@ int IPCUnixSocket::create()\n>> > * by the file descriptor \\a fd. The file descriptor is obtained from\n>> the\n>> > * IPCUnixSocket::create() method.\n>> > *\n>> > - * \\return 0 on success or a negative error code otherwise\n>> > + * \\return 0 on success or a negative error code otherwise.\n>> > + *\n>> > + * \\todo This argument should be ScopedFD because bind() takes over the\n>> > + * ownership.\n>>\n>> Could you address this ?\n>>\n>>\nI tried to address this and recalled that I gave up doing this right now\nbecause some generated code in src/libcamera/proxy/worker/ uses the\nfunction with a numerical integer.\n\n-Hiro\n\n\n> > */\n>> > int IPCUnixSocket::bind(int fd)\n>> > {\n>> > if (isBound())\n>> > return -EINVAL;\n>> >\n>> > - fd_ = fd;\n>> > - notifier_ = new EventNotifier(fd_, EventNotifier::Read);\n>> > + fd_ = ScopedFD(fd);\n>> > + notifier_ = new EventNotifier(fd_.get(), EventNotifier::Read);\n>> > notifier_->activated.connect(this, &IPCUnixSocket::dataNotifier);\n>> >\n>> > return 0;\n>> > @@ -143,9 +145,7 @@ void IPCUnixSocket::close()\n>> > delete notifier_;\n>> > notifier_ = nullptr;\n>> >\n>> > - ::close(fd_);\n>> > -\n>> > - fd_ = -1;\n>> > + fd_.reset();\n>> > headerReceived_ = false;\n>> > }\n>> >\n>> > @@ -155,7 +155,7 @@ void IPCUnixSocket::close()\n>> > */\n>> > bool IPCUnixSocket::isBound() const\n>> > {\n>> > - return fd_ != -1;\n>> > + return fd_.isValid();\n>> > }\n>> >\n>> > /**\n>> > @@ -182,7 +182,7 @@ int IPCUnixSocket::send(const Payload &payload)\n>> > if (!hdr.data && !hdr.fds)\n>> > return -EINVAL;\n>> >\n>> > - ret = ::send(fd_, &hdr, sizeof(hdr), 0);\n>> > + ret = ::send(fd_.get(), &hdr, sizeof(hdr), 0);\n>> > if (ret < 0) {\n>> > ret = -errno;\n>> > LOG(IPCUnixSocket, Error)\n>> > @@ -262,7 +262,7 @@ int IPCUnixSocket::sendData(const void *buffer,\n>> size_t length,\n>> > msg.msg_flags = 0;\n>> > memcpy(CMSG_DATA(cmsg), fds, num * sizeof(uint32_t));\n>> >\n>> > - if (sendmsg(fd_, &msg, 0) < 0) {\n>> > + if (sendmsg(fd_.get(), &msg, 0) < 0) {\n>> > int ret = -errno;\n>> > LOG(IPCUnixSocket, Error)\n>> > << \"Failed to sendmsg: \" << strerror(-ret);\n>> > @@ -296,7 +296,7 @@ int IPCUnixSocket::recvData(void *buffer, size_t\n>> length,\n>> > msg.msg_controllen = cmsg->cmsg_len;\n>> > msg.msg_flags = 0;\n>> >\n>> > - if (recvmsg(fd_, &msg, 0) < 0) {\n>> > + if (recvmsg(fd_.get(), &msg, 0) < 0) {\n>> > int ret = -errno;\n>> > if (ret != -EAGAIN)\n>> > LOG(IPCUnixSocket, Error)\n>> > @@ -315,7 +315,7 @@ void IPCUnixSocket::dataNotifier([[maybe_unused]]\n>> EventNotifier *notifier)\n>> >\n>> > if (!headerReceived_) {\n>> > /* Receive the header. */\n>> > - ret = ::recv(fd_, &header_, sizeof(header_), 0);\n>> > + ret = ::recv(fd_.get(), &header_, sizeof(header_), 0);\n>> > if (ret < 0) {\n>> > ret = -errno;\n>> > LOG(IPCUnixSocket, Error)\n>> > @@ -331,7 +331,7 @@ void IPCUnixSocket::dataNotifier([[maybe_unused]]\n>> EventNotifier *notifier)\n>> > * readyRead signal. The notifier will be reenabled by the\n>> receive()\n>> > * method.\n>> > */\n>> > - struct pollfd fds = { fd_, POLLIN, 0 };\n>> > + struct pollfd fds = { fd_.get(), POLLIN, 0 };\n>> > ret = poll(&fds, 1, 0);\n>> > if (ret < 0)\n>> > return;\n>> > diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp\n>> > index 80157b34..9ca0467b 100644\n>> > --- a/test/ipc/unixsocket.cpp\n>> > +++ b/test/ipc/unixsocket.cpp\n>> > @@ -359,11 +359,11 @@ protected:\n>> >\n>> > int run()\n>> > {\n>> > - int slavefd = ipc_.create();\n>> > - if (slavefd < 0)\n>> > + ScopedFD slavefd = ipc_.create();\n>> > + if (!slavefd.isValid())\n>> > return TestFail;\n>> >\n>> > - if (slaveStart(slavefd)) {\n>> > + if (slaveStart(slavefd.release())) {\n>>\n>> Should the slaveStart() function take a ScopedFD && ?\n>>\n>>\n> Which is correct, slaveStart() should take the ownership of a file\n> descriptor, or the ownership should still be owned by a caller?\n>\n> -Hiro\n>\n>\n>> > cerr << \"Failed to start slave\" << endl;\n>> > return TestFail;\n>> > }\n>>\n>> --\n>> Regards,\n>>\n>> Laurent Pinchart\n>>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 798C3C3206\n\tfor ;\n\tMon, 7 Jun 2021 09:58:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D4B1B6892C;\n\tMon, 7 Jun 2021 11:58:22 +0200 (CEST)","from mail-ed1-x52a.google.com (mail-ed1-x52a.google.com\n\t[IPv6:2a00:1450:4864:20::52a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A53B568925\n\tfor ;\n\tMon, 7 Jun 2021 11:58:20 +0200 (CEST)","by mail-ed1-x52a.google.com with SMTP id f5so14578640eds.0\n\tfor ;\n\tMon, 07 Jun 2021 02:58:20 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"biimKgWx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=T1idkJ0lFbCo84+kVnKVUv+fR2ZujRmxD0ib5Gmtmsw=;\n\tb=biimKgWxzYiwAnmM9qcNt18mfyBDLUU0bnx6x6xYoF/88FlgeUGUoJ2YZiugBa31mq\n\tTeK+9o2xKU+ugdVwuwrVnLj8FFbVNi0lTtRg72Dt5Y1ziSsm0kErLUP1s77S4wRvr2zz\n\tKpywxKz6u88dZYfRkBM1xxklSNk2xqhzDtkDg=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=T1idkJ0lFbCo84+kVnKVUv+fR2ZujRmxD0ib5Gmtmsw=;\n\tb=pRC/VXYRl9hSIBCWG0wg3VU5oxFSdgedw5MsMLg/hIajIxnxbxd98/wl6YtcCUOOWy\n\tqEwPdTUiEtzOqb/FdlttrVXInFiUDKcPLOlgtfo4xHiz1GbqujrFiq7K/qlgdbip+f09\n\tzYgsEy+p2uktq+d/R0+P7ZoE2qwQvloLyC8OxowAUgpHU01MT5/Xiqjxw3W3qxnT3PbH\n\t7nZuAGqhIzjoZUSUEBpqC6pu1PH/6GAX1KXPXhh1CtMcQaZxkEG8mrHCO7mj0uEiGwiV\n\tp+08fCKMhKGnE6Rfu62h+usZyrEaBtJfnDGo5hW6JjdoqSKhc6teme846Jll8GF8kTKa\n\tATFQ==","X-Gm-Message-State":"AOAM533oOLipyK1UIfIHAABNH4dR4HQAh2fBxEaZSD248j2R37lvUMen\n\tS/DtjEasQ/BSze+7Rb5mKHHUiqfJPvUgSEmJah1fCDwPbooSsg==","X-Google-Smtp-Source":"ABdhPJwFxuJHjYASNfuHAjLCgZ/4YXkD0vR8hQ9TbdRtgHuLwaMCdF4ZzmPEXWcEsepAizDDu11FaaHSqXq6hHi+kGw=","X-Received":"by 2002:a05:6402:524b:: with SMTP id\n\tt11mr19218634edd.327.1623059900237; \n\tMon, 07 Jun 2021 02:58:20 -0700 (PDT)","MIME-Version":"1.0","References":"<20210415083843.3399502-1-hiroh@chromium.org>\n\t<20210415083843.3399502-8-hiroh@chromium.org>\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 7 Jun 2021 18:58:09 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000cd9e1905c42a149a\"","Subject":"Re: [libcamera-devel] [RFC PATCH 08/10] libcamera: IPCUnixSocket:\n\tUse ScopedFD for a file descriptor","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]