Re: [PATCHv2 1/2] v4l2-ioctl: simplify code

[Hans Verkuil <hverkuil@xs4all.nl>]

On 04/13/2016 09:57 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 21 Mar 2016 09:47:59 +0100
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Instead of a big if at the beginning, just check if g_selection == NULL
>> and call the cropcap op immediately and return the result.
>>
>> No functional changes in this patch.
> 
> Hmm... not true. See below.
> 
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> 
> You forgot to add Miklas review here.
> 
>> ---
>>  drivers/media/v4l2-core/v4l2-ioctl.c | 51 ++++++++++++++++++++----------------
>>  1 file changed, 29 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>> index 6bf5a3e..3cf8d3a 100644
>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>> @@ -2160,33 +2160,40 @@ static int v4l_cropcap(const struct v4l2_ioctl_ops *ops,
>>  				struct file *file, void *fh, void *arg)
>>  {
>>  	struct v4l2_cropcap *p = arg;
>> +	struct v4l2_selection s = { .type = p->type };
>> +	int ret;
>>  
>> -	if (ops->vidioc_g_selection) {
>> -		struct v4l2_selection s = { .type = p->type };
>> -		int ret;
>> +	if (ops->vidioc_g_selection == NULL) {
>> +		/*
>> +		 * The determine_valid_ioctls() call already should ensure
>> +		 * that ops->vidioc_cropcap != NULL, but just in case...
>> +		 */
>> +		if (ops->vidioc_cropcap)
>> +			return ops->vidioc_cropcap(file, fh, p);
>> +		return -ENOTTY;
> 
> Actually, before this patch, the logic would be doing, instead:
> 
>         /* setting trivial pixelaspect */
>         p->pixelaspect.numerator = 1;
>         p->pixelaspect.denominator = 1;
>  
>         if (ops->vidioc_cropcap)
>                 return ops->vidioc_cropcap(file, fh, p);
>  
>         return 0;
> 
> With is not the same as what's there. So, there are actually two
> differences:
> 
> 1) it will now return -ENOTTY if !ops->vidioc_cropcap instead of 0.
> 
> (returning -ENOTTY is probably the best, but this is a fixup change,
> and a functional change, so, should be in a separate patch, if needed)
> 
> 2) pixel numerator/denominator is not initialized in the new code.
> 
> Ok, maybe all drivers would be setting numerator and denominator to 1
> as default, so the code won't need to do it, but:
> 
> a) This is not a trivial assumption by looking only on this patch;
> 
> b) It sounds risky to not setup denominator, as, if a driver is not
> initializing it, it could cause a division by zero at userspace.
> 
> So, I would prefer to keep initializing numerator/denominator here.
> 
> As this is just a cleanup patch, I'm skipping it for now.

I'm confused, you say you're skipping it, yet it is merged. But patch 2/2
(v4l2-ioctl: improve cropcap handling) isn't merged.

I refrain from commenting on the points you raised until I know what's going
on here.

Regards,

	Hans

> 
> Btw, IMHO, the best here would be to split the logic that it is called
> if ops->vidioc_g_selection is not NULL on a separate function. That
> would make the logic even clearer, and would help to show that it will
> be handling the logic below "setting trivial pixelaspect" the same way.
> 
> Regards,
> Mauro
> 
>> +	}
>>  
>> -		/* obtaining bounds */
>> -		if (V4L2_TYPE_IS_OUTPUT(p->type))
>> -			s.target = V4L2_SEL_TGT_COMPOSE_BOUNDS;
>> -		else
>> -			s.target = V4L2_SEL_TGT_CROP_BOUNDS;
>> +	/* obtaining bounds */
>> +	if (V4L2_TYPE_IS_OUTPUT(p->type))
>> +		s.target = V4L2_SEL_TGT_COMPOSE_BOUNDS;
>> +	else
>> +		s.target = V4L2_SEL_TGT_CROP_BOUNDS;
>>  
>> -		ret = ops->vidioc_g_selection(file, fh, &s);
>> -		if (ret)
>> -			return ret;
>> -		p->bounds = s.r;
>> +	ret = ops->vidioc_g_selection(file, fh, &s);
>> +	if (ret)
>> +		return ret;
>> +	p->bounds = s.r;
>>  
>> -		/* obtaining defrect */
>> -		if (V4L2_TYPE_IS_OUTPUT(p->type))
>> -			s.target = V4L2_SEL_TGT_COMPOSE_DEFAULT;
>> -		else
>> -			s.target = V4L2_SEL_TGT_CROP_DEFAULT;
>> +	/* obtaining defrect */
>> +	if (V4L2_TYPE_IS_OUTPUT(p->type))
>> +		s.target = V4L2_SEL_TGT_COMPOSE_DEFAULT;
>> +	else
>> +		s.target = V4L2_SEL_TGT_CROP_DEFAULT;
>>  
>> -		ret = ops->vidioc_g_selection(file, fh, &s);
>> -		if (ret)
>> -			return ret;
>> -		p->defrect = s.r;
>> -	}
>> +	ret = ops->vidioc_g_selection(file, fh, &s);
>> +	if (ret)
>> +		return ret;
>> +	p->defrect = s.r;
>>  
>>  	/* setting trivial pixelaspect */
>>  	p->pixelaspect.numerator = 1;
> 
>