[libcamera-devel,RFC,08/10] libcamera: IPCUnixSocket: Use ScopedFD for a file descriptor
diff mbox series

Message ID 20210415083843.3399502-8-hiroh@chromium.org
State Changes Requested
Headers show
Series
  • Untitled series #1933
Related show

Commit Message

Hirokazu Honda April 15, 2021, 8:38 a.m. UTC 
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@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(-)

Comments

Laurent Pinchart June 6, 2021, 6:30 p.m. UTC | #1 
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@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;
>  		}
Hirokazu Honda June 7, 2021, 9:15 a.m. UTC | #2 
Hi Laurent,

On Mon, Jun 7, 2021 at 3:30 AM Laurent Pinchart <
laurent.pinchart@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@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 && ?
>
>
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
>
Hirokazu Honda June 7, 2021, 9:58 a.m. UTC | #3 
Hi Laurent,

On Mon, Jun 7, 2021 at 6:15 PM Hirokazu Honda <hiroh@chromium.org> wrote:

> Hi Laurent,
>
> On Mon, Jun 7, 2021 at 3:30 AM Laurent Pinchart <
> laurent.pinchart@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@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
>>
>

Patch
diff mbox series 

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.
  */
 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())) {
 			cerr << "Failed to start slave" << endl;
 			return TestFail;
 		}