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

[Andreas Färber]

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.

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.

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.

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.

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. A PULL breaking the build is another
matter of course, but individual patches can still be reverted or
reworked afterwards. 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.

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