From owner-cvs-all@FreeBSD.ORG Fri Nov 10 17:28:06 2006 Return-Path: X-Original-To: cvs-all@freebsd.org Delivered-To: cvs-all@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id DECF516A40F for ; Fri, 10 Nov 2006 17:28:05 +0000 (UTC) (envelope-from jfvogel@gmail.com) Received: from nz-out-0102.google.com (nz-out-0102.google.com [64.233.162.203]) by mx1.FreeBSD.org (Postfix) with ESMTP id CFED043D79 for ; Fri, 10 Nov 2006 17:28:00 +0000 (GMT) (envelope-from jfvogel@gmail.com) Received: by nz-out-0102.google.com with SMTP id i11so342340nzh for ; Fri, 10 Nov 2006 09:28:00 -0800 (PST) DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=beta; d=gmail.com; h=received:message-id:date:from:to:subject:cc:in-reply-to:mime-version:content-type:content-transfer-encoding:content-disposition:references; b=hYJPMXSnleVh1k/n/tf2LO1YiMQMf+GocMQqKNJ2oftuDwcC/xrFrrzbz98ppUcycvcgo3u4UK1pEtE6OEHmJ7ZnzxDLyT2kqYWGZIIJo4jiWXdt0quFVhDBFOFKTXe8nyWtrLm/PRPmxfBowp2g36muXH/rmV14WEASLg7xhRk= Received: by 10.35.60.16 with SMTP id n16mr3720078pyk.1163179679636; Fri, 10 Nov 2006 09:27:59 -0800 (PST) Received: by 10.35.118.6 with HTTP; Fri, 10 Nov 2006 09:27:59 -0800 (PST) Message-ID: <2a41acea0611100927x1aadeacdi8a27527895d188f3@mail.gmail.com> Date: Fri, 10 Nov 2006 09:27:59 -0800 From: "Jack Vogel" To: "Gleb Smirnoff" , "Scott Long" , RelEng , "Prafulla Deuskar" In-Reply-To: <20061110122336.GH32700@FreeBSD.org> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <200611100930.kAA9UR7S084195@repoman.freebsd.org> <20061110122336.GH32700@FreeBSD.org> Cc: Jack F Vogel , 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 X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 10 Nov 2006 17:28:06 -0000 On 11/10/06, Gleb Smirnoff 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