[libcamera-devel] [RFC PATCH 06/10] libcamera: V4L2Device: Use ScopedFD for a file descriptor

[Hirokazu Honda]

Hi Laurent,

On Mon, Jun 28, 2021 at 11:50 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> 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.
>

You're right. If we think about the overhead, we should avoid this
pass-by-value.
I generally pass-by-value to clarify the ownership. There is no chance
that a caller's ScopedFD is valid when the function returns.
With pass-by-rvalue-reference, if a function doesn't invalidate the
ScopedFD, it can still be valid when the function returns.
I think that's why Google C++ style guide suggests rvalue reference in
specific functions only.
https://google.github.io/styleguide/cppguide.html#Rvalue_references
Although setFd can be regarded as "&&-qualified methods that logically
"consume" *this".
I am fine to use rvalue reference here as setFd should consume the
passed ScopedFD.
What do you think?

-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