[Qemu-devel] We need more reviewers/maintainers!!

[Qemu-devel] We need more reviewers/maintainers!!

[Stefano Stabellini]

Hi all,
I don't mean to steer any controversy or start any flame wars here, but
rather I want to point out a problem in the QEMU Community that is
preventing us and other people from having a good experience working
upstream with QEMU. Call it constructive criticism.

Patches are being posted to the list that don't get any reviews at all.
Other patches get reviewed the first time, then once they are reposted
they don't get any other reviews or acked-by or reviewed-by.

As a whole it takes biblical times to get through the QEMU review
process. I wonder how any commercial company with deadlines would be able
to cope with them. Even the Xen Community, that is far from a commercial
company, is having difficulties with them and now upstream QEMU is at
risk of missing the 4.2 release target.


We need more people reviewing patches. And we need more maintainers.


Anthony Liguori is still the maintainer for many areas within QEMU, and
he is clearly too busy for that. We need more people helping him review
patches for source files like savevm.c and vl.c.

I believe in leading by example, so Anthony Perard and I will try to
review more patch series, even outside Xen support in QEMU, starting
from now.
I hope more people will start to do the same to the point that it will
get natural to add more names and email addresses to the MAINTAINERS
file.

I hope that other people will recognize that this is a problem and be
willing to step up to find a solution.

Thanks,

Stefano

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Anthony Liguori]

On 03/12/2012 12:06 PM, Stefano Stabellini wrote:
> Hi all,
> I don't mean to steer any controversy or start any flame wars here, but
> rather I want to point out a problem in the QEMU Community that is
> preventing us and other people from having a good experience working
> upstream with QEMU. Call it constructive criticism.
>
> Patches are being posted to the list that don't get any reviews at all.
> Other patches get reviewed the first time, then once they are reposted
> they don't get any other reviews or acked-by or reviewed-by.

In all fairness, QEMU continues to grow year-to-year both in terms of total 
commits and number of contributors.

The area that we struggle with is infrequent contributors that contribute 
non-trivial things and are write-only contributors.

In this case, I really think the problem is expecting to be a write-only 
contributor.  Part of participating in a community is not only pushing your own 
patches for acceptance but also reviewing other people's patches and 
participating in the discussion.  If everyone only sends patches and doesn't 
review patches, then we'll never make progress.

So I'd strongly suggest trying to spend some time reviewing other people's work. 
  Right now, there are at least four different efforts around migration yet I 
don't see any of the people reviewing the other efforts.  I think this is really 
the main problem.

Regards,

Anthony Liguori

>
> As a whole it takes biblical times to get through the QEMU review
> process. I wonder how any commercial company with deadlines would be able
> to cope with them. Even the Xen Community, that is far from a commercial
> company, is having difficulties with them and now upstream QEMU is at
> risk of missing the 4.2 release target.
>
>
> We need more people reviewing patches. And we need more maintainers.
>
>
> Anthony Liguori is still the maintainer for many areas within QEMU, and
> he is clearly too busy for that. We need more people helping him review
> patches for source files like savevm.c and vl.c.
>
> I believe in leading by example, so Anthony Perard and I will try to
> review more patch series, even outside Xen support in QEMU, starting
> from now.
> I hope more people will start to do the same to the point that it will
> get natural to add more names and email addresses to the MAINTAINERS
> file.
>
> I hope that other people will recognize that this is a problem and be
> willing to step up to find a solution.
>
> Thanks,
>
> Stefano

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Stefano Stabellini]

On Mon, 12 Mar 2012, Anthony Liguori wrote:
> On 03/12/2012 12:06 PM, Stefano Stabellini wrote:
> > Hi all,
> > I don't mean to steer any controversy or start any flame wars here, but
> > rather I want to point out a problem in the QEMU Community that is
> > preventing us and other people from having a good experience working
> > upstream with QEMU. Call it constructive criticism.
> >
> > Patches are being posted to the list that don't get any reviews at all.
> > Other patches get reviewed the first time, then once they are reposted
> > they don't get any other reviews or acked-by or reviewed-by.
> 
> In all fairness, QEMU continues to grow year-to-year both in terms of total 
> commits and number of contributors.
> 
> The area that we struggle with is infrequent contributors that contribute 
> non-trivial things and are write-only contributors.
> 
> In this case, I really think the problem is expecting to be a write-only 
> contributor.  Part of participating in a community is not only pushing your own 
> patches for acceptance but also reviewing other people's patches and 
> participating in the discussion.  If everyone only sends patches and doesn't 
> review patches, then we'll never make progress.
> 
> So I'd strongly suggest trying to spend some time reviewing other people's work. 
>   Right now, there are at least four different efforts around migration yet I 
> don't see any of the people reviewing the other efforts.  I think this is really 
> the main problem.

Point taken.
However maintainers should also be responsible of reviewing patches of
"infrequent write-only contributors".

I certainly do it for the areas I am a maintainer of, and in general we
try to do it on xen-devel. Overall I think we are mostly succeeding even
though admittedly the traffic is lower than qemu-devel.
Maybe we just need more maintainers?

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Lluís Vilanova]

Stefano Stabellini writes:
[...]
> Patches are being posted to the list that don't get any reviews at all.
> Other patches get reviewed the first time, then once they are reposted
> they don't get any other reviews or acked-by or reviewed-by.

What are the different tags available and what is their semantic? Maybe this
should be written down somewhere (HACKING?).

I'll contribute a starting list of tags
(http://wiki.x.org/wiki/Development/Documentation/SubmittingPatches), please
check these are the desired semantics for this project.

* Cc: Full Name <email>

  In addition to sending the patches to the mailing list, they must be Cc'ed to,
  at least, the maintainers for the affected files. You can look up the
  maintainers for each file in the MAINTAINERS file.

  The command "git send-email" will automatically look for such a tag in order
  to send a CC of the mail to the given person.

  Document maintainer discovery with 'scripts/get_maintainer.pl'?


* Acked-by: Full Name <email>

  If a person was not directly involved in the preparation or handling of a
  patch but wishes to signify and record their approval of it then they can
  arrange to have an Acked-by: line. Acked-by: does not necessarily indicate
  acknowledgement of the entire patch.


* Tested-by: Full Name <email>

  A Tested-by: tag indicates that the patch has been successfully tested (in
  some environment) by the person named. This tag informs maintainers that some
  testing has been performed, provides a means to locate testers for future
  patches, and ensures credit for the testers.


* Reviewed-by: Full Name <email>

  A Reviewed-by tag is a statement of opinion that the patch is an appropriate
  modification without any remaining serious technical issues. Any interested
  reviewer (who has done the work) can offer a Reviewed-by tag for a patch.


My understanding until now was that both Acked-by and Reviewed-by were tags
reserved to people with privileges to write into the repository.



Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Anthony Liguori]

On 03/12/2012 01:03 PM, Lluís Vilanova wrote:
> Stefano Stabellini writes:
> [...]
>> Patches are being posted to the list that don't get any reviews at all.
>> Other patches get reviewed the first time, then once they are reposted
>> they don't get any other reviews or acked-by or reviewed-by.
>
> What are the different tags available and what is their semantic? Maybe this
> should be written down somewhere (HACKING?).
>
> I'll contribute a starting list of tags
> (http://wiki.x.org/wiki/Development/Documentation/SubmittingPatches), please
> check these are the desired semantics for this project.
>
> * Cc: Full Name<email>
>
>    In addition to sending the patches to the mailing list, they must be Cc'ed to,
>    at least, the maintainers for the affected files. You can look up the
>    maintainers for each file in the MAINTAINERS file.
>
>    The command "git send-email" will automatically look for such a tag in order
>    to send a CC of the mail to the given person.
>
>    Document maintainer discovery with 'scripts/get_maintainer.pl'?
>
>
> * Acked-by: Full Name<email>
>
>    If a person was not directly involved in the preparation or handling of a
>    patch but wishes to signify and record their approval of it then they can
>    arrange to have an Acked-by: line. Acked-by: does not necessarily indicate
>    acknowledgement of the entire patch.
>
>
> * Tested-by: Full Name<email>
>
>    A Tested-by: tag indicates that the patch has been successfully tested (in
>    some environment) by the person named. This tag informs maintainers that some
>    testing has been performed, provides a means to locate testers for future
>    patches, and ensures credit for the testers.
>
>
> * Reviewed-by: Full Name<email>
>
>    A Reviewed-by tag is a statement of opinion that the patch is an appropriate
>    modification without any remaining serious technical issues. Any interested
>    reviewer (who has done the work) can offer a Reviewed-by tag for a patch.
>
>
> My understanding until now was that both Acked-by and Reviewed-by were tags
> reserved to people with privileges to write into the repository.

That's interesting feedback.  These are all documented throughly in Linux's 
SubmittingPatches (which I think ours refers to).

Specifically:

13) When to use Acked-by: and Cc:

The Signed-off-by: tag indicates that the signer was involved in the
development of the patch, or that he/she was in the patch's delivery path.

If a person was not directly involved in the preparation or handling of a
patch but wishes to signify and record their approval of it then they can
arrange to have an Acked-by: line added to the patch's changelog.

Acked-by: is often used by the maintainer of the affected code when that
maintainer neither contributed to nor forwarded the patch.

Acked-by: is not as formal as Signed-off-by:.  It is a record that the acker
has at least reviewed the patch and has indicated acceptance.  Hence patch
mergers will sometimes manually convert an acker's "yep, looks good to me"
into an Acked-by:.

Acked-by: does not necessarily indicate acknowledgement of the entire patch.
For example, if a patch affects multiple subsystems and has an Acked-by: from
one subsystem maintainer then this usually indicates acknowledgement of just
the part which affects that maintainer's code.  Judgement should be used here.
When in doubt people should refer to the original discussion in the mailing
list archives.

If a person has had the opportunity to comment on a patch, but has not
provided such comments, you may optionally add a "Cc:" tag to the patch.
This is the only tag which might be added without an explicit action by the
person it names.  This tag documents that potentially interested parties
have been included in the discussion


14) Using Reported-by:, Tested-by: and Reviewed-by:

If this patch fixes a problem reported by somebody else, consider adding a
Reported-by: tag to credit the reporter for their contribution.  Please
note that this tag should not be added without the reporter's permission,
especially if the problem was not reported in a public forum.  That said,
if we diligently credit our bug reporters, they will, hopefully, be
inspired to help us again in the future.

A Tested-by: tag indicates that the patch has been successfully tested (in
some environment) by the person named.  This tag informs maintainers that
some testing has been performed, provides a means to locate testers for
future patches, and ensures credit for the testers.

Reviewed-by:, instead, indicates that the patch has been reviewed and found
acceptable according to the Reviewer's Statement:

	Reviewer's statement of oversight

	By offering my Reviewed-by: tag, I state that:

  	 (a) I have carried out a technical review of this patch to
	     evaluate its appropriateness and readiness for inclusion into
	     the mainline kernel.

	 (b) Any problems, concerns, or questions relating to the patch
	     have been communicated back to the submitter.  I am satisfied
	     with the submitter's response to my comments.

	 (c) While there may be things that could be improved with this
	     submission, I believe that it is, at this time, (1) a
	     worthwhile modification to the kernel, and (2) free of known
	     issues which would argue against its inclusion.

	 (d) While I have reviewed the patch and believe it to be sound, I
	     do not (unless explicitly stated elsewhere) make any
	     warranties or guarantees that it will achieve its stated
	     purpose or function properly in any given situation.

A Reviewed-by tag is a statement of opinion that the patch is an
appropriate modification of the kernel without any remaining serious
technical issues.  Any interested reviewer (who has done the work) can
offer a Reviewed-by tag for a patch.  This tag serves to give credit to
reviewers and to inform maintainers of the degree of review which has been
done on the patch.  Reviewed-by: tags, when supplied by reviewers known to
understand the subject area and to perform thorough reviews, will normally
increase the likelihood of your patch getting into the kernel.

Regards,

Anthony Liguori

>
>
>
> Lluis
>

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 2327 bytes --]

On Mon, 12 Mar 2012, Lluís Vilanova wrote:
> Stefano Stabellini writes:
> [...]
> > Patches are being posted to the list that don't get any reviews at all.
> > Other patches get reviewed the first time, then once they are reposted
> > they don't get any other reviews or acked-by or reviewed-by.
> 
> What are the different tags available and what is their semantic? Maybe this
> should be written down somewhere (HACKING?).
> 
> I'll contribute a starting list of tags
> (http://wiki.x.org/wiki/Development/Documentation/SubmittingPatches), please
> check these are the desired semantics for this project.
> 
> * Cc: Full Name <email>
> 
>   In addition to sending the patches to the mailing list, they must be Cc'ed to,
>   at least, the maintainers for the affected files. You can look up the
>   maintainers for each file in the MAINTAINERS file.
> 
>   The command "git send-email" will automatically look for such a tag in order
>   to send a CC of the mail to the given person.
> 
>   Document maintainer discovery with 'scripts/get_maintainer.pl'?
> 
> 
> * Acked-by: Full Name <email>
> 
>   If a person was not directly involved in the preparation or handling of a
>   patch but wishes to signify and record their approval of it then they can
>   arrange to have an Acked-by: line. Acked-by: does not necessarily indicate
>   acknowledgement of the entire patch.
> 
> 
> * Tested-by: Full Name <email>
> 
>   A Tested-by: tag indicates that the patch has been successfully tested (in
>   some environment) by the person named. This tag informs maintainers that some
>   testing has been performed, provides a means to locate testers for future
>   patches, and ensures credit for the testers.
> 
> 
> * Reviewed-by: Full Name <email>
> 
>   A Reviewed-by tag is a statement of opinion that the patch is an appropriate
>   modification without any remaining serious technical issues. Any interested
>   reviewer (who has done the work) can offer a Reviewed-by tag for a patch.
> 
> 
> My understanding until now was that both Acked-by and Reviewed-by were tags
> reserved to people with privileges to write into the repository.

Anybody should be allowed to give his own Acked-by or Reviewed-by, not
just maintainers. Of course an acked-by from the maintainer of the area
the patch is touching has a different weight.

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Anthony Liguori]

On 03/12/2012 12:34 PM, Stefano Stabellini wrote:
> On Mon, 12 Mar 2012, Anthony Liguori wrote:
>> On 03/12/2012 12:06 PM, Stefano Stabellini wrote:
>>> Hi all,
>>> I don't mean to steer any controversy or start any flame wars here, but
>>> rather I want to point out a problem in the QEMU Community that is
>>> preventing us and other people from having a good experience working
>>> upstream with QEMU. Call it constructive criticism.
>>>
>>> Patches are being posted to the list that don't get any reviews at all.
>>> Other patches get reviewed the first time, then once they are reposted
>>> they don't get any other reviews or acked-by or reviewed-by.
>>
>> In all fairness, QEMU continues to grow year-to-year both in terms of total
>> commits and number of contributors.
>>
>> The area that we struggle with is infrequent contributors that contribute
>> non-trivial things and are write-only contributors.
>>
>> In this case, I really think the problem is expecting to be a write-only
>> contributor.  Part of participating in a community is not only pushing your own
>> patches for acceptance but also reviewing other people's patches and
>> participating in the discussion.  If everyone only sends patches and doesn't
>> review patches, then we'll never make progress.
>>
>> So I'd strongly suggest trying to spend some time reviewing other people's work.
>>    Right now, there are at least four different efforts around migration yet I
>> don't see any of the people reviewing the other efforts.  I think this is really
>> the main problem.
>
> Point taken.
> However maintainers should also be responsible of reviewing patches of
> "infrequent write-only contributors".
>
> I certainly do it for the areas I am a maintainer of, and in general we
> try to do it on xen-devel. Overall I think we are mostly succeeding even
> though admittedly the traffic is lower than qemu-devel.
> Maybe we just need more maintainers?

Yes, we do.  But as Paul Brook likes to say, in order to be a maintainer, you 
have to be willing to say no, not just apply patches.

It's not a question of maintainers, it's a question of people providing critical 
review of patches.

Regards,

Anthony Liguori

>

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Anthony Liguori]

On 03/12/2012 02:10 PM, Stefano Stabellini wrote:
> On Mon, 12 Mar 2012, Anthony Liguori wrote:
>> On 03/12/2012 12:34 PM, Stefano Stabellini wrote:
>>> On Mon, 12 Mar 2012, Anthony Liguori wrote:
>>>> On 03/12/2012 12:06 PM, Stefano Stabellini wrote:
>>>>> Hi all,
>>>>> I don't mean to steer any controversy or start any flame wars here, but
>>>>> rather I want to point out a problem in the QEMU Community that is
>>>>> preventing us and other people from having a good experience working
>>>>> upstream with QEMU. Call it constructive criticism.
>>>>>
>>>>> Patches are being posted to the list that don't get any reviews at all.
>>>>> Other patches get reviewed the first time, then once they are reposted
>>>>> they don't get any other reviews or acked-by or reviewed-by.
>>>>
>>>> In all fairness, QEMU continues to grow year-to-year both in terms of total
>>>> commits and number of contributors.
>>>>
>>>> The area that we struggle with is infrequent contributors that contribute
>>>> non-trivial things and are write-only contributors.
>>>>
>>>> In this case, I really think the problem is expecting to be a write-only
>>>> contributor.  Part of participating in a community is not only pushing your own
>>>> patches for acceptance but also reviewing other people's patches and
>>>> participating in the discussion.  If everyone only sends patches and doesn't
>>>> review patches, then we'll never make progress.
>>>>
>>>> So I'd strongly suggest trying to spend some time reviewing other people's work.
>>>>     Right now, there are at least four different efforts around migration yet I
>>>> don't see any of the people reviewing the other efforts.  I think this is really
>>>> the main problem.
>>>
>>> Point taken.
>>> However maintainers should also be responsible of reviewing patches of
>>> "infrequent write-only contributors".
>>>
>>> I certainly do it for the areas I am a maintainer of, and in general we
>>> try to do it on xen-devel. Overall I think we are mostly succeeding even
>>> though admittedly the traffic is lower than qemu-devel.
>>> Maybe we just need more maintainers?
>>
>> Yes, we do.  But as Paul Brook likes to say, in order to be a maintainer, you
>> have to be willing to say no, not just apply patches.
>>
>> It's not a question of maintainers, it's a question of people providing critical
>> review of patches.
>
> Right, but if one's name is right below a particular subsystem in the
> MAINTAINERS file, one should be the one in charge of providing a timely
> review to all the patches that touch that subsystem.

Note that MAINTAINERS lacks an entry for savevm.c.  That should imply M: Orphan.

>
> If you one is a maintainer and one is silently ignoring a patch touching
> one's subsystem, then one is not doing a good job as a maintainer.
> Of course if one is a maintainer and rather than giving useful feedback,
> limits the reply to a statement like "No", is also not doing a very good
> job.
> Do we all agree on these basic principles?

It's more complicated than that in a large project.  MAINTAINERS has different 
support levels.  I think what you're proposing is M: Supported.

M: Odd fixes (which is what I proposed savevm.c as) is less rigorous than that.

Regards,

Anthony Liguori

Regards,

Anthony Liguori

>
> If it is not the case, and you don't think this is the role of a QEMU
> maintainer, then maybe we need to invent a new name for a new role that
> covers that function.

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Stefano Stabellini]

On Mon, 12 Mar 2012, Anthony Liguori wrote:
> On 03/12/2012 12:34 PM, Stefano Stabellini wrote:
> > On Mon, 12 Mar 2012, Anthony Liguori wrote:
> >> On 03/12/2012 12:06 PM, Stefano Stabellini wrote:
> >>> Hi all,
> >>> I don't mean to steer any controversy or start any flame wars here, but
> >>> rather I want to point out a problem in the QEMU Community that is
> >>> preventing us and other people from having a good experience working
> >>> upstream with QEMU. Call it constructive criticism.
> >>>
> >>> Patches are being posted to the list that don't get any reviews at all.
> >>> Other patches get reviewed the first time, then once they are reposted
> >>> they don't get any other reviews or acked-by or reviewed-by.
> >>
> >> In all fairness, QEMU continues to grow year-to-year both in terms of total
> >> commits and number of contributors.
> >>
> >> The area that we struggle with is infrequent contributors that contribute
> >> non-trivial things and are write-only contributors.
> >>
> >> In this case, I really think the problem is expecting to be a write-only
> >> contributor.  Part of participating in a community is not only pushing your own
> >> patches for acceptance but also reviewing other people's patches and
> >> participating in the discussion.  If everyone only sends patches and doesn't
> >> review patches, then we'll never make progress.
> >>
> >> So I'd strongly suggest trying to spend some time reviewing other people's work.
> >>    Right now, there are at least four different efforts around migration yet I
> >> don't see any of the people reviewing the other efforts.  I think this is really
> >> the main problem.
> >
> > Point taken.
> > However maintainers should also be responsible of reviewing patches of
> > "infrequent write-only contributors".
> >
> > I certainly do it for the areas I am a maintainer of, and in general we
> > try to do it on xen-devel. Overall I think we are mostly succeeding even
> > though admittedly the traffic is lower than qemu-devel.
> > Maybe we just need more maintainers?
> 
> Yes, we do.  But as Paul Brook likes to say, in order to be a maintainer, you 
> have to be willing to say no, not just apply patches.
> 
> It's not a question of maintainers, it's a question of people providing critical 
> review of patches.

Right, but if one's name is right below a particular subsystem in the
MAINTAINERS file, one should be the one in charge of providing a timely
review to all the patches that touch that subsystem.

If you one is a maintainer and one is silently ignoring a patch touching
one's subsystem, then one is not doing a good job as a maintainer.
Of course if one is a maintainer and rather than giving useful feedback,
limits the reply to a statement like "No", is also not doing a very good
job.
Do we all agree on these basic principles?

If it is not the case, and you don't think this is the role of a QEMU
maintainer, then maybe we need to invent a new name for a new role that
covers that function.

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Michael Roth]

On Mon, Mar 12, 2012 at 05:06:15PM +0000, Stefano Stabellini wrote:
> Hi all,
> I don't mean to steer any controversy or start any flame wars here, but
> rather I want to point out a problem in the QEMU Community that is
> preventing us and other people from having a good experience working
> upstream with QEMU. Call it constructive criticism.
> 
> Patches are being posted to the list that don't get any reviews at all.
> Other patches get reviewed the first time, then once they are reposted
> they don't get any other reviews or acked-by or reviewed-by.
> 
> As a whole it takes biblical times to get through the QEMU review
> process. I wonder how any commercial company with deadlines would be able
> to cope with them. Even the Xen Community, that is far from a commercial
> company, is having difficulties with them and now upstream QEMU is at
> risk of missing the 4.2 release target.
> 
> 
> We need more people reviewing patches. And we need more maintainers.
> 
> 
> Anthony Liguori is still the maintainer for many areas within QEMU, and
> he is clearly too busy for that. We need more people helping him review
> patches for source files like savevm.c and vl.c.
> 
> I believe in leading by example, so Anthony Perard and I will try to
> review more patch series, even outside Xen support in QEMU, starting
> from now.
> I hope more people will start to do the same to the point that it will
> get natural to add more names and email addresses to the MAINTAINERS
> file.
> 
> I hope that other people will recognize that this is a problem and be
> willing to step up to find a solution.
> 
> Thanks,
> 
> Stefano
> 

Thanks Stefano. I plan on doing a lot of work with migration in the
future, and as such try to keep tabs on the migration-related stuff on
qemu-devel. I often don't get around to actually reviewing things
though, and that's been nagging me for a while now, so this is a good
point for me to start doing a better job of it. I'll try to review any
migration stuff I see, and anyone needing a second set of eyes feel free
to CC me or ping me on IRC.

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Stefano Stabellini]

On Mon, 12 Mar 2012, Anthony Liguori wrote:
> >>> "infrequent write-only contributors".
> >>>
> >>> I certainly do it for the areas I am a maintainer of, and in general we
> >>> try to do it on xen-devel. Overall I think we are mostly succeeding even
> >>> though admittedly the traffic is lower than qemu-devel.
> >>> Maybe we just need more maintainers?
> >>
> >> Yes, we do.  But as Paul Brook likes to say, in order to be a maintainer, you
> >> have to be willing to say no, not just apply patches.
> >>
> >> It's not a question of maintainers, it's a question of people providing critical
> >> review of patches.
> >
> > Right, but if one's name is right below a particular subsystem in the
> > MAINTAINERS file, one should be the one in charge of providing a timely
> > review to all the patches that touch that subsystem.
> 
> Note that MAINTAINERS lacks an entry for savevm.c.  That should imply M: Orphan.
> 
> >
> > If you one is a maintainer and one is silently ignoring a patch touching
> > one's subsystem, then one is not doing a good job as a maintainer.
> > Of course if one is a maintainer and rather than giving useful feedback,
> > limits the reply to a statement like "No", is also not doing a very good
> > job.
> > Do we all agree on these basic principles?
> 
> It's more complicated than that in a large project.  MAINTAINERS has different 
> support levels.  I think what you're proposing is M: Supported.
> 
> M: Odd fixes (which is what I proposed savevm.c as) is less rigorous than that.

OK, so the actual problem seems to be that not all the source files that
are supposed to be Supported are actually supported.
And of course some key files, like savevm.c are not even Maintained!!
For example if I am not mistaken we are missing an entry for vga/cirrus,
so that would also be Orphan, correct?

The situation is actually worse than I thought!

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Anthony Liguori]

On 03/12/2012 02:21 PM, Stefano Stabellini wrote:
> On Mon, 12 Mar 2012, Anthony Liguori wrote:
>>>>> "infrequent write-only contributors".
>>>>>
>>>>> I certainly do it for the areas I am a maintainer of, and in general we
>>>>> try to do it on xen-devel. Overall I think we are mostly succeeding even
>>>>> though admittedly the traffic is lower than qemu-devel.
>>>>> Maybe we just need more maintainers?
>>>>
>>>> Yes, we do.  But as Paul Brook likes to say, in order to be a maintainer, you
>>>> have to be willing to say no, not just apply patches.
>>>>
>>>> It's not a question of maintainers, it's a question of people providing critical
>>>> review of patches.
>>>
>>> Right, but if one's name is right below a particular subsystem in the
>>> MAINTAINERS file, one should be the one in charge of providing a timely
>>> review to all the patches that touch that subsystem.
>>
>> Note that MAINTAINERS lacks an entry for savevm.c.  That should imply M: Orphan.
>>
>>>
>>> If you one is a maintainer and one is silently ignoring a patch touching
>>> one's subsystem, then one is not doing a good job as a maintainer.
>>> Of course if one is a maintainer and rather than giving useful feedback,
>>> limits the reply to a statement like "No", is also not doing a very good
>>> job.
>>> Do we all agree on these basic principles?
>>
>> It's more complicated than that in a large project.  MAINTAINERS has different
>> support levels.  I think what you're proposing is M: Supported.
>>
>> M: Odd fixes (which is what I proposed savevm.c as) is less rigorous than that.
>
> OK, so the actual problem seems to be that not all the source files that
> are supposed to be Supported are actually supported.
> And of course some key files, like savevm.c are not even Maintained!!
> For example if I am not mistaken we are missing an entry for vga/cirrus,
> so that would also be Orphan, correct?

More likely Odd Fixes.  I think most of the things not listed are Odd Fixes 
which for something like cirrus is fine.  I doubt it's ever going to see a big 
invasive change as the code hasn't changed fundamentally in years.

But I agree, migration is an area that we need a proper maintainer for and 
hopefully this thread will help make that happen.

Regards,

Anthony Liguroi

>
> The situation is actually worse than I thought!
>

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Lluís Vilanova]

Anthony Liguori writes:
[...]
>> My understanding until now was that both Acked-by and Reviewed-by were tags
>> reserved to people with privileges to write into the repository.

> That's interesting feedback.  These are all documented throughly in Linux's
> SubmittingPatches (which I think ours refers to).

It does indeed link to that file, but when referring to the "Signed-off-by" tag.

Maybe adding a line there pointing the user to look for a description of
relevant tags in sections 12 to 14 might help.

Still, it's not completely clear to me the difference between review and ack.

BTW, the links in the wiki pointing to CODING_STYLE and HACKING are broken, but
it seems that I forgot my user name or it has been deleted, and I can no longer
add an account.


Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Anthony Liguori]

On 03/12/2012 02:39 PM, Lluís Vilanova wrote:
> Anthony Liguori writes:
> [...]
>>> My understanding until now was that both Acked-by and Reviewed-by were tags
>>> reserved to people with privileges to write into the repository.
>
>> That's interesting feedback.  These are all documented throughly in Linux's
>> SubmittingPatches (which I think ours refers to).
>
> It does indeed link to that file, but when referring to the "Signed-off-by" tag.
>
> Maybe adding a line there pointing the user to look for a description of
> relevant tags in sections 12 to 14 might help.
>
> Still, it's not completely clear to me the difference between review and ack.

Reviewed-by == you have reviewed a patch in detail and were not able to spot any 
issues with it.  But you are not claiming that you've tested the patch.

Acked-by == weaker than Reviewed-by and usually reserved for submaintainers or 
for someone who needs to approve a change.  It's an indication that you agree 
with the principle of the patch but haven't reviewed it thoroughly.

> BTW, the links in the wiki pointing to CODING_STYLE and HACKING are broken, but
> it seems that I forgot my user name or it has been deleted, and I can no longer
> add an account.

Send me a private note with your desired username.

Regards,

Anthony Liguori

>
>
> Lluis
>

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Stefan Weil]

Am 12.03.2012 18:06, schrieb Stefano Stabellini:
> Hi all,
> I don't mean to steer any controversy or start any flame wars here, but
> rather I want to point out a problem in the QEMU Community that is
> preventing us and other people from having a good experience working
> upstream with QEMU. Call it constructive criticism.
>
> Patches are being posted to the list that don't get any reviews at all.
> Other patches get reviewed the first time, then once they are reposted
> they don't get any other reviews or acked-by or reviewed-by.
>
> As a whole it takes biblical times to get through the QEMU review
> process. I wonder how any commercial company with deadlines would be able
> to cope with them. Even the Xen Community, that is far from a commercial
> company, is having difficulties with them and now upstream QEMU is at
> risk of missing the 4.2 release target.
>
>
> We need more people reviewing patches. And we need more maintainers.
>
>
> Anthony Liguori is still the maintainer for many areas within QEMU, and
> he is clearly too busy for that. We need more people helping him review
> patches for source files like savevm.c and vl.c.
>
> I believe in leading by example, so Anthony Perard and I will try to
> review more patch series, even outside Xen support in QEMU, starting
> from now.
> I hope more people will start to do the same to the point that it will
> get natural to add more names and email addresses to the MAINTAINERS
> file.
>
> I hope that other people will recognize that this is a problem and be
> willing to step up to find a solution.
>
> Thanks,
>
> Stefano

I agree that more maintainers would be good, but we also need
more people with commit rights. Why? There a many examples of
urgent patches (= patches which fix broken builds) which take
several days even when they were reviewed before they finally
are committed.

We also need more resources for technical maintenance of the
QEMU infrastructure. For example, the official mirror of the
QEMU git repository (https://github.com/qemu/QEMU) is several
months behind, http://git.savannah.gnu.org/cgit/qemu.git is
even older. It should be possible to have three mirrors which
are nearly up to date (like http://repo.or.cz/w/qemu.git/).

Only two maintainers are allowed to make full use of the patchwork
(http://patchwork.ozlabs.org/project/qemu-devel/) infrastructure.
Why not all maintainers?

Maybe every maintainer can maintain a short summary of
what he maintains, how (s)he does it (repository, expected
response time, ...) in the QEMU wiki. I just added
http://wiki.qemu.org/Contribute/StartHere#Maintainers
and improved my own wiki page ("leading by example :-)").

Thanks + regards

Stefan W.

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Peter Maydell]

On 12 March 2012 20:12, Stefan Weil <sw@weilnetz.de> wrote:
> I agree that more maintainers would be good, but we also need
> more people with commit rights. Why? There a many examples of
> urgent patches (= patches which fix broken builds) which take
> several days even when they were reviewed before they finally
> are committed.

I agree that that's a specific area it would be nice to do
better in. It seems to me that the qemu-trivial process for
sweeping up trivial patches has been working well; maybe we
could use a slightly more formal qemu-urgent process for
flagging up build breakage etc?

(Personally I'd support a rule that any outstanding
build-breakage fixes must always go in before anything else.)

> Only two maintainers are allowed to make full use of the patchwork
> (http://patchwork.ozlabs.org/project/qemu-devel/) infrastructure.
> Why not all maintainers?

I tried to get myself added to the maintainers list (with
agreement from Anthony) for that a long time ago and got zero
response from the people running that patchwork instance.

A good patchwork instance is really useful -- the Linaro one
is set up with hooks into monitoring git, so patches that are
committed move automatically to 'accepted', for instance.
[It's only semi-automatic, though, and I don't know if it
would still be as useful with the much higher volume qemu gets.]

-- PMM

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Anthony Liguori]

On 03/12/2012 03:12 PM, Stefan Weil wrote:
> Am 12.03.2012 18:06, schrieb Stefano Stabellini:
>> Hi all,
>> I don't mean to steer any controversy or start any flame wars here, but
>> rather I want to point out a problem in the QEMU Community that is
>> preventing us and other people from having a good experience working
>> upstream with QEMU. Call it constructive criticism.
>>
>> Patches are being posted to the list that don't get any reviews at all.
>> Other patches get reviewed the first time, then once they are reposted
>> they don't get any other reviews or acked-by or reviewed-by.
>>
>> As a whole it takes biblical times to get through the QEMU review
>> process. I wonder how any commercial company with deadlines would be able
>> to cope with them. Even the Xen Community, that is far from a commercial
>> company, is having difficulties with them and now upstream QEMU is at
>> risk of missing the 4.2 release target.
>>
>>
>> We need more people reviewing patches. And we need more maintainers.
>>
>>
>> Anthony Liguori is still the maintainer for many areas within QEMU, and
>> he is clearly too busy for that. We need more people helping him review
>> patches for source files like savevm.c and vl.c.
>>
>> I believe in leading by example, so Anthony Perard and I will try to
>> review more patch series, even outside Xen support in QEMU, starting
>> from now.
>> I hope more people will start to do the same to the point that it will
>> get natural to add more names and email addresses to the MAINTAINERS
>> file.
>>
>> I hope that other people will recognize that this is a problem and be
>> willing to step up to find a solution.
>>
>> Thanks,
>>
>> Stefano
>
> I agree that more maintainers would be good, but we also need
> more people with commit rights.

I disagree strongly.  Having multiple pushers makes things difficult and 
encourages people to push without testing.  Part of what makes pushing take 
longer than it should today is that my test cycle takes at least 1-2 hours and 
it's not uncommon to have to go through 3-4 cycles of rebasing before being able 
to push.

> Why? There a many examples of
> urgent patches (= patches which fix broken builds) which take
> several days even when they were reviewed before they finally
> are committed.

Can you be specific?  I think you really mean, "urgent patches for Win32" but 
since you're the win32 maintainer, it's on you to do a PULL request.

> We also need more resources for technical maintenance of the
> QEMU infrastructure. For example, the official mirror of the
> QEMU git repository (https://github.com/qemu/QEMU) is several
> months behind, http://git.savannah.gnu.org/cgit/qemu.git is
> even older.

Savannah is a dead tree.  I should just remove it.

Re: github, this hasn't seemed that urgent to me.

> It should be possible to have three mirrors which
> are nearly up to date (like http://repo.or.cz/w/qemu.git/).
>
> Only two maintainers are allowed to make full use of the patchwork
> (http://patchwork.ozlabs.org/project/qemu-devel/) infrastructure.
> Why not all maintainers?
>
> Maybe every maintainer can maintain a short summary of
> what he maintains, how (s)he does it (repository, expected
> response time, ...) in the QEMU wiki. I just added
> http://wiki.qemu.org/Contribute/StartHere#Maintainers
> and improved my own wiki page ("leading by example :-)").
>
> Thanks + regards
>
> Stefan W.
>
>

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Anthony Liguori]

On 03/12/2012 03:24 PM, Peter Maydell wrote:
> On 12 March 2012 20:12, Stefan Weil<sw@weilnetz.de>  wrote:
>> I agree that more maintainers would be good, but we also need
>> more people with commit rights. Why? There a many examples of
>> urgent patches (= patches which fix broken builds) which take
>> several days even when they were reviewed before they finally
>> are committed.
>
> I agree that that's a specific area it would be nice to do
> better in. It seems to me that the qemu-trivial process for
> sweeping up trivial patches has been working well; maybe we
> could use a slightly more formal qemu-urgent process for
> flagging up build breakage etc?
>
> (Personally I'd support a rule that any outstanding
> build-breakage fixes must always go in before anything else.)

When are build-breakage fixes not trivial?

>> Only two maintainers are allowed to make full use of the patchwork
>> (http://patchwork.ozlabs.org/project/qemu-devel/) infrastructure.
>> Why not all maintainers?
>
> I tried to get myself added to the maintainers list (with
> agreement from Anthony) for that a long time ago and got zero
> response from the people running that patchwork instance.

I don't have any control over patchwork.  I believe Michael Tsirkin is the one 
that initially set it up.

Regards,

Anthony Liguori

>
> A good patchwork instance is really useful -- the Linaro one
> is set up with hooks into monitoring git, so patches that are
> committed move automatically to 'accepted', for instance.
> [It's only semi-automatic, though, and I don't know if it
> would still be as useful with the much higher volume qemu gets.]
>
> -- PMM

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Michael S. Tsirkin]

On Mon, Mar 12, 2012 at 08:24:47PM +0000, Peter Maydell wrote:
> > Only two maintainers are allowed to make full use of the patchwork
> > (http://patchwork.ozlabs.org/project/qemu-devel/) infrastructure.
> > Why not all maintainers?
> 
> I tried to get myself added to the maintainers list (with
> agreement from Anthony) for that a long time ago and got zero
> response from the people running that patchwork instance.
> 
> A good patchwork instance is really useful -- the Linaro one
> is set up with hooks into monitoring git, so patches that are
> committed move automatically to 'accepted', for instance.
> [It's only semi-automatic, though, and I don't know if it
> would still be as useful with the much higher volume qemu gets.]
> 
> -- PMM

I think most things do not need admin rights on the list -
just an account is enough.  For example I can change patch
status for netdev patches, and I do this sometimes to
my own patches to avoid confusion (tag them N/A
for review so Dave does not need to waste time looking at them).

That said I send a request to add you as an admin and
we'll see how it goes.

-- 
MST

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Peter Maydell]

On 12 March 2012 20:29, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 03/12/2012 03:24 PM, Peter Maydell wrote:
>> I agree that that's a specific area it would be nice to do
>> better in. It seems to me that the qemu-trivial process for
>> sweeping up trivial patches has been working well; maybe we
>> could use a slightly more formal qemu-urgent process for
>> flagging up build breakage etc?
>>
>> (Personally I'd support a rule that any outstanding
>> build-breakage fixes must always go in before anything else.)
>
>
> When are build-breakage fixes not trivial?

'trivial' implies "it's OK if this patch doesn't go in for a
week or two until the trivial patch queue has built up to
a reasonable size". Also sending them via trivial means
there's no mechanism for causing them to be applied before
other commits/pullreqs. So generally I don't cc build-fixes to
trivial.

-- PMM

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Anthony Liguori]

On 03/12/2012 03:43 PM, Peter Maydell wrote:
> On 12 March 2012 20:29, Anthony Liguori<anthony@codemonkey.ws>  wrote:
>> On 03/12/2012 03:24 PM, Peter Maydell wrote:
>>> I agree that that's a specific area it would be nice to do
>>> better in. It seems to me that the qemu-trivial process for
>>> sweeping up trivial patches has been working well; maybe we
>>> could use a slightly more formal qemu-urgent process for
>>> flagging up build breakage etc?
>>>
>>> (Personally I'd support a rule that any outstanding
>>> build-breakage fixes must always go in before anything else.)
>>
>>
>> When are build-breakage fixes not trivial?
>
> 'trivial' implies "it's OK if this patch doesn't go in for a
> week or two until the trivial patch queue has built up to
> a reasonable size". Also sending them via trivial means
> there's no mechanism for causing them to be applied before
> other commits/pullreqs. So generally I don't cc build-fixes to
> trivial.

In all fairness, the last build breakage I see was specific to win32, was 
reported on Mar 1st, and a patch was committed on Mar 3rd.

I don't think it's reasonable to expect more than this for a breakage on win32.

Regards,

Anthony Liguori

>
> -- PMM

Re: [Qemu-devel] We need more reviewers/maintainers!!

[malc]

On Mon, 12 Mar 2012, Anthony Liguori wrote:

> On 03/12/2012 03:43 PM, Peter Maydell wrote:
> > On 12 March 2012 20:29, Anthony Liguori<anthony@codemonkey.ws>  wrote:
> > > On 03/12/2012 03:24 PM, Peter Maydell wrote:
> > > > I agree that that's a specific area it would be nice to do
> > > > better in. It seems to me that the qemu-trivial process for
> > > > sweeping up trivial patches has been working well; maybe we
> > > > could use a slightly more formal qemu-urgent process for
> > > > flagging up build breakage etc?
> > > > 
> > > > (Personally I'd support a rule that any outstanding
> > > > build-breakage fixes must always go in before anything else.)
> > > 
> > > 
> > > When are build-breakage fixes not trivial?
> > 
> > 'trivial' implies "it's OK if this patch doesn't go in for a
> > week or two until the trivial patch queue has built up to
> > a reasonable size". Also sending them via trivial means
> > there's no mechanism for causing them to be applied before
> > other commits/pullreqs. So generally I don't cc build-fixes to
> > trivial.
> 
> In all fairness, the last build breakage I see was specific to win32, was
> reported on Mar 1st, and a patch was committed on Mar 3rd.
> 
> I don't think it's reasonable to expect more than this for a breakage on
> win32.

Why?


-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Stefan Weil]

Am 12.03.2012 21:27, schrieb Anthony Liguori:
> On 03/12/2012 03:12 PM, Stefan Weil wrote:
>> Am 12.03.2012 18:06, schrieb Stefano Stabellini:
>>> Hi all,
>>> I don't mean to steer any controversy or start any flame wars here, but
>>> rather I want to point out a problem in the QEMU Community that is
>>> preventing us and other people from having a good experience working
>>> upstream with QEMU. Call it constructive criticism.
>>>
>>> Patches are being posted to the list that don't get any reviews at all.
>>> Other patches get reviewed the first time, then once they are reposted
>>> they don't get any other reviews or acked-by or reviewed-by.
>>>
>>> As a whole it takes biblical times to get through the QEMU review
>>> process. I wonder how any commercial company with deadlines would be 
>>> able
>>> to cope with them. Even the Xen Community, that is far from a 
>>> commercial
>>> company, is having difficulties with them and now upstream QEMU is at
>>> risk of missing the 4.2 release target.
>>>
>>>
>>> We need more people reviewing patches. And we need more maintainers.
>>>
>>>
>>> Anthony Liguori is still the maintainer for many areas within QEMU, and
>>> he is clearly too busy for that. We need more people helping him review
>>> patches for source files like savevm.c and vl.c.
>>>
>>> I believe in leading by example, so Anthony Perard and I will try to
>>> review more patch series, even outside Xen support in QEMU, starting
>>> from now.
>>> I hope more people will start to do the same to the point that it will
>>> get natural to add more names and email addresses to the MAINTAINERS
>>> file.
>>>
>>> I hope that other people will recognize that this is a problem and be
>>> willing to step up to find a solution.
>>>
>>> Thanks,
>>>
>>> Stefano
>>
>> I agree that more maintainers would be good, but we also need
>> more people with commit rights.
>
> I disagree strongly.  Having multiple pushers makes things difficult 
> and encourages people to push without testing.  Part of what makes 
> pushing take longer than it should today is that my test cycle takes 
> at least 1-2 hours and it's not uncommon to have to go through 3-4 
> cycles of rebasing before being able to push.
>
>> Why? There a many examples of
>> urgent patches (= patches which fix broken builds) which take
>> several days even when they were reviewed before they finally
>> are committed.
>
> Can you be specific?  I think you really mean, "urgent patches for 
> Win32" but since you're the win32 maintainer, it's on you to do a PULL 
> request.

Please use w32 (win32 is a registered trademark of Microsoft,
and there are also other reasons why I try to avoid it).

I don't mean w32 patches only. The last build break was for ppc hosts
(e04b28996110bd6acfc059e9f2c8c5aba5119a46, it took more than 5 days),
but I remember also situations were x86 was broken more than
a day.

A selection of older patches which fixed build and the time it took
from creation to commit:

6148b23d69444a300710db0c53f6c53b7f3c8067 (kvm ppcm 3 days)
1ecf47bf0a091700e45f1b7d1f5ad85abc0acd22 (w32, 2 days)
aea317aaa5d92ee8789f976ccf105be67d956f5e (0 days, patch written by Anthony)
be85c90b74f56dca51782fa3080fcdf88593e045 (1 day)
3439eec34f8c0ded2ff08da08b058804382a3736 (0 days)
3a26360d1df3c3519a45636ec2189429d3df0ecb (0 days, patch written by Anthony)
98efaf75282a96ffbe2914f79a9f5cb736a03db4 (ppc, 7 days)
d20423788e3a3d5f6a2aad8315779bf3f952ca36 (3 days)
8e72506e20d9e606783de1cdb8d60dd9b9241e30 (0 days)
f44cc4852a04c0423780e115b935e201aaf3384e (3 days)

Cheers,

Stefan W.

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Anthony Liguori]

On 03/12/2012 04:09 PM, malc wrote:
> On Mon, 12 Mar 2012, Anthony Liguori wrote:
>
>> On 03/12/2012 03:43 PM, Peter Maydell wrote:
>>> On 12 March 2012 20:29, Anthony Liguori<anthony@codemonkey.ws>   wrote:
>>>> On 03/12/2012 03:24 PM, Peter Maydell wrote:
>>>>> I agree that that's a specific area it would be nice to do
>>>>> better in. It seems to me that the qemu-trivial process for
>>>>> sweeping up trivial patches has been working well; maybe we
>>>>> could use a slightly more formal qemu-urgent process for
>>>>> flagging up build breakage etc?
>>>>>
>>>>> (Personally I'd support a rule that any outstanding
>>>>> build-breakage fixes must always go in before anything else.)
>>>>
>>>>
>>>> When are build-breakage fixes not trivial?
>>>
>>> 'trivial' implies "it's OK if this patch doesn't go in for a
>>> week or two until the trivial patch queue has built up to
>>> a reasonable size". Also sending them via trivial means
>>> there's no mechanism for causing them to be applied before
>>> other commits/pullreqs. So generally I don't cc build-fixes to
>>> trivial.
>>
>> In all fairness, the last build breakage I see was specific to win32, was
>> reported on Mar 1st, and a patch was committed on Mar 3rd.
>>
>> I don't think it's reasonable to expect more than this for a breakage on
>> win32.
>
> Why?

Patch came on a Thursday and was applied on a Saturday.  That's pretty much one 
business day.

For a problem that affects very few people (and hence has very few people 
complaining), it seems like a reasonable response time.

Regards,

Anthony Liguori

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Peter Maydell]

On 12 March 2012 21:06, Anthony Liguori <anthony@codemonkey.ws> wrote:
> In all fairness, the last build breakage I see was specific to win32, was
> reported on Mar 1st, and a patch was committed on Mar 3rd.

Commit 02021812452 (patch reported March 2nd, applied March 9th)
is more recent than that.

I think a couple of days is fine, a week (with a pile of other
stuff committed beforehand) is less than ideal.

-- PMM

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Anthony Liguori]

On 03/12/2012 04:12 PM, Stefan Weil wrote:
> Am 12.03.2012 21:27, schrieb Anthony Liguori:
>> On 03/12/2012 03:12 PM, Stefan Weil wrote:
>>> Am 12.03.2012 18:06, schrieb Stefano Stabellini:
>>>> Hi all,
>>>> I don't mean to steer any controversy or start any flame wars here, but
>>>> rather I want to point out a problem in the QEMU Community that is
>>>> preventing us and other people from having a good experience working
>>>> upstream with QEMU. Call it constructive criticism.
>>>>
>>>> Patches are being posted to the list that don't get any reviews at all.
>>>> Other patches get reviewed the first time, then once they are reposted
>>>> they don't get any other reviews or acked-by or reviewed-by.
>>>>
>>>> As a whole it takes biblical times to get through the QEMU review
>>>> process. I wonder how any commercial company with deadlines would be able
>>>> to cope with them. Even the Xen Community, that is far from a commercial
>>>> company, is having difficulties with them and now upstream QEMU is at
>>>> risk of missing the 4.2 release target.
>>>>
>>>>
>>>> We need more people reviewing patches. And we need more maintainers.
>>>>
>>>>
>>>> Anthony Liguori is still the maintainer for many areas within QEMU, and
>>>> he is clearly too busy for that. We need more people helping him review
>>>> patches for source files like savevm.c and vl.c.
>>>>
>>>> I believe in leading by example, so Anthony Perard and I will try to
>>>> review more patch series, even outside Xen support in QEMU, starting
>>>> from now.
>>>> I hope more people will start to do the same to the point that it will
>>>> get natural to add more names and email addresses to the MAINTAINERS
>>>> file.
>>>>
>>>> I hope that other people will recognize that this is a problem and be
>>>> willing to step up to find a solution.
>>>>
>>>> Thanks,
>>>>
>>>> Stefano
>>>
>>> I agree that more maintainers would be good, but we also need
>>> more people with commit rights.
>>
>> I disagree strongly. Having multiple pushers makes things difficult and
>> encourages people to push without testing. Part of what makes pushing take
>> longer than it should today is that my test cycle takes at least 1-2 hours and
>> it's not uncommon to have to go through 3-4 cycles of rebasing before being
>> able to push.
>>
>>> Why? There a many examples of
>>> urgent patches (= patches which fix broken builds) which take
>>> several days even when they were reviewed before they finally
>>> are committed.
>>
>> Can you be specific? I think you really mean, "urgent patches for Win32" but
>> since you're the win32 maintainer, it's on you to do a PULL request.
>
> Please use w32 (win32 is a registered trademark of Microsoft,
> and there are also other reasons why I try to avoid it).
>
> I don't mean w32 patches only. The last build break was for ppc hosts
> (e04b28996110bd6acfc059e9f2c8c5aba5119a46, it took more than 5 days),
> but I remember also situations were x86 was broken more than
> a day.
>
> A selection of older patches which fixed build and the time it took
> from creation to commit:
>
> 6148b23d69444a300710db0c53f6c53b7f3c8067 (kvm ppcm 3 days)
> 1ecf47bf0a091700e45f1b7d1f5ad85abc0acd22 (w32, 2 days)

So both of these platforms have maintainers that can do PULL requests for issues 
like this.

> aea317aaa5d92ee8789f976ccf105be67d956f5e (0 days, patch written by Anthony)
> be85c90b74f56dca51782fa3080fcdf88593e045 (1 day)
> 3439eec34f8c0ded2ff08da08b058804382a3736 (0 days)
> 3a26360d1df3c3519a45636ec2189429d3df0ecb (0 days, patch written by Anthony)
> 98efaf75282a96ffbe2914f79a9f5cb736a03db4 (ppc, 7 days)

I cannot easily test patches for !x86 host so it really needs to come through a 
pull request.

> d20423788e3a3d5f6a2aad8315779bf3f952ca36 (3 days)

We've had some issues with hw/9pfs but I think we've got this covered now.

> 8e72506e20d9e606783de1cdb8d60dd9b9241e30 (0 days)
> f44cc4852a04c0423780e115b935e201aaf3384e (3 days)

Without searching the list, I think this was a BSD specific build failure.

Not ideal, but I'm pretty happy with this numbers.

Regards,

Anthony Liguori

>
> Cheers,
>
> Stefan W.
>

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Anthony Liguori]

On 03/12/2012 04:16 PM, Peter Maydell wrote:
> On 12 March 2012 21:06, Anthony Liguori<anthony@codemonkey.ws>  wrote:
>> In all fairness, the last build breakage I see was specific to win32, was
>> reported on Mar 1st, and a patch was committed on Mar 3rd.
>
> Commit 02021812452 (patch reported March 2nd, applied March 9th)
> is more recent than that.

I'll take full blame here.  I had thought I applied it and hadn't.  But I think 
this is the exception that proves the rule.

Regards,

Anthony Liguori

>
> I think a couple of days is fine, a week (with a pile of other
> stuff committed beforehand) is less than ideal.
>
> -- PMM

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Stefan Weil]

Am 12.03.2012 21:27, schrieb Anthony Liguori:
> On 03/12/2012 03:12 PM, Stefan Weil wrote:
>
>> We also need more resources for technical maintenance of the
>> QEMU infrastructure. For example, the official mirror of the
>> QEMU git repository (https://github.com/qemu/QEMU) is several
>> months behind, http://git.savannah.gnu.org/cgit/qemu.git is
>> even older.
>
> Savannah is a dead tree.  I should just remove it.

Is this your personal opinion or was it widely discussed here?

Removing data which never gets updates is of course clearly better
than pretending that it is up to date, but maybe there are more
options (see below).


>
> Re: github, this hasn't seemed that urgent to me.
>
>> It should be possible to have three mirrors which
>> are nearly up to date (like http://repo.or.cz/w/qemu.git/).


There are still a lot of links pointing to QEMU on Savannah,
that's why I'd neither remove the git repo there nor the
project page.

Maintaining a web page at Savannah which redirects to qemu.org
and having a git mirror which automatically synchronizes
with the official git repository would be a good idea
(this is my personal opinion). I offer to setup and
maintain it, if I get the necessary rights.

Regards,

Stefan Weil

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Stefan Weil]

Am 12.03.2012 22:13, schrieb Anthony Liguori:
> On 03/12/2012 04:09 PM, malc wrote:
>> On Mon, 12 Mar 2012, Anthony Liguori wrote:
>>
>>> On 03/12/2012 03:43 PM, Peter Maydell wrote:
>>>> On 12 March 2012 20:29, Anthony Liguori<anthony@codemonkey.ws>   
>>>> wrote:
>>>>> On 03/12/2012 03:24 PM, Peter Maydell wrote:
>>>>>> I agree that that's a specific area it would be nice to do
>>>>>> better in. It seems to me that the qemu-trivial process for
>>>>>> sweeping up trivial patches has been working well; maybe we
>>>>>> could use a slightly more formal qemu-urgent process for
>>>>>> flagging up build breakage etc?
>>>>>>
>>>>>> (Personally I'd support a rule that any outstanding
>>>>>> build-breakage fixes must always go in before anything else.)
>>>>>
>>>>>
>>>>> When are build-breakage fixes not trivial?
>>>>
>>>> 'trivial' implies "it's OK if this patch doesn't go in for a
>>>> week or two until the trivial patch queue has built up to
>>>> a reasonable size". Also sending them via trivial means
>>>> there's no mechanism for causing them to be applied before
>>>> other commits/pullreqs. So generally I don't cc build-fixes to
>>>> trivial.
>>>
>>> In all fairness, the last build breakage I see was specific to 
>>> win32, was
>>> reported on Mar 1st, and a patch was committed on Mar 3rd.
>>>
>>> I don't think it's reasonable to expect more than this for a 
>>> breakage on
>>> win32.
>>
>> Why?
>
> Patch came on a Thursday and was applied on a Saturday.  That's pretty 
> much one business day.
>
> For a problem that affects very few people (and hence has very few 
> people complaining), it seems like a reasonable response time.

Do you have numbers? As far as I know, more people are using Windows 
than Linux.

Ok, there are more QEMU developers which work on Linux than on Windows,
but that's no reason why w32 build fixes are less important.

Many Linux developers will simply fix a broken build in their local tree.
Windows developers expect that everything works out of the box,
without manually changing the source code.

=> All patches which fix broken build are equal.

> Regards,
>
> Anthony Liguori

Re: [Qemu-devel] We need more reviewers/maintainers!!

[malc]

On Mon, 12 Mar 2012, Anthony Liguori wrote:

> On 03/12/2012 04:09 PM, malc wrote:
> > On Mon, 12 Mar 2012, Anthony Liguori wrote:
> > 
> > > On 03/12/2012 03:43 PM, Peter Maydell wrote:
> > > > On 12 March 2012 20:29, Anthony Liguori<anthony@codemonkey.ws>   wrote:
> > > > > On 03/12/2012 03:24 PM, Peter Maydell wrote:
> > > > > > I agree that that's a specific area it would be nice to do
> > > > > > better in. It seems to me that the qemu-trivial process for
> > > > > > sweeping up trivial patches has been working well; maybe we
> > > > > > could use a slightly more formal qemu-urgent process for
> > > > > > flagging up build breakage etc?
> > > > > > 
> > > > > > (Personally I'd support a rule that any outstanding
> > > > > > build-breakage fixes must always go in before anything else.)
> > > > > 
> > > > > 
> > > > > When are build-breakage fixes not trivial?
> > > > 
> > > > 'trivial' implies "it's OK if this patch doesn't go in for a
> > > > week or two until the trivial patch queue has built up to
> > > > a reasonable size". Also sending them via trivial means
> > > > there's no mechanism for causing them to be applied before
> > > > other commits/pullreqs. So generally I don't cc build-fixes to
> > > > trivial.
> > > 
> > > In all fairness, the last build breakage I see was specific to win32, was
> > > reported on Mar 1st, and a patch was committed on Mar 3rd.
> > > 
> > > I don't think it's reasonable to expect more than this for a breakage on
> > > win32.
> > 
> > Why?
> 
> Patch came on a Thursday and was applied on a Saturday.  That's pretty much
> one business day.
> 
> For a problem that affects very few people (and hence has very few people
> complaining), it seems like a reasonable response time.

And you get "very few" statistics exactly from?

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Anthony Liguori]

On 03/12/2012 04:43 PM, malc wrote:
> On Mon, 12 Mar 2012, Anthony Liguori wrote:
>> Patch came on a Thursday and was applied on a Saturday.  That's pretty much
>> one business day.
>>
>> For a problem that affects very few people (and hence has very few people
>> complaining), it seems like a reasonable response time.
>
> And you get "very few" statistics exactly from?

Number of people who complain.

Regards,

Anthony Liguori

>

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Anthony Liguori]

On 03/12/2012 04:41 PM, Stefan Weil wrote:
> Am 12.03.2012 22:13, schrieb Anthony Liguori:
>> On 03/12/2012 04:09 PM, malc wrote:
>>> On Mon, 12 Mar 2012, Anthony Liguori wrote:
>>>
>>>> On 03/12/2012 03:43 PM, Peter Maydell wrote:
>>>>> On 12 March 2012 20:29, Anthony Liguori<anthony@codemonkey.ws> wrote:
>>>>>> On 03/12/2012 03:24 PM, Peter Maydell wrote:
>>>>>>> I agree that that's a specific area it would be nice to do
>>>>>>> better in. It seems to me that the qemu-trivial process for
>>>>>>> sweeping up trivial patches has been working well; maybe we
>>>>>>> could use a slightly more formal qemu-urgent process for
>>>>>>> flagging up build breakage etc?
>>>>>>>
>>>>>>> (Personally I'd support a rule that any outstanding
>>>>>>> build-breakage fixes must always go in before anything else.)
>>>>>>
>>>>>>
>>>>>> When are build-breakage fixes not trivial?
>>>>>
>>>>> 'trivial' implies "it's OK if this patch doesn't go in for a
>>>>> week or two until the trivial patch queue has built up to
>>>>> a reasonable size". Also sending them via trivial means
>>>>> there's no mechanism for causing them to be applied before
>>>>> other commits/pullreqs. So generally I don't cc build-fixes to
>>>>> trivial.
>>>>
>>>> In all fairness, the last build breakage I see was specific to win32, was
>>>> reported on Mar 1st, and a patch was committed on Mar 3rd.
>>>>
>>>> I don't think it's reasonable to expect more than this for a breakage on
>>>> win32.
>>>
>>> Why?
>>
>> Patch came on a Thursday and was applied on a Saturday. That's pretty much one
>> business day.
>>
>> For a problem that affects very few people (and hence has very few people
>> complaining), it seems like a reasonable response time.
>
> Do you have numbers? As far as I know, more people are using Windows than Linux.
>
> Ok, there are more QEMU developers which work on Linux than on Windows,
> but that's no reason why w32 build fixes are less important.

It's not that someone is seeing a w32 build fix and saying, oh, this is less 
important, I'm going to ignore it.  Believe it or not, it may take 1-2 days just 
to notice the patch.  qemu-devel gets an awful lot of traffic these days.

> Many Linux developers will simply fix a broken build in their local tree.
> Windows developers expect that everything works out of the box,
> without manually changing the source code.
>
> => All patches which fix broken build are equal.

Why is a broken build worse than a bug that affects functionality?

Regards,

Anthony Liguori

>> Regards,
>>
>> Anthony Liguori
>
>

Re: [Qemu-devel] We need more reviewers/maintainers!!

[malc]

On Mon, 12 Mar 2012, Anthony Liguori wrote:

> On 03/12/2012 04:43 PM, malc wrote:
> > On Mon, 12 Mar 2012, Anthony Liguori wrote:
> > > Patch came on a Thursday and was applied on a Saturday.  That's pretty
> > > much
> > > one business day.
> > > 
> > > For a problem that affects very few people (and hence has very few people
> > > complaining), it seems like a reasonable response time.
> > 
> > And you get "very few" statistics exactly from?
> 
> Number of people who complain.


http://www.google.com/search?q=linux+sucks
About 13,800,000 results

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Andreas Färber]

Am 12.03.2012 22:18, schrieb Anthony Liguori:
> On 03/12/2012 04:12 PM, Stefan Weil wrote:
>> Am 12.03.2012 21:27, schrieb Anthony Liguori:
>>> On 03/12/2012 03:12 PM, Stefan Weil wrote:
>>>> [...] There a many examples of
>>>> urgent patches (= patches which fix broken builds) which take
>>>> several days even when they were reviewed before they finally
>>>> are committed.
>>>
>>> Can you be specific? [...]
>>
>> I don't mean w32 patches only. The last build break was for ppc hosts
>> (e04b28996110bd6acfc059e9f2c8c5aba5119a46, it took more than 5 days),
>> but I remember also situations were x86 was broken more than
>> a day.
>>
>> A selection of older patches which fixed build and the time it took
>> from creation to commit:
>>
>> 6148b23d69444a300710db0c53f6c53b7f3c8067 (kvm ppcm 3 days)
>> 1ecf47bf0a091700e45f1b7d1f5ad85abc0acd22 (w32, 2 days)
> 
> So both of these platforms have maintainers that can do PULL requests
> for issues like this.
[snip]

You're missing the point here: There usually *are* patches quickly, but
they often don't get applied to qemu.git as fast. Often there are even
two or three people sending build fixes concurrently. Do you really need
a PULL for a single patch that affects everyone?

Take Blue's recent target-ppc fix
9d4df9c02866f39d3eef105033091f367cc7c29e for example: After applying
patches on day one of FOSDEM he posted a -Werror fix, it got confirmed
by me and Alex but wasn't applied until a week later, because apparently
no other committer dared to apply Blue's patch despite SoB and acks and
people reporting the issue... Not happy.
No doubt Alex is to blame for not catching that issue in his ppc queue,
but asking Alex as submaintainer to submit a PULL for a single patch
posted by Blue as committer seems overly complicated to me! ;)

If the build gets broken (which I bet will happen even with qemu-test
once in a while, given we all use different compilers) then fixing it
should not be a submaintainer responsibility IMO. If non-obvious how to
fix, then as a maintainer please just revert offending patches and let
the submaintainers fix their PULLs rather than delegating responsibility
for the whole qemu.git build to the submaintainer of the day.

This boils down to the policies question I brought up some weeks ago:
When we have, e.g., a build breakage on the major platform, do
maintainers expect a certain number of reviewers before applying? Or a
fixed timeframe for review? Again, please document these things in the
Wiki so people know what to do and don't just wait for another to act!

Regards,
Andreas

P.S. I know I'm lagging myself, especially for the cocoa queue lately,
but I'm one and in theory there's like seven committers for qemu.git...

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Anthony Liguori]

On 03/12/2012 06:32 PM, Andreas Färber wrote:
> Am 12.03.2012 22:18, schrieb Anthony Liguori:
>> On 03/12/2012 04:12 PM, Stefan Weil wrote:
>>> Am 12.03.2012 21:27, schrieb Anthony Liguori:
>>>> On 03/12/2012 03:12 PM, Stefan Weil wrote:
>>>>> [...] There a many examples of
>>>>> urgent patches (= patches which fix broken builds) which take
>>>>> several days even when they were reviewed before they finally
>>>>> are committed.
>>>>
>>>> Can you be specific? [...]
>>>
>>> I don't mean w32 patches only. The last build break was for ppc hosts
>>> (e04b28996110bd6acfc059e9f2c8c5aba5119a46, it took more than 5 days),
>>> but I remember also situations were x86 was broken more than
>>> a day.
>>>
>>> A selection of older patches which fixed build and the time it took
>>> from creation to commit:
>>>
>>> 6148b23d69444a300710db0c53f6c53b7f3c8067 (kvm ppcm 3 days)
>>> 1ecf47bf0a091700e45f1b7d1f5ad85abc0acd22 (w32, 2 days)
>>
>> So both of these platforms have maintainers that can do PULL requests
>> for issues like this.
> [snip]
>
> You're missing the point here: There usually *are* patches quickly, but
> they often don't get applied to qemu.git as fast. Often there are even
> two or three people sending build fixes concurrently. Do you really need
> a PULL for a single patch that affects everyone?

A PULL is just as good as a push.  There's a slight lag in a PULL, but generally 
it should be pretty quick.

>
> Take Blue's recent target-ppc fix
> 9d4df9c02866f39d3eef105033091f367cc7c29e for example: After applying
> patches on day one of FOSDEM he posted a -Werror fix, it got confirmed
> by me and Alex but wasn't applied until a week later, because apparently
> no other committer dared to apply Blue's patch despite SoB and acks and
> people reporting the issue... Not happy.
> No doubt Alex is to blame for not catching that issue in his ppc queue,
> but asking Alex as submaintainer to submit a PULL for a single patch
> posted by Blue as committer seems overly complicated to me! ;)

I think this is a good demonstration of what the problem is.  Unclear 
responsibility.  I'm pretty sure that Blue thought that Alex would handle the 
patch.  I'm pretty sure that Alex thought Blue would handle the patch.

If we start saying that, Alex "owns" ppc except for things that are "important" 
like a build breakage, then we get into the ugly definition of what's important 
and what's not important.

So yes, if the build breakage is limited to PPC, and came in through the PPC 
tree, I'd expect the fix to also come through the PPC tree.

Regards,

Anthony Liguori

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Alexander Graf]

Am 13.03.2012 um 01:16 schrieb Anthony Liguori <anthony@codemonkey.ws>:

> On 03/12/2012 06:32 PM, Andreas Färber wrote:
>> Am 12.03.2012 22:18, schrieb Anthony Liguori:
>>> On 03/12/2012 04:12 PM, Stefan Weil wrote:
>>>> Am 12.03.2012 21:27, schrieb Anthony Liguori:
>>>>> On 03/12/2012 03:12 PM, Stefan Weil wrote:
>>>>>> [...] There a many examples of
>>>>>> urgent patches (= patches which fix broken builds) which take
>>>>>> several days even when they were reviewed before they finally
>>>>>> are committed.
>>>>> 
>>>>> Can you be specific? [...]
>>>> 
>>>> I don't mean w32 patches only. The last build break was for ppc hosts
>>>> (e04b28996110bd6acfc059e9f2c8c5aba5119a46, it took more than 5 days),
>>>> but I remember also situations were x86 was broken more than
>>>> a day.
>>>> 
>>>> A selection of older patches which fixed build and the time it took
>>>> from creation to commit:
>>>> 
>>>> 6148b23d69444a300710db0c53f6c53b7f3c8067 (kvm ppcm 3 days)
>>>> 1ecf47bf0a091700e45f1b7d1f5ad85abc0acd22 (w32, 2 days)
>>> 
>>> So both of these platforms have maintainers that can do PULL requests
>>> for issues like this.
>> [snip]
>> 
>> You're missing the point here: There usually *are* patches quickly, but
>> they often don't get applied to qemu.git as fast. Often there are even
>> two or three people sending build fixes concurrently. Do you really need
>> a PULL for a single patch that affects everyone?
> 
> A PULL is just as good as a push.  There's a slight lag in a PULL, but generally it should be pretty quick.
> 
>> 
>> Take Blue's recent target-ppc fix
>> 9d4df9c02866f39d3eef105033091f367cc7c29e for example: After applying
>> patches on day one of FOSDEM he posted a -Werror fix, it got confirmed
>> by me and Alex but wasn't applied until a week later, because apparently
>> no other committer dared to apply Blue's patch despite SoB and acks and
>> people reporting the issue... Not happy.
>> No doubt Alex is to blame for not catching that issue in his ppc queue,
>> but asking Alex as submaintainer to submit a PULL for a single patch
>> posted by Blue as committer seems overly complicated to me! ;)
> 
> I think this is a good demonstration of what the problem is.  Unclear responsibility.  I'm pretty sure that Blue thought that Alex would handle the patch.  I'm pretty sure that Alex thought Blue would handle the patch.
> 
> If we start saying that, Alex "owns" ppc except for things that are "important" like a build breakage, then we get into the ugly definition of what's important and what's not important.
> 
> So yes, if the build breakage is limited to PPC, and came in through the PPC tree, I'd expect the fix to also come through the PPC tree.

Yeah, the ppc tree issue is probably special to my workflow. I tend to only keep a single branch - ppc-next. That branch is basically my staging and development tree which I synchronize using pull requests every now and then.

The problem with that is that single build breakage fixes don't fit in at all in that workflow - and I haven't found a good alternative yet. Jumping through different trees just increases chances I lose track of patches even more, so splitting ppc-next and ppc-something-i-commit-now doesn't work for me.

And yes, pull requests do take longer. They are also more visible than pushes. But that also means that the barrier for doing one is bigger - who really wants to spam the ML? Sending a pull request is also a matter of minutes, while a push is a matter of seconds. Smoke tsting of that tree alone is a matter of running autotest for half a day. It sounds like no big deal, but I often found myself not pushing because I felt the I don't have enough patches queued up to make a pull request worthwhile, while I do push wholeheartedly to ppc-next, since I assume that tree unstable.

This is all founded in the whole idea behind doing pull requests though, they are supposed to be visible. They are supposed to be scarce. They are supposed to be consistent, well tested states. This is orthogonal to the requirements of quick fixes, which are usually very simple and need to be applied as fast as possible and don't need deep testing because they are very limited in scope.

Either way, next time I'll just mail Blue directly that he should apply a single commit fix and then everyone'll be happy :).


Alex

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Andreas Färber]

Am 13.03.2012 01:16, schrieb Anthony Liguori:
> On 03/12/2012 06:32 PM, Andreas Färber wrote:
>> Take Blue's recent target-ppc fix
>> 9d4df9c02866f39d3eef105033091f367cc7c29e for example: After applying
>> patches on day one of FOSDEM he posted a -Werror fix, it got confirmed
>> by me and Alex but wasn't applied until a week later, because apparently
>> no other committer dared to apply Blue's patch despite SoB and acks and
>> people reporting the issue... Not happy.
>> No doubt Alex is to blame for not catching that issue in his ppc queue,
>> but asking Alex as submaintainer to submit a PULL for a single patch
>> posted by Blue as committer seems overly complicated to me! ;)
> 
> I think this is a good demonstration of what the problem is.  Unclear
> responsibility.

Agreed. Solution is documentation of expected workflows.

>  I'm pretty sure that Blue thought that Alex would
> handle the patch.  I'm pretty sure that Alex thought Blue would handle
> the patch.

Alex actually asked Blue to.

However from what I understand, Blue is not working on QEMU full-time,
like me previously. I assume he handled the patch once he read and came
around to it. It's just that no one else with commit powers reacted to
the situation.

The way I see it no one "owns" code in QEMU. Some people feel
responsible for (or comfortable reviewing changes to) parts of QEMU, and
the project scales by distributing review, testing and queuing to more
such shoulders.
However where a (sub)maintainer is unresponsive - and *there* I
differentiate between build breakages, runtime issues and feature
additions - we can't wait forever and need to adapt processes. Fixing
the build within a reasonable time is one requirement, moving forward
with target-mips at all is another example.

It's not really that we have too few maintainers, it's that not all
maintainers maintain at all times - for valid work or personal reasons -
and we don't seem to have a well-working escalation mechanism beyond
ping^n to handle that.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Alexander Graf]

Am 13.03.2012 um 02:01 schrieb Andreas Färber <afaerber@suse.de>:

> Am 13.03.2012 01:16, schrieb Anthony Liguori:
>> On 03/12/2012 06:32 PM, Andreas Färber wrote:
>>> Take Blue's recent target-ppc fix
>>> 9d4df9c02866f39d3eef105033091f367cc7c29e for example: After applying
>>> patches on day one of FOSDEM he posted a -Werror fix, it got confirmed
>>> by me and Alex but wasn't applied until a week later, because apparently
>>> no other committer dared to apply Blue's patch despite SoB and acks and
>>> people reporting the issue... Not happy.
>>> No doubt Alex is to blame for not catching that issue in his ppc queue,
>>> but asking Alex as submaintainer to submit a PULL for a single patch
>>> posted by Blue as committer seems overly complicated to me! ;)
>> 
>> I think this is a good demonstration of what the problem is.  Unclear
>> responsibility.
> 
> Agreed. Solution is documentation of expected workflows.
> 
>> I'm pretty sure that Blue thought that Alex would
>> handle the patch.  I'm pretty sure that Alex thought Blue would handle
>> the patch.
> 
> Alex actually asked Blue to.
> 
> However from what I understand, Blue is not working on QEMU full-time,
> like me previously. I assume he handled the patch once he read and came
> around to it. It's just that no one else with commit powers reacted to
> the situation.
> 
> The way I see it no one "owns" code in QEMU. Some people feel
> responsible for (or comfortable reviewing changes to) parts of QEMU, and
> the project scales by distributing review, testing and queuing to more
> such shoulders.
> However where a (sub)maintainer is unresponsive - and *there* I
> differentiate between build breakages, runtime issues and feature
> additions - we can't wait forever and need to adapt processes. Fixing
> the build within a reasonable time is one requirement, moving forward
> with target-mips at all is another example.
> 
> It's not really that we have too few maintainers, it's that not all
> maintainers maintain at all times - for valid work or personal reasons -
> and we don't seem to have a well-working escalation mechanism beyond
> ping^n to handle that.

Let's take a real world example from Linux here. 3.3-rc5 had a pretty nasty compile bug that made the build break on any 32 bit target when autofs was activated. I posted the bug plus a small bugfix upstream.

What happened? Linus went ahead and fixed the thing properly so rc6 could go out quickly.

I guess that's what Andreas is trying to say here. Making sure new stuff works, fixing uncritical bugs etc is well within a maintainer's responsibility. Keeping all the code together is where it boils down to the next, the global level, the comitters.

That doesn't mean that any of this is exclusive, but it means that whoever works on the global scale of the project - and that's what commit rights mean - is oblidged to care about the wellbeing of the whole project as a whole. You can't just pick a few subsystems and say "I maintain QEMU but I don't care if SH4 breaks". That'd be hypocritical :). The same way I can't say "This is a patch in PPC code but because this is a particular implementation I don't care about I'll ignore it".

Unfortunately - mainly for historical and political reasons - that subsytem thinking happens a lot in QEMU land these days. And I'm not sure how to change that. Few people actually care about all aspects of QEMU at the same time.


Alex

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Super Bisquit]

Is there a core group/committee that handles all fixes, code, and what not?



On 3/12/12, Alexander Graf <agraf@suse.de> wrote:
>
>
> Am 13.03.2012 um 02:01 schrieb Andreas Färber <afaerber@suse.de>:
>
>> Am 13.03.2012 01:16, schrieb Anthony Liguori:
>>> On 03/12/2012 06:32 PM, Andreas Färber wrote:
>>>> Take Blue's recent target-ppc fix
>>>> 9d4df9c02866f39d3eef105033091f367cc7c29e for example: After applying
>>>> patches on day one of FOSDEM he posted a -Werror fix, it got confirmed
>>>> by me and Alex but wasn't applied until a week later, because apparently
>>>> no other committer dared to apply Blue's patch despite SoB and acks and
>>>> people reporting the issue... Not happy.
>>>> No doubt Alex is to blame for not catching that issue in his ppc queue,
>>>> but asking Alex as submaintainer to submit a PULL for a single patch
>>>> posted by Blue as committer seems overly complicated to me! ;)
>>>
>>> I think this is a good demonstration of what the problem is.  Unclear
>>> responsibility.
>>
>> Agreed. Solution is documentation of expected workflows.
>>
>>> I'm pretty sure that Blue thought that Alex would
>>> handle the patch.  I'm pretty sure that Alex thought Blue would handle
>>> the patch.
>>
>> Alex actually asked Blue to.
>>
>> However from what I understand, Blue is not working on QEMU full-time,
>> like me previously. I assume he handled the patch once he read and came
>> around to it. It's just that no one else with commit powers reacted to
>> the situation.
>>
>> The way I see it no one "owns" code in QEMU. Some people feel
>> responsible for (or comfortable reviewing changes to) parts of QEMU, and
>> the project scales by distributing review, testing and queuing to more
>> such shoulders.
>> However where a (sub)maintainer is unresponsive - and *there* I
>> differentiate between build breakages, runtime issues and feature
>> additions - we can't wait forever and need to adapt processes. Fixing
>> the build within a reasonable time is one requirement, moving forward
>> with target-mips at all is another example.
>>
>> It's not really that we have too few maintainers, it's that not all
>> maintainers maintain at all times - for valid work or personal reasons -
>> and we don't seem to have a well-working escalation mechanism beyond
>> ping^n to handle that.
>
> Let's take a real world example from Linux here. 3.3-rc5 had a pretty nasty
> compile bug that made the build break on any 32 bit target when autofs was
> activated. I posted the bug plus a small bugfix upstream.
>
> What happened? Linus went ahead and fixed the thing properly so rc6 could go
> out quickly.
>
> I guess that's what Andreas is trying to say here. Making sure new stuff
> works, fixing uncritical bugs etc is well within a maintainer's
> responsibility. Keeping all the code together is where it boils down to the
> next, the global level, the comitters.
>
> That doesn't mean that any of this is exclusive, but it means that whoever
> works on the global scale of the project - and that's what commit rights
> mean - is oblidged to care about the wellbeing of the whole project as a
> whole. You can't just pick a few subsystems and say "I maintain QEMU but I
> don't care if SH4 breaks". That'd be hypocritical :). The same way I can't
> say "This is a patch in PPC code but because this is a particular
> implementation I don't care about I'll ignore it".
>
> Unfortunately - mainly for historical and political reasons - that subsytem
> thinking happens a lot in QEMU land these days. And I'm not sure how to
> change that. Few people actually care about all aspects of QEMU at the same
> time.
>
>
> Alex
>
>
>

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Anthony Liguori]

On 03/12/2012 08:23 PM, Alexander Graf wrote:
>
>
> Let's take a real world example from Linux here. 3.3-rc5 had a pretty nasty compile bug that made the build break on any 32 bit target when autofs was activated. I posted the bug plus a small bugfix upstream.

We have a different model than Linux.  The development tree is only open for a 
short period of time followed by RCs.

Our development tree is open for a long time.  There isn't as great a sense of 
urgency to fix things out-of-band when you're talking about an active 
development cycle.  If we were in -rc, it'd be a completely different discussion.

Really, the issue here is communication.  The default assumption is that a 
subsystem is owned by the subsystem maintainer.  If you want a patch to go 
through another channel, just say so.  It happens constantly.  It's very normal 
for people to ask me to apply a patch directly instead of doing a PULL request.

But honestly, if we made a habit of cherry picking patches in areas with an 
active maintainer, we would now be talking about how we don't respect subsystems.

Regards,

Anthony Liguori

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Alexander Graf]

Am 13.03.2012 um 02:39 schrieb Anthony Liguori <anthony@codemonkey.ws>:

> On 03/12/2012 08:23 PM, Alexander Graf wrote:
>> 
>> 
>> Let's take a real world example from Linux here. 3.3-rc5 had a pretty nasty compile bug that made the build break on any 32 bit target when autofs was activated. I posted the bug plus a small bugfix upstream.
> 
> We have a different model than Linux.  The development tree is only open for a short period of time followed by RCs.
> 
> Our development tree is open for a long time.  There isn't as great a sense of urgency to fix things out-of-band when you're talking about an active development cycle.  If we were in -rc, it'd be a completely different discussion.
> 
> Really, the issue here is communication.  The default assumption is that a subsystem is owned by the subsystem maintainer.  If you want a patch to go through another channel, just say so.  It happens constantly.  It's very normal for people to ask me to apply a patch directly instead of doing a PULL request.
> 
> But honestly, if we made a habit of cherry picking patches in areas with an active maintainer, we would now be talking about how we don't respect subsystems.

Yeah, we probably will never get the perfect model that fits everyone. Let's just try, give our best and move on ;).


Alex

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Anthony Liguori]

On 03/12/2012 09:04 PM, Alexander Graf wrote:
>
>
> Am 13.03.2012 um 02:39 schrieb Anthony Liguori<anthony@codemonkey.ws>:
>
>> On 03/12/2012 08:23 PM, Alexander Graf wrote:
>>>
>>>
>>> Let's take a real world example from Linux here. 3.3-rc5 had a pretty nasty compile bug that made the build break on any 32 bit target when autofs was activated. I posted the bug plus a small bugfix upstream.
>>
>> We have a different model than Linux.  The development tree is only open for a short period of time followed by RCs.
>>
>> Our development tree is open for a long time.  There isn't as great a sense of urgency to fix things out-of-band when you're talking about an active development cycle.  If we were in -rc, it'd be a completely different discussion.
>>
>> Really, the issue here is communication.  The default assumption is that a subsystem is owned by the subsystem maintainer.  If you want a patch to go through another channel, just say so.  It happens constantly.  It's very normal for people to ask me to apply a patch directly instead of doing a PULL request.
>>
>> But honestly, if we made a habit of cherry picking patches in areas with an active maintainer, we would now be talking about how we don't respect subsystems.
>
> Yeah, we probably will never get the perfect model that fits everyone. Let's just try, give our best and move on ;).

Agreed!

Regards,

Anthony Liguori

>
>
> Alex
>

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Peter Maydell]

On 13 March 2012 00:16, Anthony Liguori <anthony@codemonkey.ws> wrote:
> I think this is a good demonstration of what the problem is.  Unclear
> responsibility.  I'm pretty sure that Blue thought that Alex would handle
> the patch.  I'm pretty sure that Alex thought Blue would handle the patch.

Yes, this is why I suggested a 'qemu-urgent' or something so it's easy
to flag a patch up as "this is an important fix for something which
should get prompt attention and isn't expected to be going through the
long-winded process of a submaintainer tree or qemu-trivial or whatever".

> If we start saying that, Alex "owns" ppc except for things that are
> "important" like a build breakage, then we get into the ugly definition of
> what's important and what's not important.

I don't think we've had huge problems with defining "trivial" and I
don't think we'd really have big arguments about "urgent" either --
as the gatekeeper you and the other direct-committers can always use
your judgement and say 'this should go through the submaintainer tree'.

I agree completely with Alex about why urgent fixes don't mesh well with
the periodic submaintainer tree pullreq workflow. Dumping the 'urgent'
fix problem off onto submaintainers is basically asking us all to
have an extra 'foo-urgent' tree and send out single patch pullreqs,
which seems to me like a very heavyweight way of causing a patch to
be applied (plus it puts an extra person in the loop which is pretty
much guaranteed to slow things down).

[I'm not sure this is really our most pressing process issue, though.]

-- PMM

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Andreas Färber]

Am 12.03.2012 19:03, schrieb Lluís Vilanova:
> Stefano Stabellini writes:
> [...]
>> Patches are being posted to the list that don't get any reviews at all.
>> Other patches get reviewed the first time, then once they are reposted
>> they don't get any other reviews or acked-by or reviewed-by.
> 
> What are the different tags available and what is their semantic? Maybe this
> should be written down somewhere (HACKING?).
> 
> I'll contribute a starting list of tags
> (http://wiki.x.org/wiki/Development/Documentation/SubmittingPatches), please
> check these are the desired semantics for this project.
> 
> * Cc: Full Name <email>
> 
>   In addition to sending the patches to the mailing list, they must be Cc'ed to,
>   at least, the maintainers for the affected files. You can look up the
>   maintainers for each file in the MAINTAINERS file.
> 
>   The command "git send-email" will automatically look for such a tag in order
>   to send a CC of the mail to the given person.
> 
>   Document maintainer discovery with 'scripts/get_maintainer.pl'?

Add this to your .git/config:

[sendemail]
	cccmd = scripts/get_maintainer.pl --nogit-fallback

The poorly documented argument avoids fallback to git-blame.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Stefan Hajnoczi]

On Mon, Mar 12, 2012 at 8:43 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 March 2012 20:29, Anthony Liguori <anthony@codemonkey.ws> wrote:
>> On 03/12/2012 03:24 PM, Peter Maydell wrote:
>>> I agree that that's a specific area it would be nice to do
>>> better in. It seems to me that the qemu-trivial process for
>>> sweeping up trivial patches has been working well; maybe we
>>> could use a slightly more formal qemu-urgent process for
>>> flagging up build breakage etc?
>>>
>>> (Personally I'd support a rule that any outstanding
>>> build-breakage fixes must always go in before anything else.)
>>
>>
>> When are build-breakage fixes not trivial?
>
> 'trivial' implies "it's OK if this patch doesn't go in for a
> week or two until the trivial patch queue has built up to
> a reasonable size". Also sending them via trivial means
> there's no mechanism for causing them to be applied before
> other commits/pullreqs. So generally I don't cc build-fixes to
> trivial.

Right, I specifically do not take build-breakage fixes because
trivial-patches pull requests don't happen immediately.

If there is a critical problem (i.e. a build breakage), then a
committer needs to take care of it so it can get merged directly.
Let's not make the path longer by going via trivial-patches here.

Stefan

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Stefan Hajnoczi]

On Mon, Mar 12, 2012 at 8:12 PM, Stefan Weil <sw@weilnetz.de> wrote:
> Am 12.03.2012 18:06, schrieb Stefano Stabellini:
> Maybe every maintainer can maintain a short summary of
> what he maintains, how (s)he does it (repository, expected
> response time, ...) in the QEMU wiki. I just added
> http://wiki.qemu.org/Contribute/StartHere#Maintainers
> and improved my own wiki page ("leading by example :-)").

While I understand the sentiment, I don't think duplicating
./MAINTAINERS on the wiki is a good idea.  I suggest linking solely to
the ./MAINTAINERS file from the wiki instead of actually listing
people.

I have not removed your edit because I wanted to see what you think first.

Stefan

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Stefano Stabellini]

On Mon, 12 Mar 2012, Michael Roth wrote:
> Thanks Stefano. I plan on doing a lot of work with migration in the
> future, and as such try to keep tabs on the migration-related stuff on
> qemu-devel. I often don't get around to actually reviewing things
> though, and that's been nagging me for a while now, so this is a good
> point for me to start doing a better job of it. I'll try to review any
> migration stuff I see, and anyone needing a second set of eyes feel free
> to CC me or ping me on IRC.

Great, thanks!

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Kevin Wolf]

Am 12.03.2012 18:34, schrieb Stefano Stabellini:
> On Mon, 12 Mar 2012, Anthony Liguori wrote:
>> On 03/12/2012 12:06 PM, Stefano Stabellini wrote:
>>> Hi all,
>>> I don't mean to steer any controversy or start any flame wars here, but
>>> rather I want to point out a problem in the QEMU Community that is
>>> preventing us and other people from having a good experience working
>>> upstream with QEMU. Call it constructive criticism.
>>>
>>> Patches are being posted to the list that don't get any reviews at all.
>>> Other patches get reviewed the first time, then once they are reposted
>>> they don't get any other reviews or acked-by or reviewed-by.
>>
>> In all fairness, QEMU continues to grow year-to-year both in terms of total 
>> commits and number of contributors.
>>
>> The area that we struggle with is infrequent contributors that contribute 
>> non-trivial things and are write-only contributors.
>>
>> In this case, I really think the problem is expecting to be a write-only 
>> contributor.  Part of participating in a community is not only pushing your own 
>> patches for acceptance but also reviewing other people's patches and 
>> participating in the discussion.  If everyone only sends patches and doesn't 
>> review patches, then we'll never make progress.
>>
>> So I'd strongly suggest trying to spend some time reviewing other people's work. 
>>   Right now, there are at least four different efforts around migration yet I 
>> don't see any of the people reviewing the other efforts.  I think this is really 
>> the main problem.
> 
> Point taken.
> However maintainers should also be responsible of reviewing patches of
> "infrequent write-only contributors".

Yes, but maintainers are overloaded because they also need to review
patches of frequent write-only contributors.

Kevin

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Stefano Stabellini]

On Mon, 12 Mar 2012, Anthony Liguori wrote:
> > OK, so the actual problem seems to be that not all the source files that
> > are supposed to be Supported are actually supported.
> > And of course some key files, like savevm.c are not even Maintained!!
> > For example if I am not mistaken we are missing an entry for vga/cirrus,
> > so that would also be Orphan, correct?
> 
> More likely Odd Fixes.  I think most of the things not listed are Odd Fixes 
> which for something like cirrus is fine.  I doubt it's ever going to see a big 
> invasive change as the code hasn't changed fundamentally in years.
> 
> But I agree, migration is an area that we need a proper maintainer for and 
> hopefully this thread will help make that happen.

Michael Roth demonstrated some interest in the role, maybe we should add
his email address in relation to savevm.c in the MAINTAINERS file.
If not as a maintainer yet, maybe just as a person of interest, I am not
sure whether a tag already exists for that. "M:" in theory just means
"Mail patches to" but in practice are all maintainers. If we want to
keep the separation clear we could introduce a new tag, maybe "I:" ?

It is true that most of the changes to vga.c are simple nowadays but it
would be nice to have somebody in charge of it, even if he would offer
only "Odd Fixes" as level of commitment.
You cannot be in charge of everything, unless you finally manage to make
that cloning machine down in basement work once and for all ;-)
Until then, we need more names in the MAINTAINERS file.
Is there anybody that feel like maintaining vga/cirrus?

Now that I look you are still the only one in charge of pc.c, pc_piix.c
and vl.c.  I think there is room for another x86 machine emulator
maintainer and another vl.c maintainer to help you out with the reviews.
Do you agree?  Who feels up for some x86 love? Anybody that feels up for
taking care of vl.c?

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Stefano Stabellini]

On Tue, 13 Mar 2012, Kevin Wolf wrote:
> Am 12.03.2012 18:34, schrieb Stefano Stabellini:
> > On Mon, 12 Mar 2012, Anthony Liguori wrote:
> >> On 03/12/2012 12:06 PM, Stefano Stabellini wrote:
> >>> Hi all,
> >>> I don't mean to steer any controversy or start any flame wars here, but
> >>> rather I want to point out a problem in the QEMU Community that is
> >>> preventing us and other people from having a good experience working
> >>> upstream with QEMU. Call it constructive criticism.
> >>>
> >>> Patches are being posted to the list that don't get any reviews at all.
> >>> Other patches get reviewed the first time, then once they are reposted
> >>> they don't get any other reviews or acked-by or reviewed-by.
> >>
> >> In all fairness, QEMU continues to grow year-to-year both in terms of total 
> >> commits and number of contributors.
> >>
> >> The area that we struggle with is infrequent contributors that contribute 
> >> non-trivial things and are write-only contributors.
> >>
> >> In this case, I really think the problem is expecting to be a write-only 
> >> contributor.  Part of participating in a community is not only pushing your own 
> >> patches for acceptance but also reviewing other people's patches and 
> >> participating in the discussion.  If everyone only sends patches and doesn't 
> >> review patches, then we'll never make progress.
> >>
> >> So I'd strongly suggest trying to spend some time reviewing other people's work. 
> >>   Right now, there are at least four different efforts around migration yet I 
> >> don't see any of the people reviewing the other efforts.  I think this is really 
> >> the main problem.
> > 
> > Point taken.
> > However maintainers should also be responsible of reviewing patches of
> > "infrequent write-only contributors".
> 
> Yes, but maintainers are overloaded because they also need to review
> patches of frequent write-only contributors.
 
That's why we need more maintainers!
Otherwise QEMU from an Open Source project becomes a Secret Circle project.

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Paolo Bonzini]

Il 13/03/2012 12:27, Kevin Wolf ha scritto:
>> > 
>> > Point taken.
>> > However maintainers should also be responsible of reviewing patches of
>> > "infrequent write-only contributors".
> Yes, but maintainers are overloaded because they also need to review
> patches of frequent write-only contributors.

Frequent non-maintainer contributors need to have their patches
reviewed, no matter if they're write-only or not.  To improve this we
could have more maintainers beyond the one who hosts the tree.  Listing
them in MAINTAINERS gets them automatically CCed in patches.  It's just
too easy to get lost in the mailing list otherwise.

Then, some areas are considerably busier than others, and the block
layer is particularly hard in this regard.  Stefan is helping a lot
reviewing, I should do the same.

Paolo

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Avi Kivity]

On 03/12/2012 08:18 PM, Stefano Stabellini wrote:
> > 
> > * Reviewed-by: Full Name <email>
> > 
> >   A Reviewed-by tag is a statement of opinion that the patch is an appropriate
> >   modification without any remaining serious technical issues. Any interested
> >   reviewer (who has done the work) can offer a Reviewed-by tag for a patch.
> > 
> > 
> > My understanding until now was that both Acked-by and Reviewed-by were tags
> > reserved to people with privileges to write into the repository.
>
> Anybody should be allowed to give his own Acked-by or Reviewed-by, not
> just maintainers. Of course an acked-by from the maintainer of the area
> the patch is touching has a different weight.

To me, an Ack is reserved for people who have authority in an area,
either by being the formal maintainer of the subsystem, or by just being
an expert in that area.  An Acked-by short-circuit's the following exchange:

  Author: submit patch P
  Maintainer: P touches subsystem X, what do Expert E and sub-maintainer
M have to say about it?
  E, M: looks okay

The acked-by allows the maintainer to skip the exchange.  Of course
usually patches should go through a submaintainer tree, but sometimes
this is not feasible, either because there is no tree for that area, or
because the patch or patchset touches many subsystems.

So an ack should come from people who expect to be asked about the patch.

(but this is nit-picking, any review is welcome regardless of the tag it
comes with)

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Avi Kivity]

On 03/12/2012 10:27 PM, Anthony Liguori wrote:
>> I agree that more maintainers would be good, but we also need
>> more people with commit rights.
>
> I disagree strongly.  Having multiple pushers makes things difficult
> and encourages people to push without testing.  Part of what makes
> pushing take longer than it should today is that my test cycle takes
> at least 1-2 hours and it's not uncommon to have to go through 3-4
> cycles of rebasing before being able to push.

This really sucks.

If testing was automated, we could have a staging branch where
maintainers would push patches, they'd get tested automatically and then
graduate to master.  The workflow would look something like

  git fetch
  git checkout staging
  git rebase origin/staging
  <apply patches, pull trees>
  git push staging
  <wait>
  <staging gets merged into master autoamatically, or you get an email
from the test system>

If testing cannot be automated, perhaps a lock around the tree would help.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Avi Kivity]

On 03/13/2012 11:09 AM, Peter Maydell wrote:
> > If we start saying that, Alex "owns" ppc except for things that are
> > "important" like a build breakage, then we get into the ugly definition of
> > what's important and what's not important.
>
> I don't think we've had huge problems with defining "trivial" and I
> don't think we'd really have big arguments about "urgent" either --
> as the gatekeeper you and the other direct-committers can always use
> your judgement and say 'this should go through the submaintainer tree'.
>
> I agree completely with Alex about why urgent fixes don't mesh well with
> the periodic submaintainer tree pullreq workflow. Dumping the 'urgent'
> fix problem off onto submaintainers is basically asking us all to
> have an extra 'foo-urgent' tree and send out single patch pullreqs,
> which seems to me like a very heavyweight way of causing a patch to
> be applied 


Not at all.  I have a memory/core branch and a memory/urgent branch --
it's trivial to maintain them with git, and quite often I send a 1-patch
pull request.  There's no material difference between sending a patch
and sending a pull request (except if you use git.kernel.org, ugh), and
it does guarantee you priority handing.

> (plus it puts an extra person in the loop which is pretty
> much guaranteed to slow things down).

Having the committers process all these patches (and monitor all
patches) is guaranteed to slow things down too.  We have maintainers who
are supposed to be experts in their area, and who are supposed to have
an interest in keeping their subsystem working, let them also take care
of build problems in their area of responsibility.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Anthony Liguori]

On 03/13/2012 08:40 AM, Avi Kivity wrote:
> On 03/12/2012 10:27 PM, Anthony Liguori wrote:
>>> I agree that more maintainers would be good, but we also need
>>> more people with commit rights.
>>
>> I disagree strongly.  Having multiple pushers makes things difficult
>> and encourages people to push without testing.  Part of what makes
>> pushing take longer than it should today is that my test cycle takes
>> at least 1-2 hours and it's not uncommon to have to go through 3-4
>> cycles of rebasing before being able to push.
>
> This really sucks.
>
> If testing was automated, we could have a staging branch where
> maintainers would push patches, they'd get tested automatically and then
> graduate to master.  The workflow would look something like
>
>    git fetch
>    git checkout staging
>    git rebase origin/staging
>    <apply patches, pull trees>
>    git push staging
>    <wait>
>    <staging gets merged into master autoamatically, or you get an email
> from the test system>

The problem for me with this is that I test before I do a thorough review.  I do 
a quick review, but not a line-by-line review.  So I don't necessarily want to 
queue for push.

> If testing cannot be automated, perhaps a lock around the tree would help.

I think merging qemu-test into make check would help a lot.  If all committers 
are running the same test suite before pushing, then this problem would become 
less common.  It's livable now because most committers commit infrequently.

But if we added more committers, it would become pretty problematic.

Regards,

Anthony Liguori

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Peter Maydell]

On 13 March 2012 13:50, Avi Kivity <avi@redhat.com> wrote:
> Not at all.  I have a memory/core branch and a memory/urgent branch --
> it's trivial to maintain them with git, and quite often I send a 1-patch
> pull request.  There's no material difference between sending a patch
> and sending a pull request (except if you use git.kernel.org, ugh), and
> it does guarantee you priority handing.

For me, setting up a pull request is considerably harder than
sending a mail saying "Reviewed-by:, this is a build breakage,
fix, please apply.".

Faffing around with an /urgent branch for the sake of the occasional
build fix in my area is exactly what I'd rather avoid.

>> (plus it puts an extra person in the loop which is pretty
>> much guaranteed to slow things down).
>
> Having the committers process all these patches (and monitor all
> patches) is guaranteed to slow things down too.

They're in the loop anyway, because they have to process the pull
request just as they would a single patch.

-- PMM

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Avi Kivity]

On 03/13/2012 04:00 PM, Anthony Liguori wrote:
> On 03/13/2012 08:40 AM, Avi Kivity wrote:
>> On 03/12/2012 10:27 PM, Anthony Liguori wrote:
>>>> I agree that more maintainers would be good, but we also need
>>>> more people with commit rights.
>>>
>>> I disagree strongly.  Having multiple pushers makes things difficult
>>> and encourages people to push without testing.  Part of what makes
>>> pushing take longer than it should today is that my test cycle takes
>>> at least 1-2 hours and it's not uncommon to have to go through 3-4
>>> cycles of rebasing before being able to push.
>>
>> This really sucks.
>>
>> If testing was automated, we could have a staging branch where
>> maintainers would push patches, they'd get tested automatically and then
>> graduate to master.  The workflow would look something like
>>
>>    git fetch
>>    git checkout staging
>>    git rebase origin/staging
>>    <apply patches, pull trees>
>>    git push staging
>>    <wait>
>>    <staging gets merged into master autoamatically, or you get an email
>> from the test system>
>
> The problem for me with this is that I test before I do a thorough
> review.  I do a quick review, but not a line-by-line review.  So I
> don't necessarily want to queue for push.

Seems to me it's better to review before testing, no?

>
>> If testing cannot be automated, perhaps a lock around the tree would
>> help.
>
> I think merging qemu-test into make check would help a lot.  If all
> committers are running the same test suite before pushing, then this
> problem would become less common.  It's livable now because most
> committers commit infrequently.
>
> But if we added more committers, it would become pretty problematic.

I'm not arguing either for or against that, just trying to make the
commit process more efficient.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Avi Kivity]

On 03/13/2012 04:12 PM, Peter Maydell wrote:
> >> (plus it puts an extra person in the loop which is pretty
> >> much guaranteed to slow things down).
> >
> > Having the committers process all these patches (and monitor all
> > patches) is guaranteed to slow things down too.
>
> They're in the loop anyway, because they have to process the pull
> request just as they would a single patch.

Anthony's shunting emails with [PULL] into a special queue, so yes
they're in the loop, but it's a smaller one.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Anthony Liguori]

On 03/13/2012 09:38 AM, Avi Kivity wrote:
> On 03/13/2012 04:00 PM, Anthony Liguori wrote:
>> On 03/13/2012 08:40 AM, Avi Kivity wrote:
>>> On 03/12/2012 10:27 PM, Anthony Liguori wrote:
>>>>> I agree that more maintainers would be good, but we also need
>>>>> more people with commit rights.
>>>>
>>>> I disagree strongly.  Having multiple pushers makes things difficult
>>>> and encourages people to push without testing.  Part of what makes
>>>> pushing take longer than it should today is that my test cycle takes
>>>> at least 1-2 hours and it's not uncommon to have to go through 3-4
>>>> cycles of rebasing before being able to push.
>>>
>>> This really sucks.
>>>
>>> If testing was automated, we could have a staging branch where
>>> maintainers would push patches, they'd get tested automatically and then
>>> graduate to master.  The workflow would look something like
>>>
>>>     git fetch
>>>     git checkout staging
>>>     git rebase origin/staging
>>>     <apply patches, pull trees>
>>>     git push staging
>>>     <wait>
>>>     <staging gets merged into master autoamatically, or you get an email
>>> from the test system>
>>
>> The problem for me with this is that I test before I do a thorough
>> review.  I do a quick review, but not a line-by-line review.  So I
>> don't necessarily want to queue for push.
>
> Seems to me it's better to review before testing, no?

I typically do a high level review before queuing for testing, but I don't do a 
line-by-line review for coding style or minor issues.

The later must be done before committing no matter how many revisions are sent. 
  It's more time efficient to catch a functional problem without doing the 
line-by-line review.  Best case scenario is that the line-by-line review happens 
only once for a patch before it's committed.

>>> If testing cannot be automated, perhaps a lock around the tree would
>>> help.
>>
>> I think merging qemu-test into make check would help a lot.  If all
>> committers are running the same test suite before pushing, then this
>> problem would become less common.  It's livable now because most
>> committers commit infrequently.
>>
>> But if we added more committers, it would become pretty problematic.
>
> I'm not arguing either for or against that, just trying to make the
> commit process more efficient.

Yup, and appreciate the suggestions.

Regards,

Anthony Liguori

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Anthony Liguori]

On 03/13/2012 09:39 AM, Avi Kivity wrote:
> On 03/13/2012 04:12 PM, Peter Maydell wrote:
>>>> (plus it puts an extra person in the loop which is pretty
>>>> much guaranteed to slow things down).
>>>
>>> Having the committers process all these patches (and monitor all
>>> patches) is guaranteed to slow things down too.
>>
>> They're in the loop anyway, because they have to process the pull
>> request just as they would a single patch.
>
> Anthony's shunting emails with [PULL] into a special queue, so yes
> they're in the loop, but it's a smaller one.

Right, if we want urgency on something, there has to be a filterable keyword.  I 
don't read every single message on qemu-devel every hour of the day.

The advantage of [PULL] over [urgent] is that it fits the existing workflows. 
It's clear who should be doing [PULL] requests.

Regards,

Anthony Liguori

>

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Alexander Graf]

On 13.03.2012, at 15:43, Anthony Liguori wrote:

> On 03/13/2012 09:39 AM, Avi Kivity wrote:
>> On 03/13/2012 04:12 PM, Peter Maydell wrote:
>>>>> (plus it puts an extra person in the loop which is pretty
>>>>> much guaranteed to slow things down).
>>>> 
>>>> Having the committers process all these patches (and monitor all
>>>> patches) is guaranteed to slow things down too.
>>> 
>>> They're in the loop anyway, because they have to process the pull
>>> request just as they would a single patch.
>> 
>> Anthony's shunting emails with [PULL] into a special queue, so yes
>> they're in the loop, but it's a smaller one.
> 
> Right, if we want urgency on something, there has to be a filterable keyword.  I don't read every single message on qemu-devel every hour of the day.
> 
> The advantage of [PULL] over [urgent] is that it fits the existing workflows. It's clear who should be doing [PULL] requests.

The only flaw in this is that Anthony doesn't pull from my trees, so having him notified doesn't help me ;).


Alex

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Andreas Färber]

Am 13.03.2012 14:50, schrieb Avi Kivity:
> On 03/13/2012 11:09 AM, Peter Maydell wrote:
>>> If we start saying that, Alex "owns" ppc except for things that are
>>> "important" like a build breakage, then we get into the ugly definition of
>>> what's important and what's not important.
>>
>> I don't think we've had huge problems with defining "trivial" and I
>> don't think we'd really have big arguments about "urgent" either --
>> as the gatekeeper you and the other direct-committers can always use
>> your judgement and say 'this should go through the submaintainer tree'.
>>
>> I agree completely with Alex about why urgent fixes don't mesh well with
>> the periodic submaintainer tree pullreq workflow. Dumping the 'urgent'
>> fix problem off onto submaintainers is basically asking us all to
>> have an extra 'foo-urgent' tree and send out single patch pullreqs,
>> which seems to me like a very heavyweight way of causing a patch to
>> be applied 
> 
> 
> Not at all.  I have a memory/core branch and a memory/urgent branch --
> it's trivial to maintain them with git, and quite often I send a 1-patch
> pull request.  There's no material difference between sending a patch
> and sending a pull request (except if you use git.kernel.org, ugh), and
> it does guarantee you priority handing.

Actually there is: A trivial fix can be sent with a one-liner:

$ git send-email HEAD^

whereas there is no matching command for sending a pull request. You
need to manage your own scripts that place git-pull-request output into
the cover letter. (And quite obviously you need a publicly available git
repository in the first place. Most of us do by now.)

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Peter Maydell]

On 13 March 2012 14:43, Anthony Liguori <anthony@codemonkey.ws> wrote:
> Right, if we want urgency on something, there has to be a filterable
> keyword.  I don't read every single message on qemu-devel every hour of the
> day.

Yes, this is where we came in when I suggested some kind of
qemu-urgent or urgent tag...

> The advantage of [PULL] over [urgent] is that it fits the existing
> workflows. It's clear who should be doing [PULL] requests.

It is for some parts of the tree (ie the ones you do). It is less
so for other parts, as Alex points out.

-- PMM

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Avi Kivity]

On 03/13/2012 04:49 PM, Andreas Färber wrote:
> > 
> > Not at all.  I have a memory/core branch and a memory/urgent branch --
> > it's trivial to maintain them with git, and quite often I send a 1-patch
> > pull request.  There's no material difference between sending a patch
> > and sending a pull request (except if you use git.kernel.org, ugh), and
> > it does guarantee you priority handing.
>
> Actually there is: A trivial fix can be sent with a one-liner:
>
> $ git send-email HEAD^

Hey, I didn't know about this.  Thanks.

> whereas there is no matching command for sending a pull request. You
> need to manage your own scripts that place git-pull-request output into
> the cover letter. (And quite obviously you need a publicly available git
> repository in the first place. Most of us do by now.)

Implied by being a maintainer.

So yes, it takes a little more effort to send a pull request.  But it's
still pretty easy.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 747 bytes --]

On 03/13/2012 08:57 AM, Avi Kivity wrote:
> On 03/13/2012 04:49 PM, Andreas Färber wrote:
>>>
>>> Not at all.  I have a memory/core branch and a memory/urgent branch --
>>> it's trivial to maintain them with git, and quite often I send a 1-patch
>>> pull request.  There's no material difference between sending a patch
>>> and sending a pull request (except if you use git.kernel.org, ugh), and
>>> it does guarantee you priority handing.
>>
>> Actually there is: A trivial fix can be sent with a one-liner:
>>
>> $ git send-email HEAD^
> 
> Hey, I didn't know about this.  Thanks.

Even shorter:

git send-email -1

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Andreas Färber]

Am 13.03.2012 11:41, schrieb Stefan Hajnoczi:
> On Mon, Mar 12, 2012 at 8:12 PM, Stefan Weil <sw@weilnetz.de> wrote:
>> Am 12.03.2012 18:06, schrieb Stefano Stabellini:
>> Maybe every maintainer can maintain a short summary of
>> what he maintains, how (s)he does it (repository, expected
>> response time, ...) in the QEMU wiki. I just added
>> http://wiki.qemu.org/Contribute/StartHere#Maintainers
>> and improved my own wiki page ("leading by example :-)").
> 
> While I understand the sentiment, I don't think duplicating
> ./MAINTAINERS on the wiki is a good idea.  I suggest linking solely to
> the ./MAINTAINERS file from the wiki instead of actually listing
> people.
> 
> I have not removed your edit because I wanted to see what you think first.

We already have it duplicated here:

http://wiki.qemu.org/Features

I'd suggest to drop that page.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Stefan Weil]

Am 13.03.2012 17:31, schrieb Andreas Färber:
> Am 13.03.2012 11:41, schrieb Stefan Hajnoczi:
>> On Mon, Mar 12, 2012 at 8:12 PM, Stefan Weil <sw@weilnetz.de> wrote:
>>> Am 12.03.2012 18:06, schrieb Stefano Stabellini:
>>> Maybe every maintainer can maintain a short summary of
>>> what he maintains, how (s)he does it (repository, expected
>>> response time, ...) in the QEMU wiki. I just added
>>> http://wiki.qemu.org/Contribute/StartHere#Maintainers
>>> and improved my own wiki page ("leading by example :-)").
>>
>> While I understand the sentiment, I don't think duplicating
>> ./MAINTAINERS on the wiki is a good idea. I suggest linking solely to
>> the ./MAINTAINERS file from the wiki instead of actually listing
>> people.
>>
>> I have not removed your edit because I wanted to see what you think 
>> first.

My intention was simply to allow (and encourage) maintainers
to explain their way of working and to allow users to get this
information easily.


>
> We already have it duplicated here:
>
> http://wiki.qemu.org/Features
>
> I'd suggest to drop that page.
>
> Andreas

Duplicating information is not necessary of course, but it does not harm
in a wiki as long as it is easy to keep the information up to date.
Adding links for every maintainer on the Feature page would help.
Comments and git tree could be moved to the personal page of every
maintainer.

I have no special preference how the maintainers should be listed in the 
wiki,
but I think they should be listed there (not only in MAINTAINERS) 
because that
allows links. I just merged the existing list of developers and the 
maintainer
list (http://wiki.qemu.org/Contribute/StartHere#Developers_and_Maintainers).
Do you think that's a better and possible solution?

Regards,
Stefan W.

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Stefan Hajnoczi]

On Tue, Mar 13, 2012 at 6:14 PM, Stefan Weil <sw@weilnetz.de> wrote:
> Am 13.03.2012 17:31, schrieb Andreas Färber:
>
>> Am 13.03.2012 11:41, schrieb Stefan Hajnoczi:
>>>
>>> On Mon, Mar 12, 2012 at 8:12 PM, Stefan Weil <sw@weilnetz.de> wrote:
>>>>
>>>> Am 12.03.2012 18:06, schrieb Stefano Stabellini:
>>>> Maybe every maintainer can maintain a short summary of
>>>> what he maintains, how (s)he does it (repository, expected
>>>> response time, ...) in the QEMU wiki. I just added
>>>> http://wiki.qemu.org/Contribute/StartHere#Maintainers
>>>> and improved my own wiki page ("leading by example :-)").
>>>
>>>
>>> While I understand the sentiment, I don't think duplicating
>>> ./MAINTAINERS on the wiki is a good idea. I suggest linking solely to
>>> the ./MAINTAINERS file from the wiki instead of actually listing
>>> people.
>>>
>>> I have not removed your edit because I wanted to see what you think
>>> first.
>
>
> My intention was simply to allow (and encourage) maintainers
> to explain their way of working and to allow users to get this
> information easily.
>
>
>
>>
>> We already have it duplicated here:
>>
>> http://wiki.qemu.org/Features
>>
>> I'd suggest to drop that page.
>>
>> Andreas
>
>
> Duplicating information is not necessary of course, but it does not harm
> in a wiki as long as it is easy to keep the information up to date.
> Adding links for every maintainer on the Feature page would help.
> Comments and git tree could be moved to the personal page of every
> maintainer.

Knowing myself I'm sure my entry will get out-of-date.  I don't want
to update anything other than ./MAINTAINERS.

If other maintainers find it useful to keep wiki pages, that's fine.
I just won't be doing it because it doesn't work for me.

> I have no special preference how the maintainers should be listed in the
> wiki,
> but I think they should be listed there (not only in MAINTAINERS) because
> that
> allows links.

./MAINTAINERS allows links too.  The "W:" field can be used for website URLs.

> I just merged the existing list of developers and the
> maintainer
> list (http://wiki.qemu.org/Contribute/StartHere#Developers_and_Maintainers).
> Do you think that's a better and possible solution?

Yes, that seems fine - it gives folks a chance to create wiki pages if
they like.

Stefan

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Andreas Färber]

Am 13.03.2012 14:27, schrieb Avi Kivity:
> On 03/12/2012 08:18 PM, Stefano Stabellini wrote:
>>>
>>> * Reviewed-by: Full Name <email>
>>>
>>>   A Reviewed-by tag is a statement of opinion that the patch is an appropriate
>>>   modification without any remaining serious technical issues. Any interested
>>>   reviewer (who has done the work) can offer a Reviewed-by tag for a patch.
>>>
>>>
>>> My understanding until now was that both Acked-by and Reviewed-by were tags
>>> reserved to people with privileges to write into the repository.
>>
>> Anybody should be allowed to give his own Acked-by or Reviewed-by, not
>> just maintainers. Of course an acked-by from the maintainer of the area
>> the patch is touching has a different weight.
> 
> To me, an Ack is reserved for people who have authority in an area,
> either by being the formal maintainer of the subsystem, or by just being
> an expert in that area.  An Acked-by short-circuit's the following exchange:
> 
>   Author: submit patch P
>   Maintainer: P touches subsystem X, what do Expert E and sub-maintainer
> M have to say about it?
>   E, M: looks okay
> 
> The acked-by allows the maintainer to skip the exchange.  Of course
> usually patches should go through a submaintainer tree, but sometimes
> this is not feasible, either because there is no tree for that area, or
> because the patch or patchset touches many subsystems.
> 
> So an ack should come from people who expect to be asked about the patch.

The way I saw it, Acked-by means that the person asserts that the
contents of the change is sensible, and when I use it I either tested it
myself or am absolutely sure it doesn't break the build.

Reviewed-by I use by comparison to assert that a patch reasonably
conforms to our Coding guidelines, has an SoB and does nothing obviously
stupid but that I did not bother to smoke-test on my system.

What I have wondered is, is there any semantic difference between "Ack",
"Acked", "ACK" and "Acked-by: name <email>"? I.e., when someone replies
with "Ack", should one document that as an Acked-by for a PULL?

Similarly, should "Looks good." be translated to Reviewed-by or does it
mean less?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Anthony Liguori]

On 03/14/2012 08:50 AM, Andreas Färber wrote:
> Am 13.03.2012 14:27, schrieb Avi Kivity:
>> On 03/12/2012 08:18 PM, Stefano Stabellini wrote:
>>>>
>>>> * Reviewed-by: Full Name<email>
>>>>
>>>>    A Reviewed-by tag is a statement of opinion that the patch is an appropriate
>>>>    modification without any remaining serious technical issues. Any interested
>>>>    reviewer (who has done the work) can offer a Reviewed-by tag for a patch.
>>>>
>>>>
>>>> My understanding until now was that both Acked-by and Reviewed-by were tags
>>>> reserved to people with privileges to write into the repository.
>>>
>>> Anybody should be allowed to give his own Acked-by or Reviewed-by, not
>>> just maintainers. Of course an acked-by from the maintainer of the area
>>> the patch is touching has a different weight.
>>
>> To me, an Ack is reserved for people who have authority in an area,
>> either by being the formal maintainer of the subsystem, or by just being
>> an expert in that area.  An Acked-by short-circuit's the following exchange:
>>
>>    Author: submit patch P
>>    Maintainer: P touches subsystem X, what do Expert E and sub-maintainer
>> M have to say about it?
>>    E, M: looks okay
>>
>> The acked-by allows the maintainer to skip the exchange.  Of course
>> usually patches should go through a submaintainer tree, but sometimes
>> this is not feasible, either because there is no tree for that area, or
>> because the patch or patchset touches many subsystems.
>>
>> So an ack should come from people who expect to be asked about the patch.
>
> The way I saw it, Acked-by means that the person asserts that the
> contents of the change is sensible, and when I use it I either tested it
> myself or am absolutely sure it doesn't break the build.
>
> Reviewed-by I use by comparison to assert that a patch reasonably
> conforms to our Coding guidelines, has an SoB and does nothing obviously
> stupid but that I did not bother to smoke-test on my system.
>
> What I have wondered is, is there any semantic difference between "Ack",
> "Acked", "ACK" and "Acked-by: name<email>"? I.e., when someone replies
> with "Ack", should one document that as an Acked-by for a PULL?

No, Acked-by: name<email> is a formal statement.  You shouldn't infer an 
Acked-by IMHO.

FWIW, you can always ask for an actual Acked-by if someone responds with just Ack.

>
> Similarly, should "Looks good." be translated to Reviewed-by or does it
> mean less?

It means less.

Regards,

Anthony Liguori

> Andreas
>

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Peter Maydell]

On 14 March 2012 13:52, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 03/14/2012 08:50 AM, Andreas Färber wrote:
>> What I have wondered is, is there any semantic difference between "Ack",
>> "Acked", "ACK" and "Acked-by: name<email>"? I.e., when someone replies
>> with "Ack", should one document that as an Acked-by for a PULL?
>
>
> No, Acked-by: name<email> is a formal statement.  You shouldn't infer an
> Acked-by IMHO.

This is in contradiction to the kernel docs we reference, which say:
# Acked-by: is not as formal as Signed-off-by:.  It is a record that the acker
# has at least reviewed the patch and has indicated acceptance.  Hence patch
# mergers will sometimes manually convert an acker's "yep, looks good to me"
# into an Acked-by:.

-- PMM

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Anthony Liguori]

On 03/14/2012 08:58 AM, Peter Maydell wrote:
> On 14 March 2012 13:52, Anthony Liguori<anthony@codemonkey.ws>  wrote:
>> On 03/14/2012 08:50 AM, Andreas Färber wrote:
>>> What I have wondered is, is there any semantic difference between "Ack",
>>> "Acked", "ACK" and "Acked-by: name<email>"? I.e., when someone replies
>>> with "Ack", should one document that as an Acked-by for a PULL?
>>
>>
>> No, Acked-by: name<email>  is a formal statement.  You shouldn't infer an
>> Acked-by IMHO.
>
> This is in contradiction to the kernel docs we reference, which say:
> # Acked-by: is not as formal as Signed-off-by:.  It is a record that the acker
> # has at least reviewed the patch and has indicated acceptance.  Hence patch
> # mergers will sometimes manually convert an acker's "yep, looks good to me"
> # into an Acked-by:.

Oh, I guess I stand corrected.  It would certainly surprise me if someone added 
my Acked-by without asking me but maybe this is just American politeness...

Regards,

Anthony Liguori

>
> -- PMM
>

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Andreas Färber]

Am 14.03.2012 15:17, schrieb Anthony Liguori:
> On 03/14/2012 08:58 AM, Peter Maydell wrote:
>> On 14 March 2012 13:52, Anthony Liguori<anthony@codemonkey.ws>  wrote:
>>> On 03/14/2012 08:50 AM, Andreas Färber wrote:
>>>> What I have wondered is, is there any semantic difference between
>>>> "Ack",
>>>> "Acked", "ACK" and "Acked-by: name<email>"? I.e., when someone replies
>>>> with "Ack", should one document that as an Acked-by for a PULL?
>>>
>>>
>>> No, Acked-by: name<email>  is a formal statement.  You shouldn't
>>> infer an
>>> Acked-by IMHO.
>>
>> This is in contradiction to the kernel docs we reference, which say:
>> # Acked-by: is not as formal as Signed-off-by:.  It is a record that
>> the acker
>> # has at least reviewed the patch and has indicated acceptance.  Hence
>> patch
>> # mergers will sometimes manually convert an acker's "yep, looks good
>> to me"
>> # into an Acked-by:.
> 
> Oh, I guess I stand corrected.  It would certainly surprise me if
> someone added my Acked-by without asking me but maybe this is just
> American politeness...

And seeing what politeness got us into yesterday I'm asking... ;-)

I was surprised by the relicensing how-to btw that instructed to add
people's SoB - I usually tend to think that agreeing on how to go about
something and seeing how it was actually done would be separate issues.
But then again the SoBs get cc'ed so have a chance to object.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Blue Swirl]

On Tue, Mar 13, 2012 at 01:01, Andreas Färber <afaerber@suse.de> wrote:
> Am 13.03.2012 01:16, schrieb Anthony Liguori:
>> On 03/12/2012 06:32 PM, Andreas Färber wrote:
>>> Take Blue's recent target-ppc fix
>>> 9d4df9c02866f39d3eef105033091f367cc7c29e for example: After applying
>>> patches on day one of FOSDEM he posted a -Werror fix, it got confirmed
>>> by me and Alex but wasn't applied until a week later, because apparently
>>> no other committer dared to apply Blue's patch despite SoB and acks and
>>> people reporting the issue... Not happy.
>>> No doubt Alex is to blame for not catching that issue in his ppc queue,
>>> but asking Alex as submaintainer to submit a PULL for a single patch
>>> posted by Blue as committer seems overly complicated to me! ;)
>>
>> I think this is a good demonstration of what the problem is.  Unclear
>> responsibility.
>
> Agreed. Solution is documentation of expected workflows.
>
>>  I'm pretty sure that Blue thought that Alex would
>> handle the patch.  I'm pretty sure that Alex thought Blue would handle
>> the patch.
>
> Alex actually asked Blue to.
>
> However from what I understand, Blue is not working on QEMU full-time,
> like me previously. I assume he handled the patch once he read and came
> around to it. It's just that no one else with commit powers reacted to
> the situation.

Real life can be pretty demanding at times.

I can't imagine when I would mind if somebody else applied patches
I've sent if they are not RFC.

> The way I see it no one "owns" code in QEMU. Some people feel
> responsible for (or comfortable reviewing changes to) parts of QEMU, and
> the project scales by distributing review, testing and queuing to more
> such shoulders.
> However where a (sub)maintainer is unresponsive - and *there* I
> differentiate between build breakages, runtime issues and feature
> additions - we can't wait forever and need to adapt processes. Fixing
> the build within a reasonable time is one requirement, moving forward
> with target-mips at all is another example.
>
> It's not really that we have too few maintainers, it's that not all
> maintainers maintain at all times - for valid work or personal reasons -
> and we don't seem to have a well-working escalation mechanism beyond
> ping^n to handle that.

This particular issue could have been avoided with more thorough
compile testing by me before pushing Alex's PPC tree, or by Alex
himself before submitting d1e256fe.

> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Blue Swirl]

On Tue, Mar 13, 2012 at 13:40, Avi Kivity <avi@redhat.com> wrote:
> On 03/12/2012 10:27 PM, Anthony Liguori wrote:
>>> I agree that more maintainers would be good, but we also need
>>> more people with commit rights.
>>
>> I disagree strongly.  Having multiple pushers makes things difficult
>> and encourages people to push without testing.  Part of what makes
>> pushing take longer than it should today is that my test cycle takes
>> at least 1-2 hours and it's not uncommon to have to go through 3-4
>> cycles of rebasing before being able to push.
>
> This really sucks.
>
> If testing was automated, we could have a staging branch where
> maintainers would push patches, they'd get tested automatically and then
> graduate to master.  The workflow would look something like
>
>  git fetch
>  git checkout staging
>  git rebase origin/staging
>  <apply patches, pull trees>
>  git push staging
>  <wait>
>  <staging gets merged into master autoamatically, or you get an email
> from the test system>

This would be interesting, but it requires a powerful machine for
staging so that it does not become a bottle neck and multiple pushes
are not delayed. Better would be to run standard checks for all
maintainers' trees, or we could agree that all maintainers run a the
standard set before pushing or asking for pulls.

> If testing cannot be automated, perhaps a lock around the tree would help.
>
> --
> error compiling committee.c: too many arguments to function
>
>

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Blue Swirl]

On Tue, Mar 13, 2012 at 14:41, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 03/13/2012 09:38 AM, Avi Kivity wrote:
>>
>> On 03/13/2012 04:00 PM, Anthony Liguori wrote:
>>>
>>> On 03/13/2012 08:40 AM, Avi Kivity wrote:
>>>>
>>>> On 03/12/2012 10:27 PM, Anthony Liguori wrote:
>>>>>>
>>>>>> I agree that more maintainers would be good, but we also need
>>>>>> more people with commit rights.
>>>>>
>>>>>
>>>>> I disagree strongly.  Having multiple pushers makes things difficult
>>>>> and encourages people to push without testing.  Part of what makes
>>>>> pushing take longer than it should today is that my test cycle takes
>>>>> at least 1-2 hours and it's not uncommon to have to go through 3-4
>>>>> cycles of rebasing before being able to push.
>>>>
>>>>
>>>> This really sucks.
>>>>
>>>> If testing was automated, we could have a staging branch where
>>>> maintainers would push patches, they'd get tested automatically and then
>>>> graduate to master.  The workflow would look something like
>>>>
>>>>    git fetch
>>>>    git checkout staging
>>>>    git rebase origin/staging
>>>>    <apply patches, pull trees>
>>>>    git push staging
>>>>    <wait>
>>>>    <staging gets merged into master autoamatically, or you get an email
>>>> from the test system>
>>>
>>>
>>> The problem for me with this is that I test before I do a thorough
>>> review.  I do a quick review, but not a line-by-line review.  So I
>>> don't necessarily want to queue for push.
>>
>>
>> Seems to me it's better to review before testing, no?
>
>
> I typically do a high level review before queuing for testing, but I don't
> do a line-by-line review for coding style or minor issues.
>
> The later must be done before committing no matter how many revisions are
> sent.  It's more time efficient to catch a functional problem without doing
> the line-by-line review.  Best case scenario is that the line-by-line review
> happens only once for a patch before it's committed.

Perhaps patchwork is not the right tool for this. Coreboot uses
Gerrit: http://review.coreboot.org/#/q/status:open,n,z combined with
Jenkins build bot. This looks much more professional.

>>>> If testing cannot be automated, perhaps a lock around the tree would
>>>> help.
>>>
>>>
>>> I think merging qemu-test into make check would help a lot.  If all
>>> committers are running the same test suite before pushing, then this
>>> problem would become less common.  It's livable now because most
>>> committers commit infrequently.
>>>
>>> But if we added more committers, it would become pretty problematic.
>>
>>
>> I'm not arguing either for or against that, just trying to make the
>> commit process more efficient.
>
>
> Yup, and appreciate the suggestions.
>
> Regards,
>
> Anthony Liguori
>
>
>

Re: [Qemu-devel] We need more reviewers/maintainers!!

[Peter Maydell]

On 12 March 2012 20:12, Stefan Weil <sw@weilnetz.de> wrote:
> We also need more resources for technical maintenance of the
> QEMU infrastructure. For example, the official mirror of the
> QEMU git repository (https://github.com/qemu/QEMU) is several
> months behind, http://git.savannah.gnu.org/cgit/qemu.git is
> even older.

The github mirror is still wildly out of date -- can we
get the link to it removed from http://wiki.qemu.org/Download
please? (I'd do it myself but the page is 'locked'.)

thanks
-- PMM