Re: [patch 03/29] knfsd: add userspace controls for stats tables

[Greg Banks <gnb-xTcybq6BZ68@public.gmane.org>]

G'day,

Whoops, this bounced from the list because I accidentally had HTML
formatting on.  Resending.

On Tue, Apr 28, 2009 at 11:27 AM, Greg Banks <gnb-xTcybq6BZ68@public.gmane.org> wrote:
>
>
> On Tue, Apr 28, 2009 at 2:06 AM, Chuck Lever <chuck.lever@oracle.com> wrote:
>>
>> On Apr 25, 2009, at 6:03 PM, J. Bruce Fields wrote:
>>>
>>> Pfft, did it again.
>>>
>>> --b.
>>>
>>> On Sat, Apr 25, 2009 at 05:57:45PM -0400, bfields wrote:
>>>>
>>>> On Wed, Apr 01, 2009 at 07:28:03AM +1100, Greg Banks wrote:
>>>>>
>>>>> +
>>>>> +/**
>>>>> + * write_stats_prune_period - Set or report the period for pruning
>>>>> + *                           old per-client/per-export stats entries,
>>>>> + *                           in seconds.
>>>>> + *
>>>>> + * Input:
>>>>> + *                     buf:            ignored
>>>>> + *                     size:           zero
>>>>> + *
>>>>> + * OR
>>>>> + *
>>>>> + * Input:
>>>>> + *                     buf:            C string containing an unsigned
>>>>> + *                                     integer value representing the
>>>>> new value
>>>>> + *                     size:           non-zero length of C string in
>>>>> @buf
>>>>> + * Output:
>>>>> + *     On success:     passed-in buffer filled with '\n'-terminated C
>>>>> string
>>>>> + *                     containing numeric value of the current setting
>>>>> + *                     return code is the size in bytes of the string
>>>>> + *     On error:       return code is zero or a negative errno value
>>>>> + */
>>>>
>>>> Just an idle remark, don't worry about this for now, but: we might want
>>>> to rein in this write_*() comment format a little some day.  A lot of
>>>> the content seems duplicated.
>>
>> I disagree.
>>
>> The present nfsctl user space API is entirely ad hoc.
>
> Agreed.
>
>>
>>
>> Although sometimes the behavior is the same, each function/file can behave
>> slightly differently than the others.  We have to be very specific about
>> this here because the comments serve as both "user" documentation and as
>> code/API specification.
>
> Sure, but the comments aren't organised in such a way that the subtle
> differences between each pseudofile are obvious.  Instead there are large
> swathes of text above each write_*() function which describe behaviours
> common to all the write_*() functions, and you have to hunt carefully for
> the differences.
>
> For example, we don't need 10 copies of this text:
>
>  *     On success:     passed-in buffer filled with '\n'-terminated C string
>  *                     containing numeric value of the current setting
>  *                     return code is the size in bytes of the string
>
> Bruce rightly points out that my patch was continuing this poor trend.
>
>>
>> Because this code wasn't adequately documented, features have been added
>> over time without a close examination of the operation of other parts of
>> this code.
>>
>> If you really want to simplify the comments, we should consider
>> simplifying the API first, imo.
>
> Meh.  Most of the stuff in the nfsd filesystem could be *simplified* by
> using the actual /proc filesystem mechanisms instead.
>
>
>
> --
> Greg.
>



-- 
Greg.