Re: [PATCH 4/5] [media] v4l: fix copying ioctl results on failure

[Tomasz Stanislawski <t.stanislaws@samsung.com>]

On 08/26/2011 05:09 PM, Laurent Pinchart wrote:
Hi Laurent,
> Hi Tomasz,
>
> On Friday 26 August 2011 15:06:06 Tomasz Stanislawski wrote:
>> This patch fix the handling of data passed to V4L2 ioctls.  The content of
>> the structures is not copied if the ioctl fails.  It blocks ability to
>> obtain any information about occurred error other then errno code. This
>> patch fix this issue.
> Does the V4L2 spec say anything on this topic ? We might have applications
> that rely on the ioctl argument structure not being touched when a failure
> occurs.
Ups.. I missed something. It looks that modifying ioctl content is 
illegal if ioctl fails. The spec says:
"When an ioctl that takes an output or read/write parameter fails, the 
parameter remains unmodified." (v4l2 ioctl section)
However, there is probably a bug already present in V4L2 framework.
There are some ioctls that takes a pointer to an array as a field in the 
argument struct.
The examples are all VIDIOC_*_EXT_CTRLS and VIDIOC_{QUERY/DQ/Q}_BUF family.
The content of such an auxiliary arays is copied even if ioctl fails. 
Please take a look to video_usercopy function in v4l2-ioctl.c. Therefore 
I think that the spec is already violated. What is your opinion about 
this problem?

Now back to selection case.
This patch was added as proposition of fix to VIDIOC_S_SELECTION, to 
return the best-hit rectangle if constraints could not be satisfied. The 
ioctl return -ERANGE in this case. Using those return values the 
application gets some feedback
on loosing constraints.

I could remove rectangle returning from the spec and s5p-tv code for now.

Regards,
Tomasz Stanislawski

>> Signed-off-by: Tomasz Stanislawski<t.stanislaws@samsung.com>
>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>> ---
>>   drivers/media/video/v4l2-ioctl.c |    2 --
>>   1 files changed, 0 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/video/v4l2-ioctl.c
>> b/drivers/media/video/v4l2-ioctl.c index 543405b..9f54114 100644
>> --- a/drivers/media/video/v4l2-ioctl.c
>> +++ b/drivers/media/video/v4l2-ioctl.c
>> @@ -2490,8 +2490,6 @@ video_usercopy(struct file *file, unsigned int cmd,
>> unsigned long arg, err = -EFAULT;
>>   		goto out_array_args;
>>   	}
>> -	if (err<  0)
>> -		goto out;
>>
>>   out_array_args:
>>   	/*  Copy results into user buffer  */