cvs commit: src/sys/dev/em if_em.c if_em.h

[Gleb Smirnoff]

  Jack,

On Fri, Nov 10, 2006 at 09:30:27AM +0000, Jack F Vogel wrote:
J> jfv         2006-11-10 09:30:27 UTC
J> 
J>   FreeBSD src repository
J> 
J>   Modified files:        (Branch: RELENG_6)
J>     sys/dev/em           if_em.c if_em.h 
J>   Log:
J>   This patch redesigns the watchdog timer. The old version had SMP
J>   vulnerabilities. It also has the FAST_INTR code as a compile option.
J>   
J>   Submitted by: jfv
J>   Reviewed by: scottl
J>   Approved by: re, scottl
J>   
J>   Revision   Changes    Path
J>   1.65.2.21  +297 -87   src/sys/dev/em/if_em.c
J>   1.32.2.6   +11 -3     src/sys/dev/em/if_em.h

This patch also does a number of other changes, that aren't listed
in the commit message:

1) Merges revision 1.159 by jhb, that locks callouts properly.
2) Merges revisions 1.160, 1.161 by jhb, that move the
   allocation/destruction of receive/transmit structures to the
   device attach and detach methods.
3) Backouts revision 1.80 by glebius.
4) Fixes mbuf handling error in em_encap() that would let to make
   manipulations with freed mbufs.
5) Removes bogus declaration from if_em.c.

In the FreeBSD CVS there is a habit to list all the changes made,
don't hide anything. A detailed CVS log, that includes all the
changes made, including references to original revisions in HEAD,
makes easier the work of other people in the project and outside
of it. It is easier to review and understand such changes, easier 
to apply it to outside source trees, easier to backout to check 
for regressions.

Also, it is preferrable not to put a lot of different changes
into one commit, just dropping your own source tree above the
CVS checkout and typing "cvs ci". It is better to put effort
into separating each logical change into one separate patch and
commit it. Again, this will make easier to understand and to
make partial backouts in case of regressions.

Also, AFAIR, this patch was expected to fix kern/104978 problem
report. In perfect case, you should had assigned the PR to
yourself as soon as it arrived, because it is a regression
that was made by your commit, so you take the responsibility
with this PR. Then, the patch should had been sent to the PR
originator for testing. And finally, the commit message should
have referenced the PR.

Also, in FreeBSD CVS there is a habit to give credit to all
parties involved in the work, including people who reported   
the problem, who tested the patch, who reviewed it, commented
or submitted parts of the patch.

In the discussed commit you should have added me as reviewer,   
or even as submitter, because I have suggested to piggyback on
local timer and I've submitted original patch, that does this.
Also, I have pointed out the mbuf handling error in em_encap(),
even have written two emails to explain that to you.
The forward declaration fix was submitted by Ed Maste.

And concerning backing out revision 1.80. I am too tired to
speak in support of "Glebs beloved infinite loop". This question
was raised so many times during last year. I will repeat my words
for the last time, as this mail goes to public list and I will be
able to just point people at it. A year ago, I have spent several
weeks fighting with wedging interfaces on my routers and finally
came to this loop. This loop fixes a PRACTICALLY encountered
problem, and now you have removed it, because it THEORETICALLY can
cause problems. Ok... You are maintainer of the driver, you decide.
I will not insist on this loop anymore, I will just have a patched
em(4) driver for my job, as many companies already have.

Finally, I will just notice that two more FreeBSD policies were
violated by your last commits: committing directly to STABLE
branch, omitting mentor's approval.

-- 
Totus tuus, Glebius.
GLEBIUS-RIPN GLEB-RIPE