[libcamera-devel] [RFC PATCH 07/10] libcamera: V4L2VideoDevice: Use fd for a file descriptor

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Thu, Apr 15, 2021 at 05:38:40PM +0900, Hirokazu Honda wrote:
> V4L2VideoDevice deals with file descriptors. This uses ScopedFD
> for them to avoid the leakage. This also changes the return type
> of exportDmabufFd to ScopedFD from FileDescriptor in order to
> represent a caller owns the returned file file descriptor.
> 
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  include/libcamera/internal/v4l2_videodevice.h |  4 ++--
>  src/libcamera/v4l2_videodevice.cpp            | 21 ++++++++-----------
>  2 files changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index 7938343b..925a8e82 100644
> --- a/include/libcamera/internal/v4l2_videodevice.h
> +++ b/include/libcamera/internal/v4l2_videodevice.h
> @@ -30,9 +30,9 @@
>  namespace libcamera {
>  
>  class EventNotifier;
> -class FileDescriptor;
>  class MediaDevice;
>  class MediaEntity;
> +class ScopedFD;
>  
>  struct V4L2Capability final : v4l2_capability {
>  	const char *driver() const
> @@ -235,7 +235,7 @@ private:
>  	int createBuffers(unsigned int count,
>  			  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
>  	std::unique_ptr<FrameBuffer> createBuffer(unsigned int index);
> -	FileDescriptor exportDmabufFd(unsigned int index, unsigned int plane);
> +	ScopedFD exportDmabufFd(unsigned int index, unsigned int plane);
>  
>  	void bufferAvailable(EventNotifier *notifier);
>  	FrameBuffer *dequeueBuffer();
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 0bf3b5f5..fe311ed9 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -1278,12 +1278,12 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
>  
>  	std::vector<FrameBuffer::Plane> planes;
>  	for (unsigned int nplane = 0; nplane < numPlanes; nplane++) {
> -		FileDescriptor fd = exportDmabufFd(buf.index, nplane);
> +		ScopedFD fd = exportDmabufFd(buf.index, nplane);
>  		if (!fd.isValid())
>  			return nullptr;
>  
>  		FrameBuffer::Plane plane;
> -		plane.fd = std::move(fd);
> +		plane.fd = FileDescriptor(fd.release());
>  		plane.length = multiPlanar ?
>  			buf.m.planes[nplane].length : buf.length;
>  
> @@ -1293,7 +1293,7 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
>  	return std::make_unique<FrameBuffer>(std::move(planes));
>  }
>  
> -FileDescriptor V4L2VideoDevice::exportDmabufFd(unsigned int index,
> +ScopedFD V4L2VideoDevice::exportDmabufFd(unsigned int index,
>  					       unsigned int plane)
>  {
>  	struct v4l2_exportbuffer expbuf = {};
> @@ -1308,10 +1308,10 @@ FileDescriptor V4L2VideoDevice::exportDmabufFd(unsigned int index,
>  	if (ret < 0) {
>  		LOG(V4L2, Error)
>  			<< "Failed to export buffer: " << strerror(-ret);
> -		return FileDescriptor();
> +		return ScopedFD();
>  	}
>  
> -	return FileDescriptor(std::move(expbuf.fd));
> +	return ScopedFD(expbuf.fd);
>  }
>  
>  /**
> @@ -1703,7 +1703,6 @@ V4L2M2MDevice::~V4L2M2MDevice()
>   */
>  int V4L2M2MDevice::open()
>  {
> -	int fd;
>  	int ret;
>  
>  	/*
> @@ -1712,7 +1711,7 @@ int V4L2M2MDevice::open()
>  	 * as the V4L2VideoDevice::open() retains a handle by duplicating the
>  	 * fd passed in.
>  	 */
> -	fd = syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(),
> +	int fd = syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(),
>  		     O_RDWR | O_NONBLOCK);

Wrong indentation.

>  	if (fd < 0) {
>  		ret = -errno;
> @@ -1720,22 +1719,20 @@ int V4L2M2MDevice::open()
>  			<< "Failed to open V4L2 M2M device: " << strerror(-ret);
>  		return ret;
>  	}

Blank line.

> +	ScopedFD scopedFd(fd);
>  
> -	ret = output_->open(fd, V4L2_BUF_TYPE_VIDEO_OUTPUT);
> +	ret = output_->open(scopedFd.get(), V4L2_BUF_TYPE_VIDEO_OUTPUT);
>  	if (ret)
>  		goto err;
>  
> -	ret = capture_->open(fd, V4L2_BUF_TYPE_VIDEO_CAPTURE);
> +	ret = capture_->open(scopedFd.get(), V4L2_BUF_TYPE_VIDEO_CAPTURE);
>  	if (ret)
>  		goto err;
>  
> -	::close(fd);
> -
>  	return 0;
>  
>  err:
>  	close();
> -	::close(fd);
>  
>  	return ret;
>  }

-- 
Regards,

Laurent Pinchart