One of my pet hates is the lack of documentation with much of the code,
ie. comments.  As we move forward, more and more code gets written and
it doesn't get any easier to understand how it works.  At present we
are without much in the way of SMP support but once that is added, the
code will become even more complex, over time.

What I'd like to suggest is that in going forward, we adopt some changes
to the KNF that require people to at least put more text in a comment
block prior to the function.

Some things which I think would be or are going to be useful:

- statement about which locks are expected to be held and how.
  this should include extra comments if a lock is acquired or released
  prior to returning.

- statement about which interrupts are expected to be masked or not.
  this should include extra comments if an interupt is masked or unmasked
  prior to returning.

- statement about what the function does, including what is considered
  to be a successful call or not.  This may or may not include talking
  about what gets returned.

To go through and do this retrospectively is a huge task so I am not
suggesting that, rather as people touch various routines where they
may or may not alter behaviour, more comments get added.  I think that
updating the KNF is the correct way to encourage more people to do
this.  A suggestion for pre-function comments might look like is this:

/*
 * Function: foo
 * Locks:    big_lock(M)
 */

Where "M" is a mutex.  Some set of mneumonics needs to be standardised
upon here to make the lock list both meaningful and concise.

/*
 * Function:   foo_rx
 * Interrupts: imp
 */

"imp" from "splimp".

So where does the inspiration for this come from ?  Working on commercial
projects and liking the result and seeing benefits in change to IPFilter.
I'm not saying NetBSD should use the same or even anything like what I've
been doing, but it can't hurt to make pre-function comments more expansive
than they currently are.

An example from IPFilter:
/* ------------------------------------------------------------------------ */
/* Function:    nat_new                                                     */
/* Returns:     nat_t* - NULL == failure to create new NAT structure,       */
/*                       else pointer to new NAT structure                  */
/* Parameters:  fin(I)       - pointer to packet information                */
/*              np(I)        - pointer to NAT rule                          */
/*              natsave(I)   - pointer to where to store NAT struct pointer */
/*              flags(I)     - flags describing the current packet          */
/*              direction(I) - direction of packet (in/out)                 */
/* Write Lock:  ipf_nat                                                     */
/*                                                                          */
/* Attempts to create a new NAT entry.  Does not actually change the packet */
/* in any way.                                                              */
/*                                                                          */
/* This fucntion is in three main parts: (1) deal with creating a new NAT   */
/* structure for a "MAP" rule (outgoing NAT translation); (2) deal with     */
/* creating a new NAT structure for a "RDR" rule (incoming NAT translation) */
/* and (3) building that structure and putting it into the NAT table(s).    */
/* ------------------------------------------------------------------------ */

Extremely verbose, and perhaps over verbose on the "Parameters" side.
I'm not saying NetBSD should use that but perhaps think into the future,
now, about what would be good to have and set a direction for others
here to follow when hacking on NetBSD code.

Darren