Date: Fri, 10 Nov 2006 09:27:59 -0800 From: "Jack Vogel" <jfvogel@gmail.com> To: "Gleb Smirnoff" <glebius@freebsd.org>, "Scott Long" <scottl@samsco.org>, RelEng <re@freebsd.org>, "Prafulla Deuskar" <pdeuskar@freebsd.org> Cc: Jack F Vogel <jfv@freebsd.org>, 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: <2a41acea0611100927x1aadeacdi8a27527895d188f3@mail.gmail.com> In-Reply-To: <20061110122336.GH32700@FreeBSD.org> References: <200611100930.kAA9UR7S084195@repoman.freebsd.org> <20061110122336.GH32700@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 11/10/06, Gleb Smirnoff <glebius@freebsd.org> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?2a41acea0611100927x1aadeacdi8a27527895d188f3>