Date: Wed, 28 Nov 2012 09:01:53 +0100 From: Andre Oppermann <andre@freebsd.org> To: "Robert N. M. Watson" <rwatson@FreeBSD.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Peter Wemm <peter@wemm.org> Subject: Re: svn commit: r243627 - head/sys/kern Message-ID: <50B5C4F1.1020002@freebsd.org> In-Reply-To: <2A12C740-1D72-4D30-B663-47A37AAC2FF3@FreeBSD.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>
next in thread | previous in thread | raw e-mail | index | archive | help
On 28.11.2012 00:59, Robert N. M. Watson wrote: > > On 27 Nov 2012, at 23:29, Andre Oppermann wrote: > >>>>>>> Andre.. this breaks incoming connections. TCP is immediately reset and never even gets to the >>>>>>> listener process. You need to back out of fix this urgently please. >>>>>> >>>>>> I just found out and fixed it. Sorry for the breakage. >>>>> >>>>> 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 over 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 people with expertise in it. >>>> >>>> Good to see you becoming more active again. :-) And yes, >>>> you have a point there. >>> >>> 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 obligations :-). >> >> Just saw that I did indeed send you a review request three weeks ago. ;-) >> At the end of a rather long email though. > > Yes, indeed -- no patch was attached, and it followed quite a long e-mail on your plans to rewrite the TCP stack. I'm afraid that went onto the "read 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 the patch rather than trying a bit harder to find a reviewer isn't the right answer either. To maximise the likelihood of a review, construct an e-mail with a subject line like "Review request: (patch description)", attach the patch, and include a proposed commit message. 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. -- Andre
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?50B5C4F1.1020002>