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

[Laurent Pinchart]

Hi Kieran,

On Wed, Jan 16, 2019 at 12:07:23PM +0000, Kieran Bingham wrote:
> On 16/01/2019 11:11, Jacopo Mondi wrote:

[snip]

> >>>> +/**
> >>>> + * \brief Open a V4L2 device and query properties from the device.
> >>>> + * \return 0 on success, or a negative error code otherwise
> >>>> + */
> >>>> +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;
> 
> This assignment feels a little redundant.
> 
> I agree on only setting the fd_ = ret if success is good - but I'm
> feeling like I can save a line (think of the bits) and just return -errno?

You can't because LOG(Error) << ... may change errno. That's also why
you need to pass -ret to strerror and not errno, as it may change by the
time strerror is called.

> >>>> +		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
> >>
> >> Ahh oops - and good spot.
> >>
> >> But yes - I'd much rather reference the errno directly for strerror().
> >>
> >>>> +		return ret;
> >>>> +	}

[snip]

-- 
Regards,

Laurent Pinchart