[libcamera-devel] [PATCH v3 12/17] libcamera: media_device: Manage fd by UniqueFD

[Hirokazu Honda]

Hi Laurent,

On Mon, Nov 29, 2021 at 8:58 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> From: Hirokazu Honda <hiroh at chromium.org>
>
> Manages a file descriptor owned by MediaDevice for a media device
> node by UniqueFD.
>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Reviewed-by: Hirokazu Honda <hiroh at chromium.org> if applicable.
> ---
> Changes since v2:
>
> - Fix errno handling
> ---
>  include/libcamera/internal/media_device.h |  3 +-
>  src/libcamera/media_device.cpp            | 36 ++++++++++-------------
>  2 files changed, 17 insertions(+), 22 deletions(-)
>
> diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h
> index d636e34a8573..af0b25b731eb 100644
> --- a/include/libcamera/internal/media_device.h
> +++ b/include/libcamera/internal/media_device.h
> @@ -16,6 +16,7 @@
>
>  #include <libcamera/base/log.h>
>  #include <libcamera/base/signal.h>
> +#include <libcamera/base/unique_fd.h>
>
>  #include "libcamera/internal/media_object.h"
>
> @@ -82,7 +83,7 @@ private:
>         unsigned int version_;
>         unsigned int hwRevision_;
>
> -       int fd_;
> +       UniqueFD fd_;
>         bool valid_;
>         bool acquired_;
>         bool lockOwner_;
> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> index aa93da75c593..4df0a27fe193 100644
> --- a/src/libcamera/media_device.cpp
> +++ b/src/libcamera/media_device.cpp
> @@ -63,15 +63,14 @@ LOG_DEFINE_CATEGORY(MediaDevice)
>   * populate() before the media graph can be queried.
>   */
>  MediaDevice::MediaDevice(const std::string &deviceNode)
> -       : deviceNode_(deviceNode), fd_(-1), valid_(false), acquired_(false),
> +       : deviceNode_(deviceNode), valid_(false), acquired_(false),
>           lockOwner_(false)
>  {
>  }
>
>  MediaDevice::~MediaDevice()
>  {
> -       if (fd_ != -1)
> -               ::close(fd_);
> +       fd_.reset();
>         clear();
>  }
>
> @@ -143,14 +142,14 @@ void MediaDevice::release()
>   */
>  bool MediaDevice::lock()
>  {
> -       if (fd_ == -1)
> +       if (!fd_.isValid())
>                 return false;
>
>         /* Do not allow nested locking in the same libcamera instance. */
>         if (lockOwner_)
>                 return false;
>
> -       if (lockf(fd_, F_TLOCK, 0))
> +       if (lockf(fd_.get(), F_TLOCK, 0))
>                 return false;
>
>         lockOwner_ = true;
> @@ -169,7 +168,7 @@ bool MediaDevice::lock()
>   */
>  void MediaDevice::unlock()
>  {
> -       if (fd_ == -1)
> +       if (!fd_.isValid())
>                 return;
>
>         if (!lockOwner_)
> @@ -177,7 +176,7 @@ void MediaDevice::unlock()
>
>         lockOwner_ = false;
>
> -       lockf(fd_, F_ULOCK, 0);
> +       lockf(fd_.get(), F_ULOCK, 0);
>  }
>
>  /**
> @@ -220,7 +219,7 @@ int MediaDevice::populate()
>                 return ret;
>
>         struct media_device_info info = {};
> -       ret = ioctl(fd_, MEDIA_IOC_DEVICE_INFO, &info);
> +       ret = ioctl(fd_.get(), MEDIA_IOC_DEVICE_INFO, &info);
>         if (ret) {
>                 ret = -errno;
>                 LOG(MediaDevice, Error)
> @@ -243,7 +242,7 @@ int MediaDevice::populate()
>                 topology.ptr_links = reinterpret_cast<uintptr_t>(links);
>                 topology.ptr_pads = reinterpret_cast<uintptr_t>(pads);
>
> -               ret = ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology);
> +               ret = ioctl(fd_.get(), MEDIA_IOC_G_TOPOLOGY, &topology);
>                 if (ret < 0) {
>                         ret = -errno;
>                         LOG(MediaDevice, Error)
> @@ -481,20 +480,19 @@ int MediaDevice::disableLinks()
>   */
>  int MediaDevice::open()
>  {
> -       if (fd_ != -1) {
> +       if (fd_.isValid()) {
>                 LOG(MediaDevice, Error) << "MediaDevice already open";
>                 return -EBUSY;
>         }
>
> -       int ret = ::open(deviceNode_.c_str(), O_RDWR);
> -       if (ret < 0) {
> -               ret = -errno;
> +       fd_ = UniqueFD(::open(deviceNode_.c_str(), O_RDWR));
> +       if (!fd_.isValid()) {
> +               int ret = -errno;
>                 LOG(MediaDevice, Error)
>                         << "Failed to open media device at "
>                         << deviceNode_ << ": " << strerror(-ret);
>                 return ret;
>         }
> -       fd_ = ret;
>
>         return 0;
>  }
> @@ -514,11 +512,7 @@ int MediaDevice::open()
>   */
>  void MediaDevice::close()
>  {
> -       if (fd_ == -1)
> -               return;
> -
> -       ::close(fd_);
> -       fd_ = -1;
> +       fd_.reset();
>  }
>
>  /**
> @@ -756,7 +750,7 @@ void MediaDevice::fixupEntityFlags(struct media_v2_entity *entity)
>         struct media_entity_desc desc = {};
>         desc.id = entity->id;
>
> -       int ret = ioctl(fd_, MEDIA_IOC_ENUM_ENTITIES, &desc);
> +       int ret = ioctl(fd_.get(), MEDIA_IOC_ENUM_ENTITIES, &desc);
>         if (ret < 0) {
>                 ret = -errno;
>                 LOG(MediaDevice, Debug)
> @@ -799,7 +793,7 @@ int MediaDevice::setupLink(const MediaLink *link, unsigned int flags)
>
>         linkDesc.flags = flags;
>
> -       int ret = ioctl(fd_, MEDIA_IOC_SETUP_LINK, &linkDesc);
> +       int ret = ioctl(fd_.get(), MEDIA_IOC_SETUP_LINK, &linkDesc);
>         if (ret) {
>                 ret = -errno;
>                 LOG(MediaDevice, Error)
> --
> Regards,
>
> Laurent Pinchart
>