[libcamera-devel] [PATCH v4 22/22] libcamera: base: unique_fd: Pass rvalue reference to constructor and reset()
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Dec 3 17:24:07 CET 2021
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?
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?
> }
>
> if (isValidFd(fd_)) {
> --
> Regards,
>
> Laurent Pinchart
>
More information about the libcamera-devel
mailing list