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

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

[Anthony Liguori]

Hi,

I had an idea I wanted to share and see what level of interest there was in 
participating and if anyone knows of a process that other projects follow for this.

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:

1) People volunteer to be part of the audit team

2) Two people walk through a particular piece of code and independently flag 
anything that looks like a potential security issue.

3) Two people independently review everything that's flagged to see if there's a 
security issue.

Step (3) is something that requires a fairly deep understanding of QEMU but step 
(2) is probably something that a lot of people could participate in.

I'd want to focus initially on the common PC devices.   The list isn't all that 
large and a review like this should only take a few hours to complete each step.

Would folks be interested in participating in something like this?  If so, I can 
start organizing it.

Regards,

Anthony Liguori

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

[Stefan Hajnoczi]

On Fri, Jan 06, 2012 at 09:19:45AM -0600, Anthony Liguori wrote:
> Would folks be interested in participating in something like this?
> If so, I can start organizing it.

I enjoy bug hunting and would volunteer.

Stefan

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

[Corey Bryant]

On 01/06/2012 10:19 AM, Anthony Liguori wrote:
> Hi,
>
> I had an idea I wanted to share and see what level of interest there was
> in participating and if anyone knows of a process that other projects
> follow for this.
>
> 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:
>
> 1) People volunteer to be part of the audit team
>
> 2) Two people walk through a particular piece of code and independently
> flag anything that looks like a potential security issue.
>
> 3) Two people independently review everything that's flagged to see if
> there's a security issue.
>
> Step (3) is something that requires a fairly deep understanding of QEMU
> but step (2) is probably something that a lot of people could
> participate in.
>
> I'd want to focus initially on the common PC devices. The list isn't all
> that large and a review like this should only take a few hours to
> complete each step.
>
> Would folks be interested in participating in something like this? If
> so, I can start organizing it.
>
> Regards,
>
> Anthony Liguori

Count me in for step 2.  A good approach may be to run a static analysis 
tool against the code, followed by a manual scan of the code for common 
vulnerabilities that static analysis can't find.

-- 
Regards,
Corey

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

[Stefan Weil]

Am 06.01.2012 17:01, schrieb Stefan Hajnoczi:
> On Fri, Jan 06, 2012 at 09:19:45AM -0600, Anthony Liguori wrote:
>> Would folks be interested in participating in something like this?
>> If so, I can start organizing it.
> I enjoy bug hunting and would volunteer.
>
> Stefan

So do I.

Stefan

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

[Chris Wright]

* Corey Bryant (coreyb@linux.vnet.ibm.com) wrote:
> Count me in for step 2.  A good approach may be to run a static
> analysis tool against the code, followed by a manual scan of the
> code for common vulnerabilities that static analysis can't find.

Good idea.  Folks are already running things like Coverity.  The false
positive rate is high enough that it's a lot to wade through at first
(so extra eyes could be quite helpful here).  Perhaps the people who
are involved in this could share some of their findings.

thanks,
-chris

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

[Chris Wright]

* Anthony Liguori (aliguori@us.ibm.com) wrote:
> 2) Two people walk through a particular piece of code and
> independently flag anything that looks like a potential security
> issue.

Auditing is always helpful, but won't ever get full coverage.  qtest +
fuzz is another great way to identify problems.  Also improving any
anotations to help static analysis tools is useful.  And both of those
are development efforts rather than code review.  Trouble with code
review is that security bugs can be subtle and easy to miss.

> I'd want to focus initially on the common PC devices.   The list
> isn't all that large and a review like this should only take a few
> hours to complete each step.

I definitely agree on the initial scope.

thanks,
-chris

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

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
>

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

[Peter Maydell]

On 6 January 2012 20:42, Anthony Liguori <aliguori@us.ibm.com> wrote:
> On 01/06/2012 02:02 PM, Andreas Färber wrote:
>> 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.

> I disagree here.  If anything, I think we wait a bit too long for people to
> review things and that prevents progress.

Actually I think it would be useful to agree on a "standard" time
for this kind of thing. A lot of the ARM related patches I do don't
get review, and it would be nice to know how long it's sensible to wait
until I can submit them in a pull request. (I don't want to cut
short time for people to review, but I don't want them languishing
on the list for weeks either...)

>> Or should a PULL generally be re-reviewed within a
>> fixed timeframe, questionmark?

We shouldn't be rereviewing pull requests -- they should be basically
equivalent to actual tree commit.

>> 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.

Agreed. In particular it would be nice to have a definite nominated
person who I ought to send target-arm pullreqs to, since all I know
for sure is that it's not Anthony :-)

-- PMM

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

[Stefan Hajnoczi]

On Sat, Jan 7, 2012 at 3:09 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 6 January 2012 20:42, Anthony Liguori <aliguori@us.ibm.com> wrote:
>> On 01/06/2012 02:02 PM, Andreas Färber wrote:
>>> 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.
>
>> I disagree here.  If anything, I think we wait a bit too long for people to
>> review things and that prevents progress.
>
> Actually I think it would be useful to agree on a "standard" time
> for this kind of thing. A lot of the ARM related patches I do don't
> get review, and it would be nice to know how long it's sensible to wait
> until I can submit them in a pull request. (I don't want to cut
> short time for people to review, but I don't want them languishing
> on the list for weeks either...)

I typically ping if there has been no activity for 1 week or more.

Introducing a wait period of more than a few days is probably not
going to add much review, perhaps the usual reviewers will just put
off reviewing until closer to the deadline.  Something like 2 days is
reasonable though, IMO.

Stefan

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

[Dor Laor]

On 01/06/2012 07:25 PM, Chris Wright wrote:
> * Corey Bryant (coreyb@linux.vnet.ibm.com) wrote:
>> Count me in for step 2.  A good approach may be to run a static
>> analysis tool against the code, followed by a manual scan of the
>> code for common vulnerabilities that static analysis can't find.
>
> Good idea.  Folks are already running things like Coverity.  The false
> positive rate is high enough that it's a lot to wade through at first
> (so extra eyes could be quite helpful here).  Perhaps the people who
> are involved in this could share some of their findings.

Markus already done a pretty extensive review and cleanup using 
Coverity. I'm not sure if he managed to cover all the real issues, have you?

btw: in case a real security flaw is detected, I like to ask the audit 
volunteering folks to report a CVE [1] and not to disclose the info till 
an embargo is raised.

I think that kvm and qemu need to have a security page like this:
http://www.webkit.org/security/

Cheers,
Dor

[1] http://oss-security.openwall.org/wiki/disclosure/cve
>
> thanks,
> -chris
>

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

[Stefan Hajnoczi]

On Sun, Jan 8, 2012 at 2:01 PM, Dor Laor <dlaor@redhat.com> wrote:
> On 01/06/2012 07:25 PM, Chris Wright wrote:
>>
>> * Corey Bryant (coreyb@linux.vnet.ibm.com) wrote:
>>>
>>> Count me in for step 2.  A good approach may be to run a static
>>> analysis tool against the code, followed by a manual scan of the
>>> code for common vulnerabilities that static analysis can't find.
>>
>>
>> Good idea.  Folks are already running things like Coverity.  The false
>> positive rate is high enough that it's a lot to wade through at first
>> (so extra eyes could be quite helpful here).  Perhaps the people who
>> are involved in this could share some of their findings.
>
>
> Markus already done a pretty extensive review and cleanup using Coverity.
> I'm not sure if he managed to cover all the real issues, have you?
>
> btw: in case a real security flaw is detected, I like to ask the audit
> volunteering folks to report a CVE [1] and not to disclose the info till an
> embargo is raised.

The process I have followed is to raise a Launchpad bug and tick "This
bug is a security vulnerability":
https://bugs.launchpad.net/qemu/+filebug

Either way, there needs to be simple instructions on how to submit
security vulnerability information and who gets to see that
information.

> I think that kvm and qemu need to have a security page like this:
> http://www.webkit.org/security/

Good idea.  Once there is a consensus I can write up a page on qemu.org.

Stefan

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

[Zhi Yong Wu]

On Fri, Jan 6, 2012 at 11:19 PM, Anthony Liguori <aliguori@us.ibm.com> wrote:
> Hi,
>
> I had an idea I wanted to share and see what level of interest there was in
> participating and if anyone knows of a process that other projects follow
> for this.
>
> 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:
>
> 1) People volunteer to be part of the audit team
>
> 2) Two people walk through a particular piece of code and independently flag
> anything that looks like a potential security issue.
>
> 3) Two people independently review everything that's flagged to see if
> there's a security issue.
>
> Step (3) is something that requires a fairly deep understanding of QEMU but
> step (2) is probably something that a lot of people could participate in.
>
> I'd want to focus initially on the common PC devices.   The list isn't all
> that large and a review like this should only take a few hours to complete
> each step.
>
> Would folks be interested in participating in something like this?  If so, I
> can start organizing it.
If could, i would like to be one volunteer.

>
> Regards,
>
> Anthony Liguori
>
>



-- 
Regards,

Zhi Yong Wu

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

[Kevin Wolf]

Am 07.01.2012 11:42, schrieb Stefan Hajnoczi:
> On Sat, Jan 7, 2012 at 3:09 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 6 January 2012 20:42, Anthony Liguori <aliguori@us.ibm.com> wrote:
>>> On 01/06/2012 02:02 PM, Andreas Färber wrote:
>>>> 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.
>>
>>> I disagree here.  If anything, I think we wait a bit too long for people to
>>> review things and that prevents progress.
>>
>> Actually I think it would be useful to agree on a "standard" time
>> for this kind of thing. A lot of the ARM related patches I do don't
>> get review, and it would be nice to know how long it's sensible to wait
>> until I can submit them in a pull request. (I don't want to cut
>> short time for people to review, but I don't want them languishing
>> on the list for weeks either...)
> 
> I typically ping if there has been no activity for 1 week or more.
> 
> Introducing a wait period of more than a few days is probably not
> going to add much review, perhaps the usual reviewers will just put
> off reviewing until closer to the deadline.  Something like 2 days is
> reasonable though, IMO.

What would this wait period really mean? If there hasn't been any review
within 2 day, the patch is considered correct and committed without any
review? Or is it the earliest that a maintainer may pick it up and do
the work himself?

Currently I usually apply patches when I have reviewed them, because I
know that very likely nobody else is going to review them anyway. This
can be as little as a few hours (though recently it's more often a few
weeks...). Then you have some time left to object until I actually send
the pull request, which isn't a fixed date either.

I can understand if you want to have more predictable times, but really,
when nobody else is going to review it anyway, what use would it be to
create even more management overhead for me?

Probably we need to attack the reviewing problem first: That I review
all block patches myself worked well as long as we were two or three
people in that area, but today it doesn't scale any more without
lowering the review standards - and I don't want to do that. Maybe we
should introduce something like "One Reviewed-by buys you two
Signed-off-bys for your own patches" ;-)

I can imagine that other subsystem maintainers have similar problems.

Kevin

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

[Anthony Liguori]

On 01/10/2012 06:58 AM, Kevin Wolf wrote:
> Probably we need to attack the reviewing problem first: That I review
> all block patches myself worked well as long as we were two or three
> people in that area, but today it doesn't scale any more without
> lowering the review standards - and I don't want to do that. Maybe we
> should introduce something like "One Reviewed-by buys you two
> Signed-off-bys for your own patches" ;-)

I think one thing that helps is to make sure for maintainers to include 
Reviewed-bys in commits.  The script I use (below) takes a mbox with the full 
thread and folks Reviewed-by/Tested-bys into the original patch spitting out an 
mbox with just the patches and tags.

That way people are getting credit in git for doing reviews.  It's a small 
incentive but every little bit helps.

http://git.codemonkey.ws/cgit/mbox-filter.git/

Regards,

Anthony Liguori

>
> I can imagine that other subsystem maintainers have similar problems.
>
> Kevin
>

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

[Kevin Wolf]

Am 10.01.2012 14:22, schrieb Anthony Liguori:
> On 01/10/2012 06:58 AM, Kevin Wolf wrote:
>> Probably we need to attack the reviewing problem first: That I review
>> all block patches myself worked well as long as we were two or three
>> people in that area, but today it doesn't scale any more without
>> lowering the review standards - and I don't want to do that. Maybe we
>> should introduce something like "One Reviewed-by buys you two
>> Signed-off-bys for your own patches" ;-)
> 
> I think one thing that helps is to make sure for maintainers to include 
> Reviewed-bys in commits.  The script I use (below) takes a mbox with the full 
> thread and folks Reviewed-by/Tested-bys into the original patch spitting out an 
> mbox with just the patches and tags.
> 
> That way people are getting credit in git for doing reviews.  It's a small 
> incentive but every little bit helps.
> 
> http://git.codemonkey.ws/cgit/mbox-filter.git/

I usually do that, although manually.

Of the 487 patches I have committed, 71 have a Reviewed-by tag in the
commit message. Maybe I've missed to include it for some, but that's
about the ratio that feels realistic to me.

Kevin

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

[Andreas Färber]

Am 10.01.2012 14:33, schrieb Kevin Wolf:
> Am 10.01.2012 14:22, schrieb Anthony Liguori:
>> On 01/10/2012 06:58 AM, Kevin Wolf wrote:
>>> Probably we need to attack the reviewing problem first: That I review
>>> all block patches myself worked well as long as we were two or three
>>> people in that area, but today it doesn't scale any more without
>>> lowering the review standards - and I don't want to do that. Maybe we
>>> should introduce something like "One Reviewed-by buys you two
>>> Signed-off-bys for your own patches" ;-)
>>
>> I think one thing that helps is to make sure for maintainers to include 
>> Reviewed-bys in commits.  The script I use (below) takes a mbox with the full 
>> thread and folks Reviewed-by/Tested-bys into the original patch spitting out an 
>> mbox with just the patches and tags.
>>
>> That way people are getting credit in git for doing reviews.  It's a small 
>> incentive but every little bit helps.
>>
>> http://git.codemonkey.ws/cgit/mbox-filter.git/
> 
> I usually do that, although manually.
> 
> Of the 487 patches I have committed, 71 have a Reviewed-by tag in the
> commit message. Maybe I've missed to include it for some, but that's
> about the ratio that feels realistic to me.

If you want an incentive, just put up a rule that every patch needs to
be reviewed by at least the submaintainer and one person apart from the
author (i.e., SoB + RB/AB + SoB). If a patch is lacking that additional
review, the author will ping the list.

Andreas

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

[Anthony Liguori]

On 01/10/2012 07:33 AM, Kevin Wolf wrote:
> Am 10.01.2012 14:22, schrieb Anthony Liguori:
>> On 01/10/2012 06:58 AM, Kevin Wolf wrote:
>>> Probably we need to attack the reviewing problem first: That I review
>>> all block patches myself worked well as long as we were two or three
>>> people in that area, but today it doesn't scale any more without
>>> lowering the review standards - and I don't want to do that. Maybe we
>>> should introduce something like "One Reviewed-by buys you two
>>> Signed-off-bys for your own patches" ;-)
>>
>> I think one thing that helps is to make sure for maintainers to include
>> Reviewed-bys in commits.  The script I use (below) takes a mbox with the full
>> thread and folks Reviewed-by/Tested-bys into the original patch spitting out an
>> mbox with just the patches and tags.
>>
>> That way people are getting credit in git for doing reviews.  It's a small
>> incentive but every little bit helps.
>>
>> http://git.codemonkey.ws/cgit/mbox-filter.git/
>
> I usually do that, although manually.
>
> Of the 487 patches I have committed, 71 have a Reviewed-by tag in the
> commit message. Maybe I've missed to include it for some, but that's
> about the ratio that feels realistic to me.

That's 14.6%.  My rate over the past year is 15.8% so you're probably catching 
the vast majority of them.

The tree overall is 12.6% for the entire year so I guess that means the majority 
of Reviewed-by's are being added.

Regards,

Anthony Liguori

>
> Kevin
>

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

[Kevin Wolf]

Am 10.01.2012 14:39, schrieb Andreas Färber:
> Am 10.01.2012 14:33, schrieb Kevin Wolf:
>> Am 10.01.2012 14:22, schrieb Anthony Liguori:
>>> On 01/10/2012 06:58 AM, Kevin Wolf wrote:
>>>> Probably we need to attack the reviewing problem first: That I review
>>>> all block patches myself worked well as long as we were two or three
>>>> people in that area, but today it doesn't scale any more without
>>>> lowering the review standards - and I don't want to do that. Maybe we
>>>> should introduce something like "One Reviewed-by buys you two
>>>> Signed-off-bys for your own patches" ;-)
>>>
>>> I think one thing that helps is to make sure for maintainers to include 
>>> Reviewed-bys in commits.  The script I use (below) takes a mbox with the full 
>>> thread and folks Reviewed-by/Tested-bys into the original patch spitting out an 
>>> mbox with just the patches and tags.
>>>
>>> That way people are getting credit in git for doing reviews.  It's a small 
>>> incentive but every little bit helps.
>>>
>>> http://git.codemonkey.ws/cgit/mbox-filter.git/
>>
>> I usually do that, although manually.
>>
>> Of the 487 patches I have committed, 71 have a Reviewed-by tag in the
>> commit message. Maybe I've missed to include it for some, but that's
>> about the ratio that feels realistic to me.
> 
> If you want an incentive, just put up a rule that every patch needs to
> be reviewed by at least the submaintainer and one person apart from the
> author (i.e., SoB + RB/AB + SoB). If a patch is lacking that additional
> review, the author will ping the list.

And get no response, delaying the patch forever.

I already tried this unintentionally when I had just too little time to
do the 85% myself. I told people that they should find someone else to
review their series if they want to have it committed. Stefan reviewed
some of them, but in most cases, the series ended up being reviewed by
myself some weeks later.

Kevin

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

[Peter Maydell]

On 10 January 2012 13:39, Andreas Färber <andreas.faerber@web.de> wrote:
> If you want an incentive, just put up a rule that every patch needs to
> be reviewed by at least the submaintainer and one person apart from the
> author (i.e., SoB + RB/AB + SoB). If a patch is lacking that additional
> review, the author will ping the list.

Were you volunteering to do that extra review for everybody?
It's hard enough getting one review, let alone two...

-- PMM

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

[Andreas Färber]

Am 10.01.2012 16:41, schrieb Peter Maydell:
> On 10 January 2012 13:39, Andreas Färber <andreas.faerber@web.de> wrote:
>> If you want an incentive, just put up a rule that every patch needs to
>> be reviewed by at least the submaintainer and one person apart from the
>> author (i.e., SoB + RB/AB + SoB). If a patch is lacking that additional
>> review, the author will ping the list.
> 
> Were you volunteering to do that extra review for everybody?

I have been quite a lot since doing virtualization full-time, haven't I?

I'm not volunteering for the block branch though, that's still dark
magic to me!

> It's hard enough getting one review, let alone two...

Given and take... so back to Kevin's proposal after all. ;)

/-F