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

[Hirokazu Honda]

Hi Laurent,

On Mon, Jun 7, 2021 at 3:19 AM Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Hiro,
>
> Thank you for the patch.
>
> 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.
I would take a value as if ScopedFD is unique_ptr.

-Hiro

> >
> >       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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210607/4898162d/attachment-0001.htm>