From owner-svn-src-all@FreeBSD.ORG Thu Nov 29 03:49:50 2012 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 4EF89B1C; Thu, 29 Nov 2012 03:49:50 +0000 (UTC) (envelope-from yanegomi@gmail.com) Received: from mail-pb0-f54.google.com (mail-pb0-f54.google.com [209.85.160.54]) by mx1.freebsd.org (Postfix) with ESMTP id EC6568FC0C; Thu, 29 Nov 2012 03:49:49 +0000 (UTC) Received: by mail-pb0-f54.google.com with SMTP id wz12so10496069pbc.13 for ; Wed, 28 Nov 2012 19:49:48 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=references:mime-version:in-reply-to:content-type :content-transfer-encoding:message-id:cc:x-mailer:from:subject:date :to; bh=qiLaJtSoc//JPSWh0uDp9bg5LtBA/TXf8isr1HBdeWo=; b=fRXjbca57Os6t29uxrH0bDtpsOfzcKct5VXPyn+Dq2KIE2wa+GZARm26uhfoMHtkCj /QtrHnV5tUm2OVokxgEbJQvydhgalSV5pIimLkOiYRwORWC5kuh2sOulLLpjropSsymQ Bw4zGHPC4jEgji4o4iM6tI+radbfqNUbju9ZLZDrmvpBH2f/ZK4mQHJQivh3K3CwWEiA 93WnW8iw2kcq7OmT3qxNnolHvVcSTjlxVKK5OVQ4bMjWqX4gstE1PRSP873Ed19yDHDL iCEAT5AS7nVmMbDGQqK2nhfOJgjDNvPQAFU91yXoCMwZwa0trBEnPgvpCH7wCKGIBK4M Cm0A== Received: by 10.68.247.39 with SMTP id yb7mr65431088pbc.15.1354160988265; Wed, 28 Nov 2012 19:49:48 -0800 (PST) Received: from [10.11.216.81] (mobile-166-147-095-130.mycingular.net. [166.147.95.130]) by mx.google.com with ESMTPS id o7sm296629pax.31.2012.11.28.19.49.45 (version=SSLv3 cipher=OTHER); Wed, 28 Nov 2012 19:49:47 -0800 (PST) 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> Mime-Version: 1.0 (1.0) In-Reply-To: <50B64C43.50001@mu.org> Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: quoted-printable Message-Id: X-Mailer: iPhone Mail (10A523) From: Garrett Cooper Subject: Reviewing before commit and stability minibikeshed (Re: svn commit: r243627 - head/sys/kern) Date: Wed, 28 Nov 2012 19:49:40 -0800 To: Alfred Perlstein Cc: "src-committers@freebsd.org" , Andre Oppermann , Peter Wemm , "svn-src-all@freebsd.org" , "Robert N. M. Watson" , "svn-src-head@freebsd.org" X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 29 Nov 2012 03:49:50 -0000 On Nov 28, 2012, at 9:39 AM, Alfred Perlstein wrote: > On 11/28/12 12:01 AM, Andre Oppermann wrote: >> On 28.11.2012 00:59, Robert N. M. Watson wrote: >>>=20 >>> On 27 Nov 2012, at 23:29, Andre Oppermann wrote: >>>=20 >>>>>>>>> Andre.. this breaks incoming connections. TCP is immediately rese= t and never even gets to the >>>>>>>>> listener process. You need to back out of fix this urgently pleas= e. >>>>>>>>=20 >>>>>>>> I just found out and fixed it. Sorry for the breakage. >>>>>>>=20 >>>>>>> I'd like to see a much more thorough use of "Reviewed by:" in socket= and TCP-related commits -- this >>>>>>> is very sensitive code, and a second pair of eyes is always valuable= . Post-commit review is not a >>>>>>> substitute. Looking back over similar changes in the socket code ov= er the last two years, I see >>>>>>> that almost all have reviewers, so I think it would be reasonable to= consider it mandatory for these >>>>>>> subsystems at this point. The good news is that we have lots of peo= ple with expertise in it. >>>>>>=20 >>>>>> Good to see you becoming more active again. :-) And yes, >>>>>> you have a point there. >>>>>=20 >>>>> Yes -- this is only about three weeks old, however; for the prior six-= twelve months, I've been fairly non-existent in FreeBSD-land due to outside o= bligations :-). >>>>=20 >>>> Just saw that I did indeed send you a review request three weeks ago. ;= -) >>>> At the end of a rather long email though. >>>=20 >>> Yes, indeed -- no patch was attached, and it followed quite a long e-mai= l on your plans to rewrite the TCP stack. I'm afraid that went onto the "rea= d this later as time permits" pile as I was at a conference, rather than the= fast-path "oh, quickly review this patch" pile. However, simply committing t= he patch rather than trying a bit harder to find a reviewer isn't the right a= nswer either. To maximise the likelihood of a review, construct an e-mail wi= th a subject line like "Review request: (patch description)", attach the pat= ch, and include a proposed commit message. >>=20 >> Yes, and I didn't really expect you to answer (at least quickly) during >> your FreeBSD hiatus. So it was seeking review by chance. >>=20 >> 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. >>=20 >> 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 fee= t when developing FreeBSD. >=20 > Mistakes will happen, they will happen in head. Slowing down the process t= o eliminate mistakes only works to slow down change and give a false sense o= f "fixing stability" when in fact the only thing "stable" is the slowness of= submitting code. Organizing cabals of people to review code is the best way to go. It's an ex= tension of the maintainers concept applied in a better fashion because it en= courages several developers to provide input on a series of commits instead o= f one. I know it's sort of done in some groups [based on commit messages], but it w= ould be nice to have it better formalized and socialized as well. Things lik= e this are helpful for other potential freebsd contributors/developers (like= some folks I work with for instance!). An extension of this code review idea would probably be reviewboard. Email b= ased review is doable and a lot of OSS groups use it, but there are some nic= e points to using a more advanced tool, in particular: 1. Colorized diffs. 2. Various diff niceties (hide white space changes, etc). 3. It does a reasonable job at establishing code context (code block A has m= oved to B). There are 2 FreeBSD corporate consumers that use it, with at least one other= group considering it, and there are probably more groups that using it as w= ell. Plus it integrates well with p4, and does pretty well with git and svn.= Thanks! -Garrett=