From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:55879) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RjGcv-0001Uy-CW for qemu-devel@nongnu.org; Fri, 06 Jan 2012 15:43:02 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RjGct-0004fV-NT for qemu-devel@nongnu.org; Fri, 06 Jan 2012 15:43:01 -0500 Received: from e33.co.us.ibm.com ([32.97.110.151]:45447) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RjGct-0004f6-BY for qemu-devel@nongnu.org; Fri, 06 Jan 2012 15:42:59 -0500 Received: from /spool/local by e33.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 6 Jan 2012 13:42:54 -0700 Received: from d03av04.boulder.ibm.com (d03av04.boulder.ibm.com [9.17.195.170]) by d03relay03.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q06Kgp3H167394 for ; Fri, 6 Jan 2012 13:42:51 -0700 Received: from d03av04.boulder.ibm.com (loopback [127.0.0.1]) by d03av04.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q06Kgklv027835 for ; Fri, 6 Jan 2012 13:42:47 -0700 Message-ID: <4F075CC2.6010700@us.ibm.com> Date: Fri, 06 Jan 2012 14:42:42 -0600 From: Anthony Liguori MIME-Version: 1.0 References: <4F071111.6080306@us.ibm.com> <4F075371.4060904@web.de> In-Reply-To: <4F075371.4060904@web.de> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [RFC] QEMU Code Audit Team List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?ISO-8859-1?Q?Andreas_F=E4rber?= Cc: Chris Wright , Stefan Hajnoczi , Stefan Weil , Corey Bryant , qemu-devel , Avi Kivity On 01/06/2012 02:02 PM, Andreas Färber wrote: > Am 06.01.2012 16:19, schrieb Anthony Liguori: >> I'd like to start a more formal and transparent security audit of QEMU. >> The way I'd imagine it working is something like this: > > I'd like to propose something else: We should define a more formal > process for reviewing and applying patches in the first place. > > The better upfront review we have, the less issues to track down later. That's certainly a good goal, but it doesn't change that the fact that there's close to a million lines of code in tree that needs some attention... > > For example: > > i) Unless it's a build fix, I propose defining a minimum review time > before a patch is applied to a (sub)maintainer's queue. > Avi's I/O dispatch series was pulled into the trees two days after > posting it, which was definitely not enough time to review and test it. > For qemu-trivial by comparison we have a rather predictable weekly or > bi-weekly rhythm. > Otherwise we'll have to 'fearfully' spam the list with Reviewed-bys or > possible objections cluttering the list instead of reviewing the whole > series first and adding a summarized reply. I disagree here. If anything, I think we wait a bit too long for people to review things and that prevents progress. > ii) We regularly point newbies to SubmitAPatch and MAINTAINERS. Core > developers should respect those as well to give submaintainers a chance > to review and test before the merge: > "CC the relevant maintainer -- look in the MAINTAINERS file to find out > who that is" > git-send-email offers the Cc: line to help make people aware of > individual patches touching their subsystem within a large series. > If we don't have a maintainer on file for something we need to fix that. hint: if you add docs to the wiki on how to configure git-send-email to automagically do this by using scripts/get_maintainers.pl, more people will likely do it. I didn't even realize that that was the purpose of get_maintainers.pl until I was trolling through git-send-email's man page recently. > iii) The Git mailing list used to have regular "What's cooking" mails > listing patches that had been reviewed but not yet applied to master. > Sort of like Anthony's recent speak-now reminder or the former > aliguori-queue.git. > Maybe pull into a next branch and only merge from there into master > after a timeout? Not sure if it's worth the effort. We did something like this and I got a tremendous amount of negative feedback about it. > > iv) Given that i) and ii) are respected, a PULL request should be > applied within a reasonable time frame without resparking the basic > is-this-patch-doing-the-right-thing discussion since that should've > happened on the PATCHes earlier on. I don't think that ever really happens with PULL requests... The exception was during the release freeze because some submaintainers weren't respecting the freeze policy... > A PULL breaking the build is another > matter of course, but individual patches can still be reverted or > reworked afterwards. I won't take a PULL that breaks the build. I'm not going to revert patches either. That breaks bisecting which is a PITA. I don't make a big fuss about it when it happens, but honestly, I don't have a lot of sympathy for most build breaks as it usually happens because the requester neglected to do a full build before sending the request and/or patch. > Or should a PULL generally be re-reviewed within a > fixed timeframe, questionmark? If so, by whom? > It would be nice to have a more explicit process of who pulls from whom > and how this is handled during maintainers' absences - especially when > approaching a release. If queues do not get pulled into master in time, > then an orchestrated Test Day or Code Audit is not worth that much. I think we're still in a learning phase around PULL requests to be honest. I think things are working pretty well right now. 1.0 was one of our most solid releases, most patches are getting reviewed and applied in a reasonable time, and the build isn't breaking in horrible ways that often. Regards, Anthony Liguori > v) Posting static analysis reports is a good thing, but a Launchpad > attachment doesn't give them a lot of exposure. Would it be possible to > have a regular, differential textual report from some tools, similar to > how the Intel guys are summarizing test results for KVM? Maybe even > integrated with one of the buildbots? > As a simple example, false spelling positives in rarely changed > softfloat code might be filtered out by diff'ing against last week's report. > Or just a summary "For week w, $TOOL reported n new potential issues". > > Whatever we decide on, we should document it in the Wiki so that patch > contributors know ahead of time how patient they should be, for > reviewers to plan or to signal the maintainer an objection or > wait-condition in time, and for submaintainers to know how much time to > give other reviewers for comments or tags. > > Comments? Further or contrary suggestions? > > Andreas >