From owner-svn-src-head@FreeBSD.ORG Wed Nov 28 18:13:43 2012 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 945B1F34; Wed, 28 Nov 2012 18:13:42 +0000 (UTC) (envelope-from rwatson@FreeBSD.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id 2E1DA8FC0C; Wed, 28 Nov 2012 18:13:42 +0000 (UTC) Received: from fledge.watson.org (fledge.watson.org [65.122.17.41]) by cyrus.watson.org (Postfix) with ESMTPS id D3F7E46B17; Wed, 28 Nov 2012 13:13:41 -0500 (EST) Date: Wed, 28 Nov 2012 18:13:41 +0000 (GMT) From: Robert Watson X-X-Sender: robert@fledge.watson.org To: Alfred Perlstein Subject: Re: svn commit: r243627 - head/sys/kern In-Reply-To: <50B64C43.50001@mu.org> Message-ID: References: <201211272004.qARK4qS8047209@svn.freebsd.org> <50B54180.5020608@freebsd.org> <50B54492.5040100@freebsd.org> <956CE44A-BA0F-4FE4-AA38-F4B90C85ECBA@FreeBSD.org> <50B54CE0.6080008@freebsd.org> <2A12C740-1D72-4D30-B663-47A37AAC2FF3@FreeBSD.org> <50B5C4F1.1020002@freebsd.org> <50B64C43.50001@mu.org> User-Agent: Alpine 2.00 (BSF 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Andre Oppermann , Peter Wemm X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 28 Nov 2012 18:13:43 -0000 On Wed, 28 Nov 2012, Alfred Perlstein wrote: >> Yes, and I didn't really expect you to answer (at least quickly) during >> your FreeBSD hiatus. So it was seeking review by chance. >> >> Alas I found and fixed the bug myself within 2.5hrs. While not optimal, a >> sign of poor prior testing and too much trust into the submitter of the >> patch it wasn't an earth shattering event. Doesn't distract from the fact >> that it was mea culpa in any case though. >> >> For prior review of kern_socket* and netinet/tcp_* related changes it has >> been on and off by various committers over the past year. If we do have a >> policy of prior review required then it should be made official and >> codified in MAINTAINERS and universally applied to all. >> > Personally I don't think we need any more anchors attached to people's feet > when developing FreeBSD. > > Mistakes will happen, they will happen in head. Slowing down the process to > eliminate mistakes only works to slow down change and give a false sense of > "fixing stability" when in fact the only thing "stable" is the slowness of > submitting code. Yes, the purpose of review in this case would not be to catch (too many) dumb mistakes -- that's what testing is for -- but rather, to introduce architectural review and catch quite subtle mistakes. It was interesting doing the development cycle for the pcbgroup work a year or so ago -- having Bjoern do line-by-line reviews of changes generated not only lots of bug fixes, but also lots of high-level feedback on the design that improved it markedly. "MAINTAINERS" is a concept I'd like to see die, but the idea of seeking pre-commit review for changes, especially in sensitive code, is something to encourage. One reason review is so critical in this area is that there are lots of subtle implications to do with timing, object life cycles, inter-layer locking, etc, in the socket and TCP code -- things that any one author may not see the big picture on. We've been working to improve comments, but the problem there, of course, is that you end up with a book of design comments, and code review actually brings many of those concerns to light in a more structured and practical way. Robert