| Message ID | 20211130033820.18235-23-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Rejected |
| Delegated to: | Laurent Pinchart |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On Tue, Nov 30, 2021 at 12:39 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > 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@ideasonboard.com> > --- > I'm not entirely sure about this patch. If desired, it will get squashed > with the one introducing UniqueFD. > --- I am not sure either.. I would like to hear others' opinions although I am not against this. > 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)); > } > > if (isValidFd(fd_)) { > -- > Regards, > > Laurent Pinchart >
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@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 >
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@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_)) {
Quoting Laurent Pinchart (2021-12-03 16:32:52) > 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@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. Ok, it wasn't obvious from the context of the patch, or the short scope. Maybe documenting the intentions would help: /* * Ensure that the UniqueFD closes the file handle. * * We need to work from a copy of the file handle so we can keep the * fd_ number to validate it was closed at the system level while the * construction parameter will be correctly re-assigned to -1. */ > > > > } > > > > > > if (isValidFd(fd_)) { > > -- > Regards, > > Laurent Pinchart
On Fri, Dec 03, 2021 at 06:32:53PM +0200, Laurent Pinchart wrote: > 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@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. I've discussed this a bit further on #c++-general on liberachat, comparing UniqueFD to std::unique_ptr<> and pondering about why the std::unique_ptr<> constructor that takes a pointer doesn't instead take an rvalue reference to the pointer, and set it to null. Three different people didn't like the idea of the int && argument: - [It's] probably not a good idea to modify the caller. Consider the original int fd to be a "non-owning reference" much like a T* would be. Also it's occasionally useful to _use_ the original pointer anyway, rather than trying to juggle the unique_ptr and call .get(). e.g. you want to actually return a T*/T& to the resource you created, but you're owning it internally, and you created it someplace slightly incovenient (e.g. a map or similar). Similarly you may want to have-owned an fd but still return its value for select() or something. tldr, "own" doesn't mean "hide". - [I] would copy what unique_ptr does. When you use that ctor of unique_ptr you're basically saying "yes, I gurantee that this pointer can now be owned by you". It's up to the programmer to ensure that that's correct. - I would just have the ctor take an int and make the ctor explicit. If someone's using your class, they should know what it's for. The constructor is already explicit. I think I will get the rest of this series integrated without this patch, which we can discuss separately and merge on top of desired. > > 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_)) {
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)); } if (isValidFd(fd_)) {
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@ideasonboard.com> --- I'm not entirely sure about this patch. If desired, it will get squashed with the one introducing UniqueFD. --- 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(-)