[libcamera-devel] [RFC PATCH 06/10] libcamera: V4L2Device: Use ScopedFD for a file descriptor
Hirokazu Honda
hiroh at chromium.org
Mon Nov 29 13:53:53 CET 2021
Hi Laurent,
On Mon, Nov 29, 2021 at 12:50 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Replying to an old e-mail,
>
> On Mon, Jun 28, 2021 at 05:49:56AM +0300, Laurent Pinchart wrote:
> > On Mon, Jun 07, 2021 at 12:34:00PM +0900, Hirokazu Honda wrote:
> > > On Mon, Jun 7, 2021 at 3:19 AM Laurent Pinchart wrote:
> > > > On Thu, Apr 15, 2021 at 05:38:39PM +0900, Hirokazu Honda wrote:
> > > > > V4L2Device owns a file descriptor for a v4l2 device node. It
> > > > > should be managed by ScopedFD avoid the leakage.
> > > > >
> > > > > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > > > > ---
> > > > > include/libcamera/internal/v4l2_device.h | 9 +++++----
> > > > > src/libcamera/v4l2_device.cpp | 17 +++++++----------
> > > > > src/libcamera/v4l2_videodevice.cpp | 3 ++-
> > > > > 3 files changed, 14 insertions(+), 15 deletions(-)
> > > > >
> > > > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> > > > > index d006bf68..e0262de0 100644
> > > > > --- a/include/libcamera/internal/v4l2_device.h
> > > > > +++ b/include/libcamera/internal/v4l2_device.h
> > > > > @@ -13,6 +13,7 @@
> > > > >
> > > > > #include <linux/videodev2.h>
> > > > >
> > > > > +#include <libcamera/scoped_file_descriptor.h>
> > > > > #include <libcamera/signal.h>
> > > > >
> > > > > #include "libcamera/internal/log.h"
> > > > > @@ -26,7 +27,7 @@ class V4L2Device : protected Loggable
> > > > > {
> > > > > public:
> > > > > void close();
> > > > > - bool isOpen() const { return fd_ != -1; }
> > > > > + bool isOpen() const { return fd_.isValid(); }
> > > > >
> > > > > const ControlInfoMap &controls() const { return controls_; }
> > > > >
> > > > > @@ -46,11 +47,11 @@ protected:
> > > > > ~V4L2Device();
> > > > >
> > > > > int open(unsigned int flags);
> > > > > - int setFd(int fd);
> > > > > + int setFd(ScopedFD fd);
> > > >
> > > > Should this take an rvalue reference to enable move semantics ?
> > >
> > > Since ScopedFD is move-only, when declaring ScopedFD, it forces a caller to
> > > pass with move semantics.
> >
> > This means that a new ScopedFD instance will be created using the move
> > constructor, to be passed to the setFd() function, which will internally
> > use the move assignment operator. We can avoid the move constructor by
> > passing an rvalue reference here. It would also clearly indicate to the
> > user that a move is the expected behaviour of setFd(), while this is
> > currently implicit only, as a consequence of ScopedFD deleting the copy
> > constructor.
>
> It recently came to my attention that passing a std::unique_ptr<> by
> rvalue reference is usually a bad idea. The class is cheap to move, and
> with a pass-by-value call, it's clear that move is being implemented as
> std::unique_ptr<> has no copy constructor. When passing by rvalue
> reference, the caller has to use std::move(), but it's up to the callee
> to actually perform the move. Compare the following:
>
> void foo(std::unique_ptr<Foo> ptr)
> {
> }
>
> void foo(std::unique_ptr<Foo> &&ptr)
> {
> }
>
> with foo being called as
>
> std::unique_ptr<Foo> f = ...;
> foo(std::move(f));
>
> In the first case, ptr is constructed by moving f. ptr gets destroyed at
> the end of foo(), and because foo() is empty, it still owns the pointer,
> which then gets deleted. When foo() returns, f doesn't own a pointer
> anymore.
>
> In the second case, f will still own the pointer after foo() returns. Of
> course, if foo() was implemented as
>
> void foo(std::unique_ptr<Foo> &&ptr)
> {
> globalPtr = std::move(ptr);
> }
>
> then f would not own the pointer after foo() returns. The point here is
> that passing a std::unique_ptr<> rvalue reference makes the behaviour
> more dependent on the implementation of the function, while passing a
> value ensures that f will not own the pointer anymore when foo()
> returns, regardless of how it's implemented.
>
> The same applies to ScopedFD.
>
> Of course, if the class was expensive to move, it would be a different
> cases.
>
You're right. I was told the same thing a few years ago by my
colleague that an argument must be a value if the function makes sure
to take the ownership.
Otherwise, a destructor of passed value by std::move() can be called
in the end of scope in the caller side.
-Hiro
> > > I would take a value as if ScopedFD is unique_ptr.
> > >
> > > > >
> > > > > int ioctl(unsigned long request, void *argp);
> > > > >
> > > > > - int fd() const { return fd_; }
> > > > > + int fd() const { return fd_.get(); }
> > > > >
> > > > > private:
> > > > > void listControls();
> > > > > @@ -64,7 +65,7 @@ private:
> > > > > std::vector<std::unique_ptr<V4L2ControlId>> controlIds_;
> > > > > ControlInfoMap controls_;
> > > > > std::string deviceNode_;
> > > > > - int fd_;
> > > > > + ScopedFD fd_;
> > > > >
> > > > > EventNotifier *fdEventNotifier_;
> > > > > bool frameStartEnabled_;
> > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > > > > index decd19ef..4fbb2d60 100644
> > > > > --- a/src/libcamera/v4l2_device.cpp
> > > > > +++ b/src/libcamera/v4l2_device.cpp
> > > > > @@ -53,7 +53,7 @@ LOG_DEFINE_CATEGORY(V4L2)
> > > > > * at open() time, and the \a logTag to prefix log messages with.
> > > > > */
> > > > > V4L2Device::V4L2Device(const std::string &deviceNode)
> > > > > - : deviceNode_(deviceNode), fd_(-1), fdEventNotifier_(nullptr),
> > > > > + : deviceNode_(deviceNode), fdEventNotifier_(nullptr),
> > > > > frameStartEnabled_(false)
> > > > > {
> > > > > }
> > > > > @@ -89,7 +89,7 @@ int V4L2Device::open(unsigned int flags)
> > > > > return ret;
> > > > > }
> > > > >
> > > > > - setFd(ret);
> > > > > + setFd(ScopedFD(ret));
> > > > >
> > > > > listControls();
> > > > >
> > > > > @@ -112,14 +112,14 @@ int V4L2Device::open(unsigned int flags)
> > > > > *
> > > > > * \return 0 on success or a negative error code otherwise
> > > > > */
> > > > > -int V4L2Device::setFd(int fd)
> > > > > +int V4L2Device::setFd(ScopedFD fd)
> > > > > {
> > > > > if (isOpen())
> > > > > return -EBUSY;
> > > > >
> > > > > - fd_ = fd;
> > > > > + fd_ = std::move(fd);
> > > > >
> > > > > - fdEventNotifier_ = new EventNotifier(fd_, EventNotifier::Exception);
> > > > > + fdEventNotifier_ = new EventNotifier(fd_.get(), EventNotifier::Exception);
> > > > > fdEventNotifier_->activated.connect(this, &V4L2Device::eventAvailable);
> > > > > fdEventNotifier_->setEnabled(false);
> > > > >
> > > > > @@ -138,10 +138,7 @@ void V4L2Device::close()
> > > > >
> > > > > delete fdEventNotifier_;
> > > > >
> > > > > - if (::close(fd_) < 0)
> > > > > - LOG(V4L2, Error) << "Failed to close V4L2 device: "
> > > > > - << strerror(errno);
> > > > > - fd_ = -1;
> > > > > + fd_.reset();
> > > > > }
> > > > >
> > > > > /**
> > > > > @@ -449,7 +446,7 @@ int V4L2Device::ioctl(unsigned long request, void *argp)
> > > > > * Printing out an error message is usually better performed
> > > > > * in the caller, which can provide more context.
> > > > > */
> > > > > - if (::ioctl(fd_, request, argp) < 0)
> > > > > + if (::ioctl(fd_.get(), request, argp) < 0)
> > > > > return -errno;
> > > > >
> > > > > return 0;
> > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > > > index 12c09dc7..0bf3b5f5 100644
> > > > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > > > @@ -22,6 +22,7 @@
> > > > > #include <linux/version.h>
> > > > >
> > > > > #include <libcamera/file_descriptor.h>
> > > > > +#include <libcamera/scoped_file_descriptor.h>
> > > > >
> > > > > #include "libcamera/internal/event_notifier.h"
> > > > > #include "libcamera/internal/log.h"
> > > > > @@ -614,7 +615,7 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)
> > > > > return ret;
> > > > > }
> > > > >
> > > > > - ret = V4L2Device::setFd(newFd);
> > > > > + ret = V4L2Device::setFd(ScopedFD(newFd));
> > > > > if (ret < 0) {
> > > > > LOG(V4L2, Error) << "Failed to set file handle: "
> > > > > << strerror(-ret);
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list