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

11943 diff mbox series

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@chromium.org>
---
 include/libcamera/internal/v4l2_videodevice.h |  4 ++--
 src/libcamera/v4l2_videodevice.cpp            | 21 ++++++++-----------
 2 files changed, 11 insertions(+), 14 deletions(-)

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@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;
>  }