[libcamera-devel] [RFC PATCH 08/10] libcamera: IPCUnixSocket: Use ScopedFD for a file descriptor
Hirokazu Honda
hiroh at chromium.org
Mon Jun 7 11:58:09 CEST 2021
Hi Laurent,
On Mon, Jun 7, 2021 at 6:15 PM Hirokazu Honda <hiroh at chromium.org> wrote:
> Hi Laurent,
>
> On Mon, Jun 7, 2021 at 3:30 AM Laurent Pinchart <
> laurent.pinchart at ideasonboard.com> wrote:
>
>> 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 ?
>>
>>
I tried to address this and recalled that I gave up doing this right now
because some generated code in src/libcamera/proxy/worker/ uses the
function with a numerical integer.
-Hiro
> > */
>> > 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 && ?
>>
>>
> Which is correct, slaveStart() should take the ownership of a file
> descriptor, or the ownership should still be owned by a caller?
>
> -Hiro
>
>
>> > cerr << "Failed to start slave" << endl;
>> > return TestFail;
>> > }
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210607/2991bc15/attachment-0001.htm>
More information about the libcamera-devel
mailing list