<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Jun 7, 2021 at 3:30 AM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Hiro,<br>
<br>
Thank you for the patch.<br>
<br>
On Thu, Apr 15, 2021 at 05:38:41PM +0900, Hirokazu Honda wrote:<br>
> IPCUnixSocket::create() creates two file descriptors. One of<br>
> them is stored in IPCUnixSocket and the other is returned to a<br>
> caller. This clarifies the ownership using ScopedFD.<br>
> <br>
> Signed-off-by: Hirokazu Honda <<a href="mailto:hiroh@chromium.org" target="_blank">hiroh@chromium.org</a>><br>
> ---<br>
>  include/libcamera/internal/ipc_unixsocket.h |  5 +--<br>
>  src/libcamera/ipc_pipe_unixsocket.cpp  Â  Â  Â |  8 ++---<br>
>  src/libcamera/ipc_unixsocket.cpp  Â  Â  Â  Â  Â  | 40 ++++++++++-----------<br>
>  test/ipc/unixsocket.cpp  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â |  6 ++--<br>
>  4 files changed, 30 insertions(+), 29 deletions(-)<br>
> <br>
> diff --git a/include/libcamera/internal/ipc_unixsocket.h b/include/libcamera/internal/ipc_unixsocket.h<br>
> index e871b650..c06fee47 100644<br>
> --- a/include/libcamera/internal/ipc_unixsocket.h<br>
> +++ b/include/libcamera/internal/ipc_unixsocket.h<br>
> @@ -12,6 +12,7 @@<br>
>  #include <sys/types.h><br>
>  #include <vector><br>
>  <br>
> +#include <libcamera/scoped_file_descriptor.h><br>
>  #include <libcamera/signal.h><br>
>  <br>
>  namespace libcamera {<br>
> @@ -29,7 +30,7 @@ public:<br>
>  Â  Â  Â IPCUnixSocket();<br>
>  Â  Â  Â ~IPCUnixSocket();<br>
>  <br>
> -  Â  Â int create();<br>
> +  Â  Â ScopedFD create();<br>
>  Â  Â  Â int bind(int fd);<br>
>  Â  Â  Â void close();<br>
>  Â  Â  Â bool isBound() const;<br>
> @@ -50,7 +51,7 @@ private:<br>
>  <br>
>  Â  Â  Â void dataNotifier(EventNotifier *notifier);<br>
>  <br>
> -  Â  Â int fd_;<br>
> +  Â  Â ScopedFD fd_;<br>
>  Â  Â  Â bool headerReceived_;<br>
>  Â  Â  Â struct Header header_;<br>
>  Â  Â  Â EventNotifier *notifier_;<br>
> diff --git a/src/libcamera/ipc_pipe_unixsocket.cpp b/src/libcamera/ipc_pipe_unixsocket.cpp<br>
> index db0e260f..1997643f 100644<br>
> --- a/src/libcamera/ipc_pipe_unixsocket.cpp<br>
> +++ b/src/libcamera/ipc_pipe_unixsocket.cpp<br>
> @@ -30,14 +30,14 @@ IPCPipeUnixSocket::IPCPipeUnixSocket(const char *ipaModulePath,<br>
>  Â  Â  Â args.push_back(ipaModulePath);<br>
>  <br>
>  Â  Â  Â socket_ = std::make_unique<IPCUnixSocket>();<br>
> -  Â  Â int fd = socket_->create();<br>
> -  Â  Â if (fd < 0) {<br>
> +  Â  Â ScopedFD fd = socket_->create();<br>
> +  Â  Â if (!fd.isValid()) {<br>
>  Â  Â  Â  Â  Â  Â  Â LOG(IPCPipe, Error) << "Failed to create socket";<br>
>  Â  Â  Â  Â  Â  Â  Â return;<br>
>  Â  Â  Â }<br>
>  Â  Â  Â socket_->readyRead.connect(this, &IPCPipeUnixSocket::readyRead);<br>
> -  Â  Â args.push_back(std::to_string(fd));<br>
> -  Â  Â fds.push_back(fd);<br>
> +  Â  Â args.push_back(std::to_string(fd.get()));<br>
> +  Â  Â fds.push_back(fd.release());<br>
>  <br>
>  Â  Â  Â proc_ = std::make_unique<Process>();<br>
>  Â  Â  Â int ret = proc_->start(ipaProxyWorkerPath, args, fds);<br>
> diff --git a/src/libcamera/ipc_unixsocket.cpp b/src/libcamera/ipc_unixsocket.cpp<br>
> index fdb359f7..2da43188 100644<br>
> --- a/src/libcamera/ipc_unixsocket.cpp<br>
> +++ b/src/libcamera/ipc_unixsocket.cpp<br>
> @@ -68,7 +68,7 @@ LOG_DEFINE_CATEGORY(IPCUnixSocket)<br>
>  Â */<br>
>  <br>
>  IPCUnixSocket::IPCUnixSocket()<br>
> -  Â  Â : fd_(-1), headerReceived_(false), notifier_(nullptr)<br>
> +  Â  Â : headerReceived_(false), notifier_(nullptr)<br>
>  {<br>
>  }<br>
>  <br>
> @@ -86,9 +86,9 @@ IPCUnixSocket::~IPCUnixSocket()<br>
>  Â * the remote process, where it can be used with IPCUnixSocket::bind() to bind<br>
>  Â * the remote side socket.<br>
>  Â *<br>
> - * \return A file descriptor on success, negative error code on failure<br>
> + * \return A file descriptor. It is valid on success or invalid otherwise.<br>
>  Â */<br>
> -int IPCUnixSocket::create()<br>
> +ScopedFD IPCUnixSocket::create()<br>
>  {<br>
>  Â  Â  Â int sockets[2];<br>
>  Â  Â  Â int ret;<br>
> @@ -98,14 +98,13 @@ int IPCUnixSocket::create()<br>
>  Â  Â  Â  Â  Â  Â  Â ret = -errno;<br>
>  Â  Â  Â  Â  Â  Â  Â LOG(IPCUnixSocket, Error)<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â << "Failed to create socket pair: " << strerror(-ret);<br>
> -  Â  Â  Â  Â  Â  Â return ret;<br>
> +  Â  Â  Â  Â  Â  Â return ScopedFD();<br>
>  Â  Â  Â }<br>
>  <br>
> -  Â  Â ret = bind(sockets[0]);<br>
> -  Â  Â if (ret)<br>
> -  Â  Â  Â  Â  Â  Â return ret;<br>
> +  Â  Â if (bind(sockets[0]) < 0)<br>
> +  Â  Â  Â  Â  Â  Â return ScopedFD();<br>
>  <br>
> -  Â  Â return sockets[1];<br>
> +  Â  Â return ScopedFD(sockets[1]);<br>
>  }<br>
>  <br>
>  /**<br>
> @@ -116,15 +115,18 @@ int IPCUnixSocket::create()<br>
>  Â * by the file descriptor \a fd. The file descriptor is obtained from the<br>
>  Â * IPCUnixSocket::create() method.<br>
>  Â *<br>
> - * \return 0 on success or a negative error code otherwise<br>
> + * \return 0 on success or a negative error code otherwise.<br>
> + *<br>
> + * \todo This argument should be ScopedFD because bind() takes over the<br>
> + * ownership.<br>
<br>
Could you address this ?<br>
<br>
>  Â */<br>
>  int IPCUnixSocket::bind(int fd)<br>
>  {<br>
>  Â  Â  Â if (isBound())<br>
>  Â  Â  Â  Â  Â  Â  Â return -EINVAL;<br>
>  <br>
> -  Â  Â fd_ = fd;<br>
> -  Â  Â notifier_ = new EventNotifier(fd_, EventNotifier::Read);<br>
> +  Â  Â fd_ = ScopedFD(fd);<br>
> +  Â  Â notifier_ = new EventNotifier(fd_.get(), EventNotifier::Read);<br>
>  Â  Â  Â notifier_->activated.connect(this, &IPCUnixSocket::dataNotifier);<br>
>  <br>
>  Â  Â  Â return 0;<br>
> @@ -143,9 +145,7 @@ void IPCUnixSocket::close()<br>
>  Â  Â  Â delete notifier_;<br>
>  Â  Â  Â notifier_ = nullptr;<br>
>  <br>
> -  Â  Â ::close(fd_);<br>
> -<br>
> -  Â  Â fd_ = -1;<br>
> +  Â  Â fd_.reset();<br>
>  Â  Â  Â headerReceived_ = false;<br>
>  }<br>
>  <br>
> @@ -155,7 +155,7 @@ void IPCUnixSocket::close()<br>
>  Â */<br>
>  bool IPCUnixSocket::isBound() const<br>
>  {<br>
> -  Â  Â return fd_ != -1;<br>
> +  Â  Â return fd_.isValid();<br>
>  }<br>
>  <br>
>  /**<br>
> @@ -182,7 +182,7 @@ int IPCUnixSocket::send(const Payload &payload)<br>
>  Â  Â  Â if (!hdr.data && !hdr.fds)<br>
>  Â  Â  Â  Â  Â  Â  Â return -EINVAL;<br>
>  <br>
> -  Â  Â ret = ::send(fd_, &hdr, sizeof(hdr), 0);<br>
> +  Â  Â ret = ::send(fd_.get(), &hdr, sizeof(hdr), 0);<br>
>  Â  Â  Â if (ret < 0) {<br>
>  Â  Â  Â  Â  Â  Â  Â ret = -errno;<br>
>  Â  Â  Â  Â  Â  Â  Â LOG(IPCUnixSocket, Error)<br>
> @@ -262,7 +262,7 @@ int IPCUnixSocket::sendData(const void *buffer, size_t length,<br>
>  Â  Â  Â msg.msg_flags = 0;<br>
>  Â  Â  Â memcpy(CMSG_DATA(cmsg), fds, num * sizeof(uint32_t));<br>
>  <br>
> -  Â  Â if (sendmsg(fd_, &msg, 0) < 0) {<br>
> +  Â  Â if (sendmsg(fd_.get(), &msg, 0) < 0) {<br>
>  Â  Â  Â  Â  Â  Â  Â int ret = -errno;<br>
>  Â  Â  Â  Â  Â  Â  Â LOG(IPCUnixSocket, Error)<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â << "Failed to sendmsg: " << strerror(-ret);<br>
> @@ -296,7 +296,7 @@ int IPCUnixSocket::recvData(void *buffer, size_t length,<br>
>  Â  Â  Â msg.msg_controllen = cmsg->cmsg_len;<br>
>  Â  Â  Â msg.msg_flags = 0;<br>
>  <br>
> -  Â  Â if (recvmsg(fd_, &msg, 0) < 0) {<br>
> +  Â  Â if (recvmsg(fd_.get(), &msg, 0) < 0) {<br>
>  Â  Â  Â  Â  Â  Â  Â int ret = -errno;<br>
>  Â  Â  Â  Â  Â  Â  Â if (ret != -EAGAIN)<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â LOG(IPCUnixSocket, Error)<br>
> @@ -315,7 +315,7 @@ void IPCUnixSocket::dataNotifier([[maybe_unused]] EventNotifier *notifier)<br>
>  <br>
>  Â  Â  Â if (!headerReceived_) {<br>
>  Â  Â  Â  Â  Â  Â  Â /* Receive the header. */<br>
> -  Â  Â  Â  Â  Â  Â ret = ::recv(fd_, &header_, sizeof(header_), 0);<br>
> +  Â  Â  Â  Â  Â  Â ret = ::recv(fd_.get(), &header_, sizeof(header_), 0);<br>
>  Â  Â  Â  Â  Â  Â  Â if (ret < 0) {<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â ret = -errno;<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â LOG(IPCUnixSocket, Error)<br>
> @@ -331,7 +331,7 @@ void IPCUnixSocket::dataNotifier([[maybe_unused]] EventNotifier *notifier)<br>
>  Â  Â  Â  * readyRead signal. The notifier will be reenabled by the receive()<br>
>  Â  Â  Â  * method.<br>
>  Â  Â  Â  */<br>
> -  Â  Â struct pollfd fds = { fd_, POLLIN, 0 };<br>
> +  Â  Â struct pollfd fds = { fd_.get(), POLLIN, 0 };<br>
>  Â  Â  Â ret = poll(&fds, 1, 0);<br>
>  Â  Â  Â if (ret < 0)<br>
>  Â  Â  Â  Â  Â  Â  Â return;<br>
> diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp<br>
> index 80157b34..9ca0467b 100644<br>
> --- a/test/ipc/unixsocket.cpp<br>
> +++ b/test/ipc/unixsocket.cpp<br>
> @@ -359,11 +359,11 @@ protected:<br>
>  <br>
>  Â  Â  Â int run()<br>
>  Â  Â  Â {<br>
> -  Â  Â  Â  Â  Â  Â int slavefd = ipc_.create();<br>
> -  Â  Â  Â  Â  Â  Â if (slavefd < 0)<br>
> +  Â  Â  Â  Â  Â  Â ScopedFD slavefd = ipc_.create();<br>
> +  Â  Â  Â  Â  Â  Â if (!slavefd.isValid())<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â return TestFail;<br>
>  <br>
> -  Â  Â  Â  Â  Â  Â if (slaveStart(slavefd)) {<br>
> +  Â  Â  Â  Â  Â  Â if (slaveStart(slavefd.release())) {<br>
<br>
Should the slaveStart() function take a ScopedFD && ?<br>
<br></blockquote><div><br></div><div>Which is correct, slaveStart() should take the ownership of a file descriptor, or the ownership should still be owned by a caller?</div><div><br></div><div>-Hiro</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â cerr << "Failed to start slave" << endl;<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â return TestFail;<br>
>  Â  Â  Â  Â  Â  Â  Â }<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>