[libcamera-devel] [PATCH v4 22/22] libcamera: base: unique_fd: Pass rvalue reference to constructor and reset()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Dec 3 17:32:52 CET 2021
Hi Kieran,
On Fri, Dec 03, 2021 at 04:24:07PM +0000, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2021-11-30 03:38:20)
> > To avoid the risk of using a file descriptor whose ownership has been
> > transferred to a UniqueFD instance, pass an rvalue reference to the
> > constructor taking an int, and to the reset() function. The fd argument
> > is set to -1 upon return, making incorrect usage of the file descriptor
> > impossible.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > I'm not entirely sure about this patch. If desired, it will get squashed
> > with the one introducing UniqueFD.
>
> I kind of like that it makes the ownership explicit. I wasn't sure about
> the requirement to add std::move() ... but that makes it clear I guess.
>
> If you don't provide the std::move() will the compiler warn/error?
UniqueFd fd(std::move(num));
UniqueFD fd(3);
fd.reset(std::move(num));
fd.reset(3);
should all compile, while
UniqueFd fd(num);
fd.reset(num);
should result in a compilation error.
> Also down in the testing, assigning to a new int, just so that one can
> be moved in now looks like a hack - so should either be removed or
> clarified in a comment.
>
> > ---
> > include/libcamera/base/unique_fd.h | 5 +++--
> > src/libcamera/base/shared_fd.cpp | 2 +-
> > src/libcamera/base/unique_fd.cpp | 10 ++++++++--
> > src/libcamera/ipc_unixsocket.cpp | 4 ++--
> > src/libcamera/pipeline/raspberrypi/dma_heaps.cpp | 15 ++++++---------
> > src/libcamera/process.cpp | 4 ++--
> > src/libcamera/v4l2_videodevice.cpp | 2 +-
> > test/unique-fd.cpp | 10 ++++++----
> > 8 files changed, 29 insertions(+), 23 deletions(-)
> >
> > diff --git a/include/libcamera/base/unique_fd.h b/include/libcamera/base/unique_fd.h
> > index ae4d96b75797..a5ffb1a435dc 100644
> > --- a/include/libcamera/base/unique_fd.h
> > +++ b/include/libcamera/base/unique_fd.h
> > @@ -22,9 +22,10 @@ public:
> > {
> > }
> >
> > - explicit UniqueFD(int fd)
> > + explicit UniqueFD(int &&fd)
> > : fd_(fd)
> > {
> > + fd = -1;
> > }
> >
> > UniqueFD(UniqueFD &&other)
> > @@ -50,7 +51,7 @@ public:
> > return fd;
> > }
> >
> > - void reset(int fd = -1);
> > + void reset(int &&fd = -1);
> >
> > void swap(UniqueFD &other)
> > {
> > diff --git a/src/libcamera/base/shared_fd.cpp b/src/libcamera/base/shared_fd.cpp
> > index 0c8b93a47f85..8aecd34038bd 100644
> > --- a/src/libcamera/base/shared_fd.cpp
> > +++ b/src/libcamera/base/shared_fd.cpp
> > @@ -260,7 +260,7 @@ UniqueFD SharedFD::dup() const
> > << "Failed to dup() fd: " << strerror(-ret);
> > }
> >
> > - return UniqueFD(dupFd);
> > + return UniqueFD(std::move(dupFd));
> > }
> >
> > SharedFD::Descriptor::Descriptor(int fd, bool duplicate)
> > diff --git a/src/libcamera/base/unique_fd.cpp b/src/libcamera/base/unique_fd.cpp
> > index 83d6919cf623..7a961dbc01d0 100644
> > --- a/src/libcamera/base/unique_fd.cpp
> > +++ b/src/libcamera/base/unique_fd.cpp
> > @@ -38,9 +38,12 @@ LOG_DEFINE_CATEGORY(UniqueFD)
> > */
> >
> > /**
> > - * \fn UniqueFD::UniqueFD(int fd)
> > + * \fn UniqueFD::UniqueFD(int &&fd)
> > * \brief Construct a UniqueFD that owns \a fd
> > * \param[in] fd A file descriptor to manage
> > + *
> > + * Ownership of the file descriptor is transferred to the UniqueFD instance.
> > + * Upon return \a fd is set to -1.
> > */
> >
> > /**
> > @@ -88,11 +91,12 @@ LOG_DEFINE_CATEGORY(UniqueFD)
> > * \param[in] fd The new file descriptor to manage
> > *
> > * Close the managed file descriptor, if any, and replace it with the new \a fd.
> > + * Upon return \a fd is set to -1.
> > *
> > * Self-resetting (passing an \a fd already managed by this instance) is invalid
> > * and results in undefined behaviour.
> > */
> > -void UniqueFD::reset(int fd)
> > +void UniqueFD::reset(int &&fd)
> > {
> > ASSERT(!isValid() || fd != fd_);
> >
> > @@ -100,6 +104,8 @@ void UniqueFD::reset(int fd)
> >
> > if (fd >= 0)
> > close(fd);
> > +
> > + fd = -1;
> > }
> >
> > /**
> > diff --git a/src/libcamera/ipc_unixsocket.cpp b/src/libcamera/ipc_unixsocket.cpp
> > index 1980d374cea8..32bd6533d192 100644
> > --- a/src/libcamera/ipc_unixsocket.cpp
> > +++ b/src/libcamera/ipc_unixsocket.cpp
> > @@ -103,8 +103,8 @@ UniqueFD IPCUnixSocket::create()
> > }
> >
> > std::array<UniqueFD, 2> socketFds{
> > - UniqueFD(sockets[0]),
> > - UniqueFD(sockets[1]),
> > + UniqueFD(std::move(sockets[0])),
> > + UniqueFD(std::move(sockets[1])),
> > };
> >
> > if (bind(std::move(socketFds[0])) < 0)
> > diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp
> > index 69831dabbe75..f2a95300d187 100644
> > --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp
> > @@ -37,16 +37,13 @@ namespace RPi {
> > DmaHeap::DmaHeap()
> > {
> > for (const char *name : heapNames) {
> > - int ret = ::open(name, O_RDWR, 0);
> > - if (ret < 0) {
> > - ret = errno;
> > - LOG(RPI, Debug) << "Failed to open " << name << ": "
> > - << strerror(ret);
> > - continue;
> > - }
> > + dmaHeapHandle_ = UniqueFD(open(name, O_RDWR, 0));
> > + if (dmaHeapHandle_.isValid())
> > + break;
> >
> > - dmaHeapHandle_ = UniqueFD(ret);
> > - break;
> > + int err = errno;
> > + LOG(RPI, Debug) << "Failed to open " << name << ": "
> > + << strerror(err);
> > }
> >
> > if (!dmaHeapHandle_.isValid())
> > diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp
> > index 0e6b4e1dde58..dc8576d934b6 100644
> > --- a/src/libcamera/process.cpp
> > +++ b/src/libcamera/process.cpp
> > @@ -134,8 +134,8 @@ ProcessManager::ProcessManager()
> > LOG(Process, Fatal)
> > << "Failed to initialize pipe for signal handling";
> >
> > - pipe_[0] = UniqueFD(pipe[0]);
> > - pipe_[1] = UniqueFD(pipe[1]);
> > + pipe_[0] = UniqueFD(std::move(pipe[0]));
> > + pipe_[1] = UniqueFD(std::move(pipe[1]));
> >
> > sigEvent_ = new EventNotifier(pipe_[0].get(), EventNotifier::Read);
> > sigEvent_->activated.connect(this, &ProcessManager::sighandler);
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index b4b89e2759b9..9d0d0077f4e4 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -1396,7 +1396,7 @@ UniqueFD V4L2VideoDevice::exportDmabufFd(unsigned int index,
> > return {};
> > }
> >
> > - return UniqueFD(expbuf.fd);
> > + return UniqueFD(std::move(expbuf.fd));
> > }
> >
> > /**
> > diff --git a/test/unique-fd.cpp b/test/unique-fd.cpp
> > index eb3b591fec28..7e94e3ca450e 100644
> > --- a/test/unique-fd.cpp
> > +++ b/test/unique-fd.cpp
> > @@ -39,7 +39,8 @@ protected:
> > }
> >
> > /* Test creating UniqueFD from numerical file descriptor. */
> > - UniqueFD fd2(fd_);
> > + int numFd = fd_;
> > + UniqueFD fd2(std::move(numFd));
> > if (!fd2.isValid() || fd2.get() != fd_) {
> > std::cout << "Failed fd check (fd constructor)"
> > << std::endl;
> > @@ -113,7 +114,7 @@ protected:
> > }
> >
> > /* Test release. */
> > - int numFd = fd2.release();
> > + numFd = fd2.release();
> > if (fd2.isValid() || fd2.get() != -1) {
> > std::cout << "Failed fd check (release)"
> > << std::endl;
> > @@ -133,7 +134,7 @@ protected:
> > }
> >
> > /* Test reset assignment. */
> > - fd.reset(numFd);
> > + fd.reset(std::move(numFd));
> > if (!fd.isValid() || fd.get() != fd_) {
> > std::cout << "Failed fd check (reset assignment)"
> > << std::endl;
> > @@ -168,7 +169,8 @@ protected:
> > }
> >
> > {
> > - UniqueFD fd4(fd_);
> > + numFd = fd_;
> > + UniqueFD fd4(std::move(numFd));
>
> This now looks like a hack ... Should it be removed? What is it really
> testing?
I need to keep the fd_ value valid, as I use it on the next line. When
fd4 gets out of scope, the destructor will close the file descriptor,
and isValid(fd_) should then fail. Without numFd = fd_, fd_ will then
become -1, which defeats the point of the test.
> > }
> >
> > if (isValidFd(fd_)) {
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list