[PATCH] staging:iio: Add wrapper functions around buffer access ops

[Lars-Peter Clausen]

On 12/14/2011 07:23 PM, Greg KH wrote:
> On Wed, Dec 14, 2011 at 06:35:25PM +0100, Lars-Peter Clausen wrote:
>> On 12/14/2011 04:49 PM, Greg KH wrote:
>>> On Wed, Dec 14, 2011 at 11:15:49AM +0100, Lars-Peter Clausen wrote:
>>>> On 12/14/2011 12:59 AM, Greg KH wrote:
>>>>>
>>>>>>>> +static inline int buffer_get_length(struct iio_buffer *buffer)
>>>>>>>> +{
>>>>>>>> +	if (buffer->access->get_length)
>>>>>>>> +		return buffer->access->get_length(buffer);
>>>>>>>> +
>>>>>>>> +	return -ENOSYS;
>>>>>>>
>>>>>>> Here you return an error, but why ENOSYS?
>>>>>>>
>>>>>>> Consistancy is key, and you don't have it here at all.  Or if you do, I
>>>>>>> sure don't understand it...
>>>>>>
>>>>>> Well, different types of functions require different semantics. While the
>>>>>> previous ones did either return 0 in case of success or a error value in case
>>>>>> of an error, buffer_get_length returns an integer value where 0 is a valid
>>>>>> value. Since we can't make any meaningful assumptions about the buffer size if
>>>>>> the callback is not implemented we return an error value. Why ENOSYS? Because
>>>>>> it is the code for 'function not implemented' and is used throughout the kernel
>>>>>> in similar situations.
>>>>>
>>>>> Is the caller always supposed to check this?  If so, please mark the
>>>>> function as such so the compiler will complain if it isn't.
>>>>
>>>> Marking the function as __must_check doesn't make much sense here. Since it
>>>> will either return an error or the buffer length. So you'll always use the
>>>> returned result one way or the other.
>>>
>>> That's exactly the point, you must use it, so mark it as such.
>>>
>> So by that logic all functions without side effects should be marked as
>> __must_check?
> 
> "Without side effects"?  What do you mean by this?

A function which doesn't not modify any state outside it's scope (See also
[1]). I.e. it just returns an value, based on it's input parameters. E.g.
strlen(), memcmp(), bitmap_weight(), atomic_read() etc...

> 
> Any function, whose return code MUST be checked, should be marked with
> __must_check, it's quite simple.

I think we can agree on this one. But I would say that for buffer_get_length()
the semantics are that its return code SHOULD be checked.

- Lars

[1] http://en.wikipedia.org/wiki/Side_effect_%28computer_science%29