[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 18:05:37 CET 2021
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 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.
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_)) {
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list