Date: Fri, 10 Nov 2006 15:23:36 +0300 From: Gleb Smirnoff <glebius@FreeBSD.org> To: Jack F Vogel <jfv@FreeBSD.org> Cc: cvs-src@FreeBSD.org, src-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/dev/em if_em.c if_em.h Message-ID: <20061110122336.GH32700@FreeBSD.org> In-Reply-To: <200611100930.kAA9UR7S084195@repoman.freebsd.org> References: <200611100930.kAA9UR7S084195@repoman.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20061110122336.GH32700>