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

[Kieran Bingham]

On 16/01/2019 15:00, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Wed, Jan 16, 2019 at 03:54:23PM +0100, Niklas Söderlund wrote:
>> Hi Kieran, Jacopo
>>
>>
>>
>> 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;
	}
}



> Thanks
>   j
> 
>>>
>>>> +		return ret;
>>>> +	}
>>
>> --
>> Regards,
>> Niklas Söderlund

-- 
Regards
--
Kieran