[libcamera-devel] [RFC PATCH 05/10] libcamera: MediaDevice: Manage fd by ScopedFD

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Thu, Apr 15, 2021 at 05:38:38PM +0900, Hirokazu Honda wrote:
> MediaDevice owns a file descriptor for a media device node. It
> should be managed by ScopedFD to avoid the leakage.
> 
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  include/libcamera/internal/media_device.h |  3 ++-
>  src/libcamera/media_device.cpp            | 33 ++++++++++-------------
>  2 files changed, 16 insertions(+), 20 deletions(-)
> 
> diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h
> index c3292508..efdfbf36 100644
> --- a/include/libcamera/internal/media_device.h
> +++ b/include/libcamera/internal/media_device.h
> @@ -14,6 +14,7 @@
>  
>  #include <linux/media.h>
>  
> +#include <libcamera/scoped_file_descriptor.h>
>  #include <libcamera/signal.h>
>  
>  #include "libcamera/internal/log.h"
> @@ -82,7 +83,7 @@ private:
>  	unsigned int version_;
>  	unsigned int hwRevision_;
>  
> -	int fd_;
> +	ScopedFD fd_;
>  	bool valid_;
>  	bool acquired_;
>  	bool lockOwner_;
> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> index 9ec84e56..ad5efbf6 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,20 @@ int MediaDevice::disableLinks()
>   */
>  int MediaDevice::open()
>  {
> -	if (fd_ != -1) {
> +	if (!fd_.isValid()) {

This should be

	if (fd_.isValid()) {

>  		LOG(MediaDevice, Error) << "MediaDevice already open";
>  		return -EBUSY;
>  	}
>  
>  	int ret = ::open(deviceNode_.c_str(), O_RDWR);
>  	if (ret < 0) {
> -		ret = -errno;

This doesn't seem correct.

>  		LOG(MediaDevice, Error)
>  			<< "Failed to open media device at "
>  			<< deviceNode_ << ": " << strerror(-ret);
>  		return ret;
>  	}
> -	fd_ = ret;
> +
> +	fd_ = ScopedFD(ret);
>  
>  	return 0;
>  }
> @@ -514,11 +513,7 @@ int MediaDevice::open()
>   */
>  void MediaDevice::close()
>  {
> -	if (fd_ == -1)
> -		return;
> -
> -	::close(fd_);
> -	fd_ = -1;
> +	fd_.reset();
>  }
>  
>  /**
> @@ -763,7 +758,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)
> @@ -806,7 +801,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