Re: [RFC c/r 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v7

[ebiederm@xmission.com (Eric W. Biederman)]

KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> writes:

> On 1/27/2012 12:53 PM, Cyrill Gorcunov wrote:
>> +	switch (type) {
>> +	case KCMP_FILE: {
>> +		struct file *filp1, *filp2;
>> +
>> +		filp1 = get_file_raw_ptr(task1, idx1);
>> +		filp2 = get_file_raw_ptr(task2, idx2);
>> +
>> +		if (filp1 && filp2)
>> +			ret = kcmp_ptr((long)filp1, (long)filp2, KCMP_FILE);
>> +		else
>> +			ret = -ENOENT;
>
> If my remember is correct, Andrew pointed out EINVAL is better than ENOENT.

Ah yes.  And really what it should be is
		if (!filp1 || !filp2)
 			return -EBADF;

At least EBADF is what you return if it is your process that doesn't
have the filedescriptor.

>> +		break;
>> +	case KCMP_SYSVSEM:
>> +#ifdef CONFIG_SYSVIPC
>> +		ret = kcmp_ptr((long)task1->sysvsem.undo_list,
>> +			       (long)task2->sysvsem.undo_list,
>> +			       KCMP_SYSVSEM);
>> +#else
>> +		ret = -EINVAL;
>
> ENOTSUP is better, I think. because of, EINVAL implicitly mean _caller_ is wrong.
> but in this case, it is not bad. only the kernel doesn't have enough
> feature.

Careful a type compiled out should in principle match a type whose
support has not been implemented. That is the default case should match
what happens when you don't compile in sysvipc support.

>
>> +		goto err;
>
> you don't need err label at all.
>
>
>> +#endif
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
>> +		goto err;
>> +	}
>> +
>> +err:
>> +	put_task_struct(task1);
>> +	put_task_struct(task2);
>> +
>> +	return ret;
>> +}

Eric