[libcamera-devel] [PATCH v3 2/3] libcamera: Add V4L2 Device object

[Laurent Pinchart]

Hi Kieran,

On Wed, Jan 16, 2019 at 03:42:50PM +0000, Kieran Bingham wrote:
> On 16/01/2019 15:00, Jacopo Mondi wrote:
> > On Wed, Jan 16, 2019 at 03:54:23PM +0100, Niklas Söderlund wrote:
> >> On 2019-01-16 09:44:55 +0100, Jacopo Mondi wrote:
> >>
> >> [snip]
> >>
> >>>> +int V4L2Device::open()
> >>>> +{
> >>>> +	int ret;
> >>>> +
> >>>> +	if (isOpen()) {
> >>>> +		LOG(Error) << "Device already open";
> >>>> +		return -EBUSY;
> >>>> +	}
> >>>> +
> >>>> +	ret = ::open(devnode_.c_str(), O_RDWR);
> >>>> +	if (ret < 0) {
> >>>> +		ret = -errno;
> >>>> +		LOG(Error) << "Failed to open V4L2 device " << devnode_
> >>>> +			   << " : " << strerror(ret);
> >>>
> >>> strerror(-ret)
> >>>
> >>> Even if in this case you could have used errno directly. Sorry, my
> >>> comment might has mis-lead you
> >>
> >> No you can't use errno in the call to strerror() as it might already
> >> have been been changed by the previous strings passed to LOG(Error),
> >> unfortunately it needs to be cached.
> >>
> > 
> > Right, that's why we standardized on that patter... sorry, I've been
> > sloppy commenting this...
> > 
> > Kieran, pay attention to this in the IOCTL error handling you have a
> > few lines below these ones...
> > 
> 
> Ok - updated. Can I change the location of the negative inversions?
> 
> Setting ret = -errno, to then re-negate it in strerror() feels horrible
> to me.
> 
> Negating it for the return code seems more acceptable as we know that
> return codes are checked for if(ret < 0)...
> 
> {
> 	...
> 
> 	ret = ::open(devnode_.c_str(), O_RDWR);
> 	if (ret < 0) {
> 		ret = errno;
> 		LOG(Error) << "Failed to open V4L2 device " << devnode_
> 			   << " : " << strerror(ret);
> 		return -ret;
> 	}
> 	fd_ = ret;
> 
> 	ret = ioctl(fd_, VIDIOC_QUERYCAP, caps_.caps());
> 	if (ret < 0) {
> 		ret = errno;
> 		LOG(Error) << "Failed to query device capabilities: " << strerror(ret);
> 		return -ret;
> 	}
> }

I think both options have their pros and cons. I'm fine with this
proposal, but in that case, please change it library-wide.

-- 
Regards,

Laurent Pinchart