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

701 diff mbox series

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.

Fixes: 771befc6dc0e ("libcamera: v4l2_device: Request buffers from the device")
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/libcamera/v4l2_device.cpp | 1 +
 1 file changed, 1 insertion(+)

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@ideasonboard.com>

> Fixes: 771befc6dc0e ("libcamera: v4l2_device: Request buffers from the device")
> Signed-off-by: Kieran Bingham <kieran.bingham@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;
>  }

Hi Laurent,

On 05/03/2019 12:47, Laurent Pinchart wrote:
> 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.

Well, it didn't exactly go unnoticed - It just hadn't been dug out.

This was part of the reason for the call to
V4L2Device::requestBuffers(0) failing.


> 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


I did think about that too, and considered removing the dup. Especially
as it feels a bit weird to take the expbuf.fd and close it (almost)
immediately.

But, I think the duplication does keep the 'layering' and ownership in
better form. And in this instance it's just a matter of making sure the
internal ownership is tracked.

When we start using externally provided buffers - I believe the
separation of the internal FD will be beneficial.



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

I think that might just add more complication without any real benefit
right now. If it becomes an issue we can reconsider this when we add
support for externally provided buffers later.

> 
> In any case,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks, I'll merge this to master now as it fixes a bug.

--
Kieran


> 
>> Fixes: 771befc6dc0e ("libcamera: v4l2_device: Request buffers from the device")
>> Signed-off-by: Kieran Bingham <kieran.bingham@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;
>>  }
>