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

[Laurent Pinchart]

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 ?

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