Re: [Qemu-devel] [RFC] QEMU Code Audit Team

[Anthony Liguori]

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