On 11/10/06, Gleb Smirnoff <[EMAIL PROTECTED]> wrote:
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.
Not trying to hide anything Gleb, what I checked in was the patch
that was posted to stable. OK, so it wasnt broken down into
each individual piece, or I didnt list them all, I will try to be more
dutiful on that kind of detail.
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.
Still not familiar with dealing with PRs.
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.
I did give all parties credit back when the patch was posted to
mailing list.
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.
I talked with Scott specifically about this, he supported this change.
You said before that FAST_INTR solves that problem for you, my
intention was that you use that solution.
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.
This was NO violation Gleb, I had email and approval from RE and
Scott to check this code in, thats why my checkin said it was
approved by RE.
I am sorry that this checkin seems to have aggrevated you so
much, I have just been trying to make this release a success.
Jack
_______________________________________________
cvs-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/cvs-all
To unsubscribe, send any mail to "[EMAIL PROTECTED]"
