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

[Laurent Pinchart]

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.

> 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