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

[Jacopo Mondi]

Hi Laurent

On Mon, Nov 29, 2021 at 01:57:47AM +0200, Laurent Pinchart 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: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j

> ---
> 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
>