Date: Wed, 28 Nov 2012 18:13:41 +0000 (GMT) From: Robert Watson <rwatson@FreeBSD.org> To: Alfred Perlstein <bright@mu.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Andre Oppermann <andre@freebsd.org>, Peter Wemm <peter@wemm.org> Subject: Re: svn commit: r243627 - head/sys/kern Message-ID: <alpine.BSF.2.00.1211281810190.91531@fledge.watson.org> In-Reply-To: <50B64C43.50001@mu.org> References: <201211272004.qARK4qS8047209@svn.freebsd.org> <CAGE5yCpxOdsjefe6quR_gjs82pk9a2e_H_WUNUWhUGA3WZPJaw@mail.gmail.com> <50B54180.5020608@freebsd.org> <alpine.BSF.2.00.1211272246560.37292@fledge.watson.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>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?alpine.BSF.2.00.1211281810190.91531>