[libcamera-devel] [PATCH 1/2] libcamera: v4l2_device: Close Plane dmabuf fd

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Mon, Mar 04, 2019 at 11:25:29PM +0000, Kieran Bingham wrote:
> When constructing a Plane, the exported buffer provides a dmabuf handle which
> is set to the Plane object.
> 
> This action duplicates the handle for internal storage, and the original fd is
> not used and needs to be closed.
> 
> Close the handle, ensuring that the resources can be correctly managed.

Good catch !

The fact that this problem went unnoticed makes me wonder if we have a
problem with the API. Would there be a way to make it more robust,
preventing these errors from happening ? We could take ownership of the
fd passed to the setDmabuf() function, requiring the caller to call
dup() if needed, but that would create a different issue, equally bad in
my opinion. We could however change the setDmabuf() prototype in that case

int Plane::setDmabuf(int &&fd, unsigned int length);

which would require the caller to use std::move(). Pros, the caller
would be forced to think about it, cons, the caller may just add an
std::move() to fix the compilation error without actually thinking about
it (the default move constructor for int doesn't modify the value of the
other instance). I thus wonder if it's worth it, or if we'll have to
live with this. Or if there's another way I'm not thinking about right
now.

In any case,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> Fixes: 771befc6dc0e ("libcamera: v4l2_device: Request buffers from the device")
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  src/libcamera/v4l2_device.cpp | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 0cd9f4b8e178..a88a5f5ff036 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -676,6 +676,7 @@ int V4L2Device::createPlane(Buffer *buffer, unsigned int planeIndex,
>  	buffer->planes().emplace_back();
>  	Plane &plane = buffer->planes().back();
>  	plane.setDmabuf(expbuf.fd, length);
> +	::close(expbuf.fd);
>  
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart