[libcamera-devel] [PATCH 03/14] libcamera: v4l2_videodevice: Move FD duplication in V4L2Device::Open

[Laurent Pinchart]

On Wed, Sep 14, 2022 at 11:19:01AM +0200, Jacopo Mondi via libcamera-devel wrote:
> Hello Xavier,
> 
> On Thu, Sep 08, 2022 at 08:48:39PM +0200, Xavier Roumegue via libcamera-devel wrote:
> > This ensures that the V4L2Device controls attributes have been enumerated
> > in its open() method in case of file descriptor duplication.
> 
> Do I get this right or the actual issue is the controls are not
> enumerated for M2M devices because V4L2Device::open() (and consequentially
> V4L2Device::listControls()) is never called for them ?
> 
> If that's the case, I would squash 2 and 3 and clarify this in the
> commit message.

I agree. And as setFd() is now used internally only, I would turn it
from a protected to a private function.

I think I would prefer keeping the file handle duplication in
V4L2VideoDevice::open() though. I'll propose a patch.

> > Signed-off-by: Xavier Roumegue <xavier.roumegue at oss.nxp.com>
> > ---
> >  src/libcamera/v4l2_videodevice.cpp | 12 ++----------
> >  1 file changed, 2 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index 955e1508..d0427fef 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -661,17 +661,9 @@ int V4L2VideoDevice::open(SharedFD handle, enum v4l2_buf_type type)
> >  {
> >  	int ret;
> >
> > -	UniqueFD newFd = handle.dup();
> > -	if (!newFd.isValid()) {
> > -		ret = -errno;
> > -		LOG(V4L2, Error) << "Failed to duplicate file handle: "
> > -				 << strerror(-ret);
> > -		return ret;
> > -	}
> > -
> > -	ret = V4L2Device::setFd(std::move(newFd));
> > +	ret = V4L2Device::open(handle);
> >  	if (ret < 0) {
> > -		LOG(V4L2, Error) << "Failed to set file handle: "
> > +		LOG(V4L2, Error) << "Failed to open file handle: "
> >  				 << strerror(-ret);
> 
> V4L2Device::open() already complains out loud in case of failure. I
> would drop the error message here.
> 
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> >  		return ret;
> >  	}

-- 
Regards,

Laurent Pinchart