Patchwork [RESEND] RTC polling mode broken

login
register
mail settings
Submitter Bernhard Kauer
Date Sept. 2, 2009, 7:49 a.m.
Message ID <20090902074905.GB25711@chrom.inf.tu-dresden.de>
Download mbox | patch
Permalink /patch/32816/
State Superseded
Headers show

Comments

Bernhard Kauer - Sept. 2, 2009, 7:49 a.m.
The RTC emulation does not set the IRQ flags independent of the IRQ enable bits.

The original MC146818A datasheet from 1984 notes:
        "flag bits in Register C [...] are set independent of the
        state of the corresponding enable bits in Register B"
Similar sections can be found in newer documentation e.g. in rtc82885.

Qemu and Bochs set the IRQ flags only if they are enabled,
which breaks drivers polling on them.

The following patch corrects this for the update-ended-flag in Qemu only.
It does not fix the handling of the other flags.


Signed-off-by: Bernhard Kauer <kauer@tudos.org>
Bernhard Kauer - Sept. 9, 2009, 12:18 p.m.
Its wednesday again, time to resend a patch to the list.
Until today the following happened in this endless story:

	1. I wrote an RTC driver according to the manual which works on real hardware but not on Qemu.
	2. I traced the bug back to the first public version of Bochs. Its seems to be also
	   present in all derived version: Qemu, KVM, Xen, VBox...
	3. I send a patch to the Qemu list at least 4 times.
	4. I never got a single response to my patch.

Thus I have to assume that device models in Qemu are unmaintained: further resending 
does not make any sense in this case.

I still hope somebody will step up and take a stake in this story.  Otherwise Qemu
will be more and more unusable.  For me and probably others.


Greetings,

	 Bernhard Kauer



On Wed, Sep 02, 2009 at 09:49:05AM +0200, Bernhard Kauer wrote:
> The RTC emulation does not set the IRQ flags independent of the IRQ enable bits.
> 
> The original MC146818A datasheet from 1984 notes:
>         "flag bits in Register C [...] are set independent of the
>         state of the corresponding enable bits in Register B"
> Similar sections can be found in newer documentation e.g. in rtc82885.
> 
> Qemu and Bochs set the IRQ flags only if they are enabled,
> which breaks drivers polling on them.
> 
> The following patch corrects this for the update-ended-flag in Qemu only.
> It does not fix the handling of the other flags.
> 
> 
> Signed-off-by: Bernhard Kauer <kauer@tudos.org>
> 
> 
> diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
> index 2022548..2b040a7 100644
> --- a/hw/mc146818rtc.c
> +++ b/hw/mc146818rtc.c
> @@ -421,9 +421,10 @@ static void rtc_update_second2(void *opaque)
>      }
> 
>      /* update ended interrupt */
> +    s->cmos_data[RTC_REG_C] |= REG_C_UF;
>      if (s->cmos_data[RTC_REG_B] & REG_B_UIE) {
> -        s->cmos_data[RTC_REG_C] |= 0x90;
> -        rtc_irq_raise(s->irq);
> +      s->cmos_data[RTC_REG_C] |= REG_C_IRQF;
> +      rtc_irq_raise(s->irq);
>      }
> 
>      /* clear update in progress bit */
> 
> 
>
Amit Shah - Sept. 9, 2009, 12:23 p.m.
On (Wed) Sep 09 2009 [14:18:17], Bernhard Kauer wrote:
> Its wednesday again, time to resend a patch to the list.
> Until today the following happened in this endless story:
> 
> 	1. I wrote an RTC driver according to the manual which works on real hardware but not on Qemu.
> 	2. I traced the bug back to the first public version of Bochs. Its seems to be also
> 	   present in all derived version: Qemu, KVM, Xen, VBox...
> 	3. I send a patch to the Qemu list at least 4 times.
> 	4. I never got a single response to my patch.

It's included in Anthony's staging tree:

http://repo.or.cz/w/qemu/aliguori-queue.git

which means it can be flushed to the main git tree in the near future.

The current process of picking up patches is not verbose at all;
developers either have to wait till the patch magically appears in the
git tree or keep resending it.

		Amit
Kevin Wolf - Sept. 9, 2009, 12:25 p.m.
Hi Bernhard,

Am 09.09.2009 14:18, schrieb Bernhard Kauer:
> Its wednesday again, time to resend a patch to the list.
> Until today the following happened in this endless story:
> 
> 	1. I wrote an RTC driver according to the manual which works on real hardware but not on Qemu.
> 	2. I traced the bug back to the first public version of Bochs. Its seems to be also
> 	   present in all derived version: Qemu, KVM, Xen, VBox...
> 	3. I send a patch to the Qemu list at least 4 times.
> 	4. I never got a single response to my patch.
> 
> Thus I have to assume that device models in Qemu are unmaintained: further resending 
> does not make any sense in this case.
> 
> I still hope somebody will step up and take a stake in this story.  Otherwise Qemu
> will be more and more unusable.  For me and probably others.

The patch seems to be in Anthony's queue at least (see
http://repo.or.cz/w/qemu/aliguori-queue.git), so it might be committed
the next time he pushes.

Kevin
Bernhard Kauer - Sept. 9, 2009, 12:58 p.m.
On Wed, Sep 09, 2009 at 05:53:49PM +0530, Amit Shah wrote:
> On (Wed) Sep 09 2009 [14:18:17], Bernhard Kauer wrote:
> > Its wednesday again, time to resend a patch to the list.
> > Until today the following happened in this endless story:
> > 
> > 	1. I wrote an RTC driver according to the manual which works on real hardware but not on Qemu.
> > 	2. I traced the bug back to the first public version of Bochs. Its seems to be also
> > 	   present in all derived version: Qemu, KVM, Xen, VBox...
> > 	3. I send a patch to the Qemu list at least 4 times.
> > 	4. I never got a single response to my patch.
> 
> It's included in Anthony's staging tree:
> 
> http://repo.or.cz/w/qemu/aliguori-queue.git
> 
> which means it can be flushed to the main git tree in the near future.

Ah i missed this.


> The current process of picking up patches is not verbose at all;
> developers either have to wait till the patch magically appears in the
> git tree or keep resending it.

Yes, it would be nice to get a mail when somebody picked up the patch.


Thanks,

	Bernhard
Anthony Liguori - Sept. 9, 2009, 1 p.m.
Bernhard Kauer wrote:
> Its wednesday again, time to resend a patch to the list.
> Until today the following happened in this endless story:
>   

Really, the whining just makes me want to drop your patch..

The first patch you sent didn't have [PATCH] in the subject.  There are 
hundreds of mails that are sent to the list every day.  If you don't 
mark a patch in a way that allows automatic filtering, it will get dropped.

When you resent, you did include [PATCH].  That patch made it into my 
staging tree.  See http://repo.or.cz/w/qemu/aliguori-queue.git.  We had 
a holiday this past weekend so I'm a little slow in flushing that queue 
to master.

Regards,

Anthony Liguori
Gleb Natapov - Sept. 9, 2009, 1:26 p.m.
On Wed, Sep 09, 2009 at 08:00:28AM -0500, Anthony Liguori wrote:
> Bernhard Kauer wrote:
> >Its wednesday again, time to resend a patch to the list.
> >Until today the following happened in this endless story:
> 
> Really, the whining just makes me want to drop your patch..
> 
> The first patch you sent didn't have [PATCH] in the subject.  There
> are hundreds of mails that are sent to the list every day.  If you
> don't mark a patch in a way that allows automatic filtering, it will
> get dropped.
> 
> When you resent, you did include [PATCH].  That patch made it into
> my staging tree.  See http://repo.or.cz/w/qemu/aliguori-queue.git.
> We had a holiday this past weekend so I'm a little slow in flushing
> that queue to master.
> 
It would be really nice to get mail that your patch was applied to staging.
Casual contributor don't know that there is staging tree that should be
checked or separate mailing list that he should subscribe too.

--
			Gleb.
Bernhard Kauer - Sept. 9, 2009, 1:34 p.m.
On Wed, Sep 09, 2009 at 08:00:28AM -0500, Anthony Liguori wrote:
> The first patch you sent didn't have [PATCH] in the subject.  There
> are hundreds of mails that are sent to the list every day.  If you
> don't mark a patch in a way that allows automatic filtering, it will
> get dropped.

That was at a time I thought: it cann't be true, that nobody noticed
the bug that many years.  Time proved otherwise...


> When you resent, you did include [PATCH].  That patch made it into
> my staging tree.  See http://repo.or.cz/w/qemu/aliguori-queue.git.
> We had a holiday this past weekend so I'm a little slow in flushing
> that queue to master.

Your speed is not the problem.  Getting a mail when you picked up the
patch would have avoided my frustration. 

A mail to the mailinglist with all patches that made it in your tree
could be also an option.


Greetings,

	Bernhard Kauer
Anthony Liguori - Sept. 9, 2009, 1:45 p.m.
Gleb Natapov wrote:
> On Wed, Sep 09, 2009 at 08:00:28AM -0500, Anthony Liguori wrote:
>   
>> Bernhard Kauer wrote:
>>     
>>> Its wednesday again, time to resend a patch to the list.
>>> Until today the following happened in this endless story:
>>>       
>> Really, the whining just makes me want to drop your patch..
>>
>> The first patch you sent didn't have [PATCH] in the subject.  There
>> are hundreds of mails that are sent to the list every day.  If you
>> don't mark a patch in a way that allows automatic filtering, it will
>> get dropped.
>>
>> When you resent, you did include [PATCH].  That patch made it into
>> my staging tree.  See http://repo.or.cz/w/qemu/aliguori-queue.git.
>> We had a holiday this past weekend so I'm a little slow in flushing
>> that queue to master.
>>
>>     
> It would be really nice to get mail that your patch was applied to staging.
> Casual contributor don't know that there is staging tree that should be
> checked or separate mailing list that he should subscribe too.
>   

I would like to use patchworks for this but I haven't figured out a sane 
way to integrate it into my workflow yet.

Regards,

Anthony Liguori

> --
> 			Gleb.
>
Avi Kivity - Sept. 9, 2009, 2:31 p.m.
On 09/09/2009 04:26 PM, Gleb Natapov wrote:
> On Wed, Sep 09, 2009 at 08:00:28AM -0500, Anthony Liguori wrote:
>    
>> Bernhard Kauer wrote:
>>      
>>> Its wednesday again, time to resend a patch to the list.
>>> Until today the following happened in this endless story:
>>>        
>> Really, the whining just makes me want to drop your patch..
>>
>> The first patch you sent didn't have [PATCH] in the subject.  There
>> are hundreds of mails that are sent to the list every day.  If you
>> don't mark a patch in a way that allows automatic filtering, it will
>> get dropped.
>>
>> When you resent, you did include [PATCH].  That patch made it into
>> my staging tree.  See http://repo.or.cz/w/qemu/aliguori-queue.git.
>> We had a holiday this past weekend so I'm a little slow in flushing
>> that queue to master.
>>
>>      
> It would be really nice to get mail that your patch was applied to staging.
> Casual contributor don't know that there is staging tree that should be
> checked or separate mailing list that he should subscribe too.
>    

Also, if the queue can be a branch of the main repository, a 'git fetch' 
will get both the master branch and the queue.
Amit Shah - Sept. 10, 2009, 7:03 a.m.
On (Wed) Sep 09 2009 [08:00:28], Anthony Liguori wrote:
> Bernhard Kauer wrote:
>> Its wednesday again, time to resend a patch to the list.
>> Until today the following happened in this endless story:
>>   
>
> Really, the whining just makes me want to drop your patch..

Let's be courteous and not drive away contributors.

There's no relation between accepting patches and some nudging on the
contributor's part especially when there's no feedback on patches;
positive or negative.

If there'd be a daemon sending a mail saying the patch is in some
staging queue it'll reduce everyone's effort. Such extra mails
definitely aren't a problem. If it's later reverted because of any kind
of failure, again a polite mail wouldn't hurt.

		Amit
Reimar Döffinger - Sept. 10, 2009, 7:56 a.m.
On Thu, Sep 10, 2009 at 12:33:36PM +0530, Amit Shah wrote:
> On (Wed) Sep 09 2009 [08:00:28], Anthony Liguori wrote:
> > Bernhard Kauer wrote:
> >> Its wednesday again, time to resend a patch to the list.
> >> Until today the following happened in this endless story:
> >>   
> >
> > Really, the whining just makes me want to drop your patch..
> 
> Let's be courteous and not drive away contributors.
> 
> There's no relation between accepting patches and some nudging on the
> contributor's part especially when there's no feedback on patches;
> positive or negative.
> 
> If there'd be a daemon sending a mail saying the patch is in some
> staging queue it'll reduce everyone's effort. Such extra mails
> definitely aren't a problem. If it's later reverted because of any kind
> of failure, again a polite mail wouldn't hurt.

May I repeat my previous question: Is there some CODING file or
something that has some hints?
If not, would a patch to add such a file be welcome?
I have the impression that some rules for the Linux kernel development
apply, but I wouldn't know out of my head where to find the guide for
that.
(Things that should be in such a file, at least:
- Patches should be sent to this list
- Patches should have [PATCH] in subject
- Patches should be sent as a new thread even when they are in response
  to some other mail (not sure about that? I didn't really understand
  what that one response I got meant to say, I am just guessing. I also
  think it makes quite a mess for people just reading the list and not
  trying to apply the patches).
- Patches are preferred inline, even better created by git format-patch
  (not sure about that either)
- URL of staging tree
- Possibly coding style rules once decided (no "pointless" casts of void
  *)?
)
Amit Shah - Sept. 10, 2009, 10:08 a.m.
On (Thu) Sep 10 2009 [09:56:44], Reimar Döffinger wrote:
> May I repeat my previous question: Is there some CODING file or
> something that has some hints?

There is: CODIING_STYLE.

But that file talks about how to write qemu code.

> If not, would a patch to add such a file be welcome?

Sure.

> I have the impression that some rules for the Linux kernel development
> apply, but I wouldn't know out of my head where to find the guide for
> that.
> (Things that should be in such a file, at least:
> - Patches should be sent to this list

Yes

> - Patches should have [PATCH] in subject

Yes

> - Patches should be sent as a new thread even when they are in response
>   to some other mail (not sure about that? I didn't really understand
>   what that one response I got meant to say, I am just guessing. I also
>   think it makes quite a mess for people just reading the list and not
>   trying to apply the patches).

Right; that's what seems to be requested.

> - Patches are preferred inline, even better created by git format-patch
>   (not sure about that either)

This is right too

> - URL of staging tree

Might help; but various developers might have their own staging trees
(as it now is). If things change in the future, this will have to be
modified.

> - Possibly coding style rules once decided (no "pointless" casts of void
>   *)?

That should be covered in CODING_STYLE. The list you're talking about
makes sense for something like SUBMITTING_PATCHES or CONTRIBUTING.

		Amit
Luiz Capitulino - Sept. 10, 2009, 11:47 a.m.
On Thu, 10 Sep 2009 15:38:04 +0530
Amit Shah <amit.shah@redhat.com> wrote:

> > - URL of staging tree
> 
> Might help; but various developers might have their own staging trees
> (as it now is). If things change in the future, this will have to be
> modified.

 I think he's talking about Anthony's staging, which is the most
important one for those submitting patches.

 I might be wrong about this but, the Linux kernel way of having
people maintaining subsystems didn't work out here yet.

 This discussion makes me think that the subject should be
'QEMU development process'.
Amit Shah - Sept. 10, 2009, 12:01 p.m.
On (Thu) Sep 10 2009 [08:47:13], Luiz Capitulino wrote:
> On Thu, 10 Sep 2009 15:38:04 +0530
> Amit Shah <amit.shah@redhat.com> wrote:
> 
> > > - URL of staging tree
> > 
> > Might help; but various developers might have their own staging trees
> > (as it now is). If things change in the future, this will have to be
> > modified.
> 
>  I think he's talking about Anthony's staging, which is the most
> important one for those submitting patches.

Right; and what I mean is like Anthony has his own queue, others might
have theirs too. If something along the lines of what Avi suggested
(having branches in the qemu repo for staging) gets implemented, this
will have to be redone.

>  I might be wrong about this but, the Linux kernel way of having
> people maintaining subsystems didn't work out here yet.

It can easily be done. The process has to scale considering the number
of submissions we're seeing.

>  This discussion makes me think that the subject should be
> 'QEMU development process'.

		Amit
Luiz Capitulino - Sept. 10, 2009, 12:29 p.m.
On Thu, 10 Sep 2009 17:31:21 +0530
Amit Shah <amit.shah@redhat.com> wrote:

> On (Thu) Sep 10 2009 [08:47:13], Luiz Capitulino wrote:
> > On Thu, 10 Sep 2009 15:38:04 +0530
> > Amit Shah <amit.shah@redhat.com> wrote:
> > 
> > > > - URL of staging tree
> > > 
> > > Might help; but various developers might have their own staging trees
> > > (as it now is). If things change in the future, this will have to be
> > > modified.
> > 
> >  I think he's talking about Anthony's staging, which is the most
> > important one for those submitting patches.
> 
> Right; and what I mean is like Anthony has his own queue, others might
> have theirs too. If something along the lines of what Avi suggested
> (having branches in the qemu repo for staging) gets implemented, this
> will have to be redone.

 Yes, the point is: if we have a central tree where patches are
merged before going to master, the address of that tree should be
listed somewhere.

> >  I might be wrong about this but, the Linux kernel way of having
> > people maintaining subsystems didn't work out here yet.
> 
> It can easily be done. The process has to scale considering the number
> of submissions we're seeing.

 I agree, with more maintainers QEMU would evolve a lot faster and I think
this is becoming an issue.

 On the other hand I don't think we will have them by distributing
responsibilities, those interested should setup a tree and
start collecting patches.
Reimar Döffinger - Sept. 10, 2009, 12:51 p.m.
On Thu, Sep 10, 2009 at 09:29:38AM -0300, Luiz Capitulino wrote:
> On Thu, 10 Sep 2009 17:31:21 +0530
> Amit Shah <amit.shah@redhat.com> wrote:
> > It can easily be done. The process has to scale considering the number
> > of submissions we're seeing.
> 
>  I agree, with more maintainers QEMU would evolve a lot faster and I think
> this is becoming an issue.
> 
>  On the other hand I don't think we will have them by distributing
> responsibilities, those interested should setup a tree and
> start collecting patches.

You're saying that just like that, but if you just set up your own tree,
at least if you are not a git guru you end up with the same mess as I
have now:
The official qemu git tree, Stefan Weil's git tree and the kvm tree, all
have slightly different eepro100 code and for each one I have to do
merges since I have no idea which path has a chance to get patches
accepted (since after all I got no response anywhere).
For me that is simply too much effort (I am after all a FFmpeg/MPlayer developer
in the first place) and I'll probably just drop all the non-trivial
changes I made because they have a too high merging effort.

Greetings,
Reimar Döffinger
Luiz Capitulino - Sept. 10, 2009, 1:11 p.m.
On Thu, 10 Sep 2009 14:51:09 +0200
Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:

> On Thu, Sep 10, 2009 at 09:29:38AM -0300, Luiz Capitulino wrote:
> > On Thu, 10 Sep 2009 17:31:21 +0530
> > Amit Shah <amit.shah@redhat.com> wrote:
> > > It can easily be done. The process has to scale considering the number
> > > of submissions we're seeing.
> > 
> >  I agree, with more maintainers QEMU would evolve a lot faster and I think
> > this is becoming an issue.
> > 
> >  On the other hand I don't think we will have them by distributing
> > responsibilities, those interested should setup a tree and
> > start collecting patches.
> 
> You're saying that just like that, but if you just set up your own tree,
> at least if you are not a git guru you end up with the same mess as I
> have now:
> The official qemu git tree, Stefan Weil's git tree and the kvm tree, all
> have slightly different eepro100 code and for each one I have to do
> merges since I have no idea which path has a chance to get patches
> accepted (since after all I got no response anywhere).

 This is a sightly different problem that is related to the fact
that qemu is forked.

> For me that is simply too much effort (I am after all a FFmpeg/MPlayer developer
> in the first place) and I'll probably just drop all the non-trivial
> changes I made because they have a too high merging effort.

 Not sure if this is what you meant but, having different trees for
different subsystems is one of the best ways to get large scale
development and that's how git was designed to work.
Anthony Liguori - Sept. 10, 2009, 1:56 p.m.
Amit Shah wrote:
> On (Wed) Sep 09 2009 [08:00:28], Anthony Liguori wrote:
>   
>> Bernhard Kauer wrote:
>>     
>>> Its wednesday again, time to resend a patch to the list.
>>> Until today the following happened in this endless story:
>>>   
>>>       
>> Really, the whining just makes me want to drop your patch..
>>     
>
> Let's be courteous and not drive away contributors.
>
> There's no relation between accepting patches and some nudging on the
> contributor's part especially when there's no feedback on patches;
> positive or negative.
>
> If there'd be a daemon sending a mail saying the patch is in some
> staging queue it'll reduce everyone's effort. Such extra mails
> definitely aren't a problem. If it's later reverted because of any kind
> of failure, again a polite mail wouldn't hurt.
>   

The problem is patch volume.  We often see hundreds of patches a day.  
If typing a mail for each patch takes 2 minutes, that's potentially 
hours spent just on sending these mails.

What I really need is some way to automatically generate these 
notifications.  It's pretty easy to send a mail when a patch enters the 
queue but it's more difficult to send a mail when a patch is removed 
from the queue via a rebase.  Often times, I remove patches from the 
queue simply because I'm not the right path for the patches to be 
committed from (like linux-user).

Usually, if I remove a patch from the queue because something is wrong 
with it, I send out an email explaining what is wrong.

Regards,

Anthony Liguori

> 		Amit
>
Anthony Liguori - Sept. 10, 2009, 1:58 p.m.
Luiz Capitulino wrote:
> On Thu, 10 Sep 2009 15:38:04 +0530
> Amit Shah <amit.shah@redhat.com> wrote:
>
>   
>>> - URL of staging tree
>>>       
>> Might help; but various developers might have their own staging trees
>> (as it now is). If things change in the future, this will have to be
>> modified.
>>     
>
>  I think he's talking about Anthony's staging, which is the most
> important one for those submitting patches.
>
>  I might be wrong about this but, the Linux kernel way of having
> people maintaining subsystems didn't work out here yet.
>   

I believe this is the long term solution but I do think our previous 
attempt wasn't successful.  As we do a better job isolating the 
subsystems and people get more familiar with the internal bits of QEMU, 
I believe we'll naturally migrate to this.

Regards,

Anthony Liguori
Amit Shah - Sept. 10, 2009, 2:04 p.m.
On (Thu) Sep 10 2009 [08:56:34], Anthony Liguori wrote:
>>
>> If there'd be a daemon sending a mail saying the patch is in some
>> staging queue it'll reduce everyone's effort. Such extra mails
>> definitely aren't a problem. If it's later reverted because of any kind
>> of failure, again a polite mail wouldn't hurt.
>>   
>
> The problem is patch volume.  We often see hundreds of patches a day.   
> If typing a mail for each patch takes 2 minutes, that's potentially  
> hours spent just on sending these mails.

How about a bot that goes through your mailbox (or your git queue) and
sends the email? One reply-to-all email for each unique hash you have
against qemu-master?

> What I really need is some way to automatically generate these  
> notifications.  It's pretty easy to send a mail when a patch enters the  
> queue but it's more difficult to send a mail when a patch is removed  
> from the queue via a rebase.  Often times, I remove patches from the  
> queue simply because I'm not the right path for the patches to be  
> committed from (like linux-user).

... and store the commit ids and subject in some file. When you rebase
and that particular commit-id + subject combination is not present, an
auto-email can be triggered

> Usually, if I remove a patch from the queue because something is wrong  
> with it, I send out an email explaining what is wrong.

... but this is simpler and more efficient also because you can give the
reason along with the email rather than people mailing you later asking
why the patch was dropped.

		Amit
Anthony Liguori - Sept. 10, 2009, 2:12 p.m.
Amit Shah wrote:
> On (Thu) Sep 10 2009 [08:56:34], Anthony Liguori wrote:
>   
>>> If there'd be a daemon sending a mail saying the patch is in some
>>> staging queue it'll reduce everyone's effort. Such extra mails
>>> definitely aren't a problem. If it's later reverted because of any kind
>>> of failure, again a polite mail wouldn't hurt.
>>>   
>>>       
>> The problem is patch volume.  We often see hundreds of patches a day.   
>> If typing a mail for each patch takes 2 minutes, that's potentially  
>> hours spent just on sending these mails.
>>     
>
> How about a bot that goes through your mailbox (or your git queue) and
> sends the email? One reply-to-all email for each unique hash you have
> against qemu-master?
>   

Yeah, it's easy to do that for initial commit.

Git lacks the hooks to do it for when you drop something via git rebase 
though.  A few weeks ago, a bunch of commits slipped through with 
Message-Id tags.  That was my attempt to add some smarts to git to allow 
for this but it didn't work out the way I wanted it to.


> ... and store the commit ids and subject in some file. When you rebase
> and that particular commit-id + subject combination is not present, an
> auto-email can be triggered
>   

Yeah, I tried that without a lot of success.  Basically, my work flow is 
the following:

1) Mail client filters things that look like patches into a folder
2) I do a once through to remove obvious dups or non-patches
3) I have some scripts that pull patches into an mbox format.  They are 
smart about identifying threads and trying to find a sane sort order.
4) I have more scripts that let me split the mbox into smaller mboxes.  
This is because git am is not very forgiving and having to do a git am 
--abort can lead to a lot of wasted effort if the mbox has 100+ patches
5) I git am the smaller mboxes into staging
6) I push to staging
7) I do a build check, and fix errors, and push to staging
8) I start the testing cycle, removing patches based on testing results 
and pushing to staging
9) I manually review each patch that's survived testing
10) I do final build/test, then push to master

>> Usually, if I remove a patch from the queue because something is wrong  
>> with it, I send out an email explaining what is wrong.
>>     
>
> ... but this is simpler and more efficient also because you can give the
> reason along with the email rather than people mailing you later asking
> why the patch was dropped.
>   

And commit mails are sent whenever something goes to master.  So someone 
should get a mail when a patch goes to master or when I reject it.  To 
me, the area to optimize is reducing the time things are spent in 
staging.  I don't think people really need visibility into staging.

Regards,

Anthony Liguori
Avi Kivity - Sept. 10, 2009, 2:38 p.m.
On 09/10/2009 04:56 PM, Anthony Liguori wrote:
>
> The problem is patch volume.  We often see hundreds of patches a day.  
> If typing a mail for each patch takes 2 minutes, that's potentially 
> hours spent just on sending these mails.
>

You exaggerate.  The average rate is 13 patches per calendar day.  The 
bulk of the patches are in patchsets which can be acked as a set, not 
once per patch.

> What I really need is some way to automatically generate these 
> notifications.  It's pretty easy to send a mail when a patch enters 
> the queue but it's more difficult to send a mail when a patch is 
> removed from the queue via a rebase.  Often times, I remove patches 
> from the queue simply because I'm not the right path for the patches 
> to be committed from (like linux-user).

I think more per-patch attention is needed, not less, for example see 
this commit:

commit e09a5267adf0af25b55d2abaf06e288b2d9537ea
Author: Dustin Kirkland <kirkland@canonical.com>
Date:   Thu Sep 3 12:31:33 2009 -0500

     qemu-kvm: fix segfault when running kvm without /dev/kvm, falling 
back to non-accelerated mode

     qemu-kvm: fix segfault when running kvm without /dev/kvm, falling back
     to non-accelerated mode

     We're seeing segfaults on systems without access to /dev/kvm.  It
     looks like the global kvm_allowed is being set just a little too late
     in vl.c.  This patch moves the kvm initialization a bit higher in the
     vl.c main, just after options processing, and solves the segfaults.
     We're carrying this patch in Ubuntu 9.10 Alpha.  Please apply
     upstream, or advise if and why this might not be the optimal solution.

     Signed-off-by: Dustin Kirkland <kirkland@canonical.com>

     Move the kvm_init() call a bit higher to fix a segfault when
     /dev/kvm is not available.  The kvm_allowed global needs
     to be set correctly a little earlier.

     Signed-off-by: Dustin Kirkland <kirkland@canonical.com>
     Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

There are many examples like this in the tree which is a pity.  Others 
include parts of an email conversation.  I'd like history to look better 
than this.
Amit Shah - Sept. 10, 2009, 3:15 p.m.
On (Thu) Sep 10 2009 [09:12:21], Anthony Liguori wrote:
>
> Yeah, I tried that without a lot of success.  Basically, my work flow is  
> the following:
>
> 1) Mail client filters things that look like patches into a folder
> 2) I do a once through to remove obvious dups or non-patches
> 3) I have some scripts that pull patches into an mbox format.  They are  
> smart about identifying threads and trying to find a sane sort order.
> 4) I have more scripts that let me split the mbox into smaller mboxes.   
> This is because git am is not very forgiving and having to do a git am  
> --abort can lead to a lot of wasted effort if the mbox has 100+ patches
> 5) I git am the smaller mboxes into staging
> 6) I push to staging
> 7) I do a build check, and fix errors, and push to staging
> 8) I start the testing cycle, removing patches based on testing results  
> and pushing to staging
> 9) I manually review each patch that's survived testing
> 10) I do final build/test, then push to master

Obviously there's a lot of manual intervention and can get you
overloaded. But if some developer knows that his patch is in some queue
or getting some consideration, he'll rest easier than to check if his
patch has made it either to master or to some staging queue somewhere.
If it can't be automated, it sadly has to drop back to manual acking.

> And commit mails are sent whenever something goes to master.  So someone  
> should get a mail when a patch goes to master or when I reject it.  To  
> me, the area to optimize is reducing the time things are spent in  
> staging.  I don't think people really need visibility into staging.

Absolutely. I think the trend has been anywhere from 2 to 4 weeks for
patches landing on the list to the master tree. Also, this results in
delays for important patches to get to the master tree because they're
delayed behind a huge pile of other patches. Avi has mentioned this
before but I don't know if there was any plan to address it.

		Amit
Anthony Liguori - Sept. 10, 2009, 3:54 p.m.
Avi Kivity wrote:
> On 09/10/2009 04:56 PM, Anthony Liguori wrote:
>>
>> The problem is patch volume.  We often see hundreds of patches a 
>> day.  If typing a mail for each patch takes 2 minutes, that's 
>> potentially hours spent just on sending these mails.
>>
>
> You exaggerate.  The average rate is 13 patches per calendar day.

On list or committed?  There are a lot more on list than 13..

> commit e09a5267adf0af25b55d2abaf06e288b2d9537ea
> Author: Dustin Kirkland <kirkland@canonical.com>
> Date:   Thu Sep 3 12:31:33 2009 -0500
>
>     qemu-kvm: fix segfault when running kvm without /dev/kvm, falling 
> back to non-accelerated mode
>
>     qemu-kvm: fix segfault when running kvm without /dev/kvm, falling 
> back
>     to non-accelerated mode
>
>     We're seeing segfaults on systems without access to /dev/kvm.  It
>     looks like the global kvm_allowed is being set just a little too late
>     in vl.c.  This patch moves the kvm initialization a bit higher in the
>     vl.c main, just after options processing, and solves the segfaults.
>     We're carrying this patch in Ubuntu 9.10 Alpha.  Please apply
>     upstream, or advise if and why this might not be the optimal 
> solution.
>
>     Signed-off-by: Dustin Kirkland <kirkland@canonical.com>
>
>     Move the kvm_init() call a bit higher to fix a segfault when
>     /dev/kvm is not available.  The kvm_allowed global needs
>     to be set correctly a little earlier.
>
>     Signed-off-by: Dustin Kirkland <kirkland@canonical.com>
>     Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>
> There are many examples like this in the tree which is a pity.  Others 
> include parts of an email conversation.  I'd like history to look 
> better than this.

This is goofy and is caused by improper patch submission.  But when 
people quote email threads in a commit message, I don't remove them.  It 
don't see it as a problem.

Regards,

Anthony Liguori
Anthony Liguori - Sept. 10, 2009, 4:05 p.m.
Avi Kivity wrote:
> On 09/10/2009 04:56 PM, Anthony Liguori wrote:
>>
>> The problem is patch volume.  We often see hundreds of patches a 
>> day.  If typing a mail for each patch takes 2 minutes, that's 
>> potentially hours spent just on sending these mails.
>>
>
> You exaggerate.  The average rate is 13 patches per calendar day.
FYI, in the past 14 hours, there were 92 patches posted to the list.

Regards,

Anthony Liguori
Avi Kivity - Sept. 10, 2009, 4:09 p.m.
On 09/10/2009 06:54 PM, Anthony Liguori wrote:
> Avi Kivity wrote:
>> On 09/10/2009 04:56 PM, Anthony Liguori wrote:
>>>
>>> The problem is patch volume.  We often see hundreds of patches a 
>>> day.  If typing a mail for each patch takes 2 minutes, that's 
>>> potentially hours spent just on sending these mails.
>>>
>>
>> You exaggerate.  The average rate is 13 patches per calendar day.
>
> On list or committed?  There are a lot more on list than 13..

You certainly shouldn't ack patches you don't commit!

>
>> commit e09a5267adf0af25b55d2abaf06e288b2d9537ea
>> Author: Dustin Kirkland <kirkland@canonical.com>
>> Date:   Thu Sep 3 12:31:33 2009 -0500
>>
>>     qemu-kvm: fix segfault when running kvm without /dev/kvm, falling 
>> back to non-accelerated mode
>>
>>     qemu-kvm: fix segfault when running kvm without /dev/kvm, falling 
>> back
>>     to non-accelerated mode
>>
>>     We're seeing segfaults on systems without access to /dev/kvm.  It
>>     looks like the global kvm_allowed is being set just a little too 
>> late
>>     in vl.c.  This patch moves the kvm initialization a bit higher in 
>> the
>>     vl.c main, just after options processing, and solves the segfaults.
>>     We're carrying this patch in Ubuntu 9.10 Alpha.  Please apply
>>     upstream, or advise if and why this might not be the optimal 
>> solution.
>>
>>     Signed-off-by: Dustin Kirkland <kirkland@canonical.com>
>>
>>     Move the kvm_init() call a bit higher to fix a segfault when
>>     /dev/kvm is not available.  The kvm_allowed global needs
>>     to be set correctly a little earlier.
>>
>>     Signed-off-by: Dustin Kirkland <kirkland@canonical.com>
>>     Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>>
>> There are many examples like this in the tree which is a pity.  
>> Others include parts of an email conversation.  I'd like history to 
>> look better than this.
>
> This is goofy and is caused by improper patch submission.  But when 
> people quote email threads in a commit message, I don't remove them.  
> It don't see it as a problem.

 From long experience, most commit messages need to be edited.  People 
rarely write commit messages that can be understood a year later, and 
they don't know how 'git am' works.
Avi Kivity - Sept. 10, 2009, 4:14 p.m.
On 09/10/2009 07:05 PM, Anthony Liguori wrote:
> Avi Kivity wrote:
>> On 09/10/2009 04:56 PM, Anthony Liguori wrote:
>>>
>>> The problem is patch volume.  We often see hundreds of patches a 
>>> day.  If typing a mail for each patch takes 2 minutes, that's 
>>> potentially hours spent just on sending these mails.
>>>
>>
>> You exaggerate.  The average rate is 13 patches per calendar day.
> FYI, in the past 14 hours, there were 92 patches posted to the list.

Only 4 or 5 patchsets though.  It's not that much effort to ack when 
committing.  And of course, the average is way lower and I expect it to 
drop as the code base matures.
Anthony Liguori - Sept. 10, 2009, 4:22 p.m.
Avi Kivity wrote:
> On 09/10/2009 06:54 PM, Anthony Liguori wrote:
>> Avi Kivity wrote:
>>> On 09/10/2009 04:56 PM, Anthony Liguori wrote:
>>>>
>>>> The problem is patch volume.  We often see hundreds of patches a 
>>>> day.  If typing a mail for each patch takes 2 minutes, that's 
>>>> potentially hours spent just on sending these mails.
>>>>
>>>
>>> You exaggerate.  The average rate is 13 patches per calendar day.
>>
>> On list or committed?  There are a lot more on list than 13..
>
> You certainly shouldn't ack patches you don't commit!

But most spend time in staging.

Acking patches that go to master, that's perfectly fine to do.  The 
qemu-commits list does that and should CC the author directly so this 
should be happening.  It's acking things that go into staging that I 
think would be difficult and not necessarily productive.

>> This is goofy and is caused by improper patch submission.  But when 
>> people quote email threads in a commit message, I don't remove them.  
>> It don't see it as a problem.
>
> From long experience, most commit messages need to be edited.  People 
> rarely write commit messages that can be understood a year later, and 
> they don't know how 'git am' works.

I don't like editing patches.  I think it's unfair to the submitter to 
change their patch underneath of them.  I'd suggest providing feedback 
on the list to people who write bad commit messages and ask them to 
write better ones.  I try to limit the changes I make to resolving merge 
conflicts.

Regards,

Anthony Liguori
Avi Kivity - Sept. 10, 2009, 4:35 p.m.
On 09/10/2009 07:22 PM, Anthony Liguori wrote:
>> You certainly shouldn't ack patches you don't commit!
>
>
> But most spend time in staging.

What's the percentage of patches that make it to master?  For me it's 
 >90%.  If it's too low we nned to fix that.

>
> Acking patches that go to master, that's perfectly fine to do. 

I think that's too late, especially as it often takes a week for master 
to be pushed.  To a submitter, an ack means "no further action is 
required from you at this time" and it's good to provide it as early as 
possible.

> The qemu-commits list does that and should CC the author directly so 
> this should be happening. 

It's too late, and doesn't help others who have an interest in the patch.

> It's acking things that go into staging that I think would be 
> difficult and not necessarily productive.

That is what I do and it doesn't seem to be troublesome.  If I drop a 
patch from a queue I explicitly unack it.

>
>>> This is goofy and is caused by improper patch submission.  But when 
>>> people quote email threads in a commit message, I don't remove 
>>> them.  It don't see it as a problem.
>>
>> From long experience, most commit messages need to be edited.  People 
>> rarely write commit messages that can be understood a year later, and 
>> they don't know how 'git am' works.
>
> I don't like editing patches.  I think it's unfair to the submitter to 
> change their patch underneath of them.  I'd suggest providing feedback 
> on the list to people who write bad commit messages and ask them to 
> write better ones.  I try to limit the changes I make to resolving 
> merge conflicts.

Editing the commit log is not changing the patch.  I doubt you'll be 
able to get better commit messages - submitters have more immediate 
perspectives than maintainers (should have).  I always try to make the 
log make sense a year from now (the code may change, but the commit log 
won't).

Unfairly picking on Mark (who usually writes truly excellent changelogs, 
but this one is such a gem):

> Subject: [Qemu-devel] [PATCH 01/19] Suppress more more kraxelism
>
> Let's kick off this series with some of the more critical fixes.
>
> Signed-off-by: Mark McLoughlin<markmc@redhat.com>
>    

What would you be thinking hunting the commit log for some change and 
coming up with this?

(Mark, apologies for picking on you, it's truly unfair of me, but I 
can't help it)
Anthony Liguori - Sept. 10, 2009, 4:38 p.m.
Avi Kivity wrote:
> On 09/10/2009 07:22 PM, Anthony Liguori wrote:
>>> You certainly shouldn't ack patches you don't commit!
>>
>>
>> But most spend time in staging.
>
> What's the percentage of patches that make it to master?  For me it's 
> >90%.  If it's too low we nned to fix that.

Closer to 20% I'd say.  This is largely due to multiple versions of the 
same series.  If there's a way to improve this, that would make my life 
a lot easier.

>> I don't like editing patches.  I think it's unfair to the submitter 
>> to change their patch underneath of them.  I'd suggest providing 
>> feedback on the list to people who write bad commit messages and ask 
>> them to write better ones.  I try to limit the changes I make to 
>> resolving merge conflicts.
>
> Editing the commit log is not changing the patch.  I doubt you'll be 
> able to get better commit messages - submitters have more immediate 
> perspectives than maintainers (should have).  I always try to make the 
> log make sense a year from now (the code may change, but the commit 
> log won't).
>
> Unfairly picking on Mark (who usually writes truly excellent 
> changelogs, but this one is such a gem):
>
>> Subject: [Qemu-devel] [PATCH 01/19] Suppress more more kraxelism
>>
>> Let's kick off this series with some of the more critical fixes.
>>
>> Signed-off-by: Mark McLoughlin<markmc@redhat.com>
>>    
>
> What would you be thinking hunting the commit log for some change and 
> coming up with this?

Well the question is, should I/you edit this, or reject the patch 
requesting a better changelog?

Certainly, the later is what akpm often does.

Regards,

Anthony Liguori
Avi Kivity - Sept. 10, 2009, 4:46 p.m.
On 09/10/2009 07:38 PM, Anthony Liguori wrote:
> Avi Kivity wrote:
>> On 09/10/2009 07:22 PM, Anthony Liguori wrote:
>>>> You certainly shouldn't ack patches you don't commit!
>>>
>>>
>>> But most spend time in staging.
>>
>> What's the percentage of patches that make it to master?  For me it's 
>> >90%.  If it's too low we nned to fix that.
>
> Closer to 20% I'd say.  This is largely due to multiple versions of 
> the same series.  If there's a way to improve this, that would make my 
> life a lot easier.

Well, I'd say this is much more important than acking patches (though if 
you acked and unacked patches, it would be a lot more visible, and might 
shame the submitters into increased care).

Why are there multiple series?  If you have comments, clearly you 
wouldn't stage the patches.  If others have comments, then you can 
simply wait a few days before staging (this is what I do).  If testing 
fails, well, that's what testing is for.  If the submitter puts out a 
new version, well, no good answer to that.  Ask for an incremental?

> Well the question is, should I/you edit this, or reject the patch 
> requesting a better changelog?
>
> Certainly, the later is what akpm often does.

I'm happy to reject patches for whitespace but I will edit changelogs.  
My rationale is that many people don't care about that and I can't make 
them care; further the log is mostly for my own benefit - I spend quite 
a lot of time reading it when hunting regressions or preparing 
patchqueues for upstream.
Reimar Döffinger - Sept. 10, 2009, 5:19 p.m.
On Thu, Sep 10, 2009 at 07:46:23PM +0300, Avi Kivity wrote:
> On 09/10/2009 07:38 PM, Anthony Liguori wrote:
> > Well the question is, should I/you edit this, or reject the patch 
> > requesting a better changelog?
> >
> > Certainly, the later is what akpm often does.
> 
> I'm happy to reject patches for whitespace but I will edit changelogs.  
> My rationale is that many people don't care about that and I can't make 
> them care; further the log is mostly for my own benefit - I spend quite 
> a lot of time reading it when hunting regressions or preparing 
> patchqueues for upstream.

I'd also add that anyone used to projects using SVN will probably be
used to writing only a general explanation of the patch while the real
commit message is left to whoever commits it. Maybe they ideally should
be identical, but I expect that quite a few people _won't_ expect their
email to appear 1:1 in the repository as log message (I usually feel
quite embarrassed when my hastily written email by which I only wanted
to get first comments ends up in a project's permanent history).
I am sure it misses a lot of stuff and there might even be objections to
some parts, but I wrote this draft of a "PATCHES" (or "CONTRIBUTING" or
whatever) file that might help newcomers (and even some long-term
developers might not know all the unwritten rules ;-).
Suggested text:

This is a (very) rough guide on how to submit patches to qemu, what is expected
of you and what to expect from the process.
Patches should go to the qemu-devel@nongnu.org list, subscription is possible
via http://lists.nongnu.org/mailman/listinfo/qemu-devel
The subject for any emails containing patches should start with [PATCH] so it is
obvious that there is a patch included.
Whenever you send a new patch or a new version of a patch, you should start a new
thread - i.e. do _not_ reply to any email but write a new one.
Patches are preferred inline (i.e. not as attachments - but be careful your mailer
does not mangle them e.g. by adding line breaks).
Emails generated via "git format-patch" are even better.
Also be aware that emails are often used as-is as log messages, so try to write
your emails in a way that is suitable for this, in particular they should in an
understandable and not too verbose way describe what change was made and
why.
Also do not forget to add the appropriate Signed-off-by lines.
Do not expect an immediate reply to your patches, reacting to patches simply
takes some time. If there is no reaction and you checked that the patch was
not already applied (there currently is no notification about this) try sending
the patch once again, it might just have got lost or forgotten at some
point.
Reimar Döffinger - Sept. 10, 2009, 5:24 p.m.
On Thu, Sep 10, 2009 at 10:11:48AM -0300, Luiz Capitulino wrote:
> On Thu, 10 Sep 2009 14:51:09 +0200
> Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
> > For me that is simply too much effort (I am after all a FFmpeg/MPlayer developer
> > in the first place) and I'll probably just drop all the non-trivial
> > changes I made because they have a too high merging effort.
> 
>  Not sure if this is what you meant but, having different trees for
> different subsystems is one of the best ways to get large scale
> development and that's how git was designed to work.

My point was: If you tell people to just use their own repository to
"maintain" some part without actually having some appropriate "power" to
get things accepted and directing patches to that part "through" them,
then I think that instead of managing things by subsystems (good) you
end up with a "wilderness" of forks (very, very bad IMO).
Mark McLoughlin - Sept. 10, 2009, 6:29 p.m.
On Thu, 2009-09-10 at 19:35 +0300, Avi Kivity wrote:

> Unfairly picking on Mark (who usually writes truly excellent changelogs, 
> but this one is such a gem):
> 
> > Subject: [Qemu-devel] [PATCH 01/19] Suppress more more kraxelism
> >
> > Let's kick off this series with some of the more critical fixes.
> >
> > Signed-off-by: Mark McLoughlin<markmc@redhat.com>
> >    
> 
> What would you be thinking hunting the commit log for some change and 
> coming up with this?
> 
> (Mark, apologies for picking on you, it's truly unfair of me, but I 
> can't help it)

As you say, I normally try very hard with my changelogs, but I don't
think the odd joke hurts much.

Look back over the changelog to a similar, but even more concise commit
message, a couple of weeks back that didn't go through Anthony. I was
parodying that one :-)

Anyway, you've cc-ed me now, so IMHO:

  - If one is so inclined, improving a commit message before pushing is 
    perfectly fine

  - A simple "Applied, thanks" would be hugely appreciated

  - This "apply everything, test at length, reject problematic patches" 
    appears to lead to a very batchy patch flow; there's a trade off to 
    be made between trying to catch every regression before it hits the
    tree and the delay that effort introduces before the tree gets more
    widely tested by others

Cheers,
Mark.
Avi Kivity - Sept. 10, 2009, 6:40 p.m.
On 09/10/2009 09:29 PM, Mark McLoughlin wrote:
> On Thu, 2009-09-10 at 19:35 +0300, Avi Kivity wrote:
>
>    
>> Unfairly picking on Mark (who usually writes truly excellent changelogs,
>> but this one is such a gem):
>>
>>      
>>> Subject: [Qemu-devel] [PATCH 01/19] Suppress more more kraxelism
>>>
>>> Let's kick off this series with some of the more critical fixes.
>>>
>>> Signed-off-by: Mark McLoughlin<markmc@redhat.com>
>>>
>>>        
>> What would you be thinking hunting the commit log for some change and
>> coming up with this?
>>
>> (Mark, apologies for picking on you, it's truly unfair of me, but I
>> can't help it)
>>      
> As you say, I normally try very hard with my changelogs, but I don't
> think the odd joke hurts much.
>    

Jokes are fine, but the commit log is sacred.


>    - This "apply everything, test at length, reject problematic patches"
>      appears to lead to a very batchy patch flow; there's a trade off to
>      be made between trying to catch every regression before it hits the
>      tree and the delay that effort introduces before the tree gets more
>      widely tested by others
>    

For qemu.git I'd agree since it's undergoing a lot of churn.  
Unfortunately it also feeds qemu-kvm.git which I try very hard to keep 
regression-free (and finding and fixing regressions during a merge is 
quite horrible), so I'd really appreciate it if qemu.git quality didn't 
deteriorate.

(and we're quite far from catching every regression btw).

Anthony, how long are your test cycles?
Reimar Döffinger - Sept. 10, 2009, 6:59 p.m.
On Thu, Sep 10, 2009 at 07:29:56PM +0100, Mark McLoughlin wrote:
> On Thu, 2009-09-10 at 19:35 +0300, Avi Kivity wrote:
> > Unfairly picking on Mark (who usually writes truly excellent changelogs, 
> > but this one is such a gem):
> > 
> > > Subject: [Qemu-devel] [PATCH 01/19] Suppress more more kraxelism
> > >
> > > Let's kick off this series with some of the more critical fixes.
> > >
> > > Signed-off-by: Mark McLoughlin<markmc@redhat.com>
> > >    
> > 
> > What would you be thinking hunting the commit log for some change and 
> > coming up with this?
> > 
> > (Mark, apologies for picking on you, it's truly unfair of me, but I 
> > can't help it)
> 
> As you say, I normally try very hard with my changelogs, but I don't
> think the odd joke hurts much.

I don't mind jokes, but when I read that message I had not the slightest
idea what it might do, so in that case it hurts exactly as much as "." as
commit message hurts and not one iota less IMHO (note that I am not
necessarily advocating to leave jokes out, but at least they should come
with an explanation. Though considering an international community it
might be better to avoid jokes in log messages completely).
I'd also like to point out that the reference to "this series" is
another reason I don't like it when log messages are created from emails
unedited, such references are (probably sometimes) useful when sending
a patch series but become completely meaningless in a log message since
it is impossible to find out what it refers to.
Anthony Liguori - Sept. 10, 2009, 7:31 p.m.
Avi Kivity wrote:
> For qemu.git I'd agree since it's undergoing a lot of churn.  
> Unfortunately it also feeds qemu-kvm.git which I try very hard to keep 
> regression-free (and finding and fixing regressions during a merge is 
> quite horrible), so I'd really appreciate it if qemu.git quality 
> didn't deteriorate.

Or more accurately, you'd prefer if there were no bugs in qemu so that 
you only had to deal with the bugs that were introduced in qemu-kvm.

That would be nice for you of course :-)  But it's unrealistic to 
compare the two. 

$ git log --since='1 Month ago' --no-merges qemu-kvm/master 
--committer='Avi Kivity' --pretty=format:'%an' | wc -l
5
$ git log --since='1 Month ago' --no-merges qemu-kvm/master 
--committer='Marcelo Tosatti' --pretty=format:'%an' | wc -l
5

$ git log --since='1 Month ago' --no-merges origin/master 
--committer='Anthony Liguori' --pretty=format:'%an' | wc -l
251

So there are going to be at least 25x more regressions introduced from 
upstream qemu than what are introduced in qemu-kvm.

> (and we're quite far from catching every regression btw).
>
> Anthony, how long are your test cycles?

Depends on the number of regressions.  I can usually get through testing 
in 4-5 hours when everything works.  Everything usually doesn't work though.

Regards,

Anthony Liguori
Mark McLoughlin - Sept. 10, 2009, 8:36 p.m.
On Thu, 2009-09-10 at 21:40 +0300, Avi Kivity wrote:

> (and we're quite far from catching every regression btw).

That's kind of my point. A lot of regressions are only found after
they've been pushed. Delaying a push delays finding those regressions.

Don't get me wrong - we certainly want to avoid regressions and doing
some testing before pushing is a good idea, but there is a balance to be
struck.

It's also the case that not all patches are equal. It should be possible
to short-cut the process for small patches, or regression fixes, or
patches from a trusted author who has done significant testing already.

Cheers,
Mark.
Gerd Hoffmann - Sept. 11, 2009, 7:06 a.m.
Hi,

> Closer to 20% I'd say. This is largely due to multiple versions of the
> same series. If there's a way to improve this, that would make my life a
> lot easier.

I think you should do smaller batches more frequently, i.e. commit 20 
patches every day instead of >100 patches each week.

Advantages:
   * Patches don't hang around in staging forever, which makes
     everybody happy.
   * Patch ack-ing via qemu-commits works reasonable.
   * Reduce your workload: You can ask patch submitters to deal
     with merge conflicts then without making the merge process
     unreasonable slow.  Just drop anything which doesn't apply
     as-is, ask the submitter to rebase+resend, pick up the fresh
     patches the next day.
   * I'd suspect we'll also have less patch conflicts in the first
     place then.

Disadvantages:
   * Might be more testing work (how automated is this btw?).

>>> Subject: [Qemu-devel] [PATCH 01/19] Suppress more more kraxelism
>>>
>>> Let's kick off this series with some of the more critical fixes.
>>>
>>> Signed-off-by: Mark McLoughlin<markmc@redhat.com>
>>
>> What would you be thinking hunting the commit log for some change and
>> coming up with this?

I like that one, but it is a insider gag for qemu-commits readers and 
thus indeed not very useful when you'll read it a year or two in the 
future ...

cheers,
   Gerd
Jan Kiszka - Sept. 11, 2009, 9:16 a.m.
Avi Kivity wrote:
> On 09/10/2009 04:56 PM, Anthony Liguori wrote:
>>
>> The problem is patch volume.  We often see hundreds of patches a day. 
>> If typing a mail for each patch takes 2 minutes, that's potentially
>> hours spent just on sending these mails.
>>
> 
> You exaggerate.  The average rate is 13 patches per calendar day.  The
> bulk of the patches are in patchsets which can be acked as a set, not
> once per patch.
> 
>> What I really need is some way to automatically generate these
>> notifications.  It's pretty easy to send a mail when a patch enters
>> the queue but it's more difficult to send a mail when a patch is
>> removed from the queue via a rebase.  Often times, I remove patches
>> from the queue simply because I'm not the right path for the patches
>> to be committed from (like linux-user).
> 
> I think more per-patch attention is needed, not less, for example see
> this commit:
> 
> commit e09a5267adf0af25b55d2abaf06e288b2d9537ea
> Author: Dustin Kirkland <kirkland@canonical.com>
> Date:   Thu Sep 3 12:31:33 2009 -0500
> 
>     qemu-kvm: fix segfault when running kvm without /dev/kvm, falling
> back to non-accelerated mode
> 
>     qemu-kvm: fix segfault when running kvm without /dev/kvm, falling back
>     to non-accelerated mode
> 
>     We're seeing segfaults on systems without access to /dev/kvm.  It
>     looks like the global kvm_allowed is being set just a little too late
>     in vl.c.  This patch moves the kvm initialization a bit higher in the
>     vl.c main, just after options processing, and solves the segfaults.
>     We're carrying this patch in Ubuntu 9.10 Alpha.  Please apply
>     upstream, or advise if and why this might not be the optimal solution.
> 
>     Signed-off-by: Dustin Kirkland <kirkland@canonical.com>
> 
>     Move the kvm_init() call a bit higher to fix a segfault when
>     /dev/kvm is not available.  The kvm_allowed global needs
>     to be set correctly a little earlier.
> 
>     Signed-off-by: Dustin Kirkland <kirkland@canonical.com>
>     Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> There are many examples like this in the tree which is a pity.  Others
> include parts of an email conversation.  I'd like history to look better
> than this.

Even worse, I think this patch does not belong into upstream as it fixed
a qemu-kvm-only bug. I think this was caused by Dustin CC'ing qemu, right?

Did anyone test properly if the change has no side effects on upstream
kvm (which has a different initialization scheme)?

Jan
Amit Shah - Sept. 11, 2009, 12:39 p.m.
On (Thu) Sep 10 2009 [19:19:03], Reimar Döffinger wrote:
> 
> I'd also add that anyone used to projects using SVN will probably be
> used to writing only a general explanation of the patch while the real
> commit message is left to whoever commits it. Maybe they ideally should
> be identical, but I expect that quite a few people _won't_ expect their
> email to appear 1:1 in the repository as log message (I usually feel
> quite embarrassed when my hastily written email by which I only wanted
> to get first comments ends up in a project's permanent history).

So perhaps your "SUBMITTING_PATCHES" file should mention that the
patch descriptions should be as good as possible, and mention:

- what's being fixed
- why it's being fixed the way it is
- how does it solve the problem that's being fixed

all of these reduce the load on maintainers and reviewers.

If you took time out to write a patch and better some piece of code, you
should take the time to tell why you spent so much time on it.

> I am sure it misses a lot of stuff and there might even be objections to
> some parts, but I wrote this draft of a "PATCHES" (or "CONTRIBUTING" or
> whatever) file that might help newcomers (and even some long-term
> developers might not know all the unwritten rules ;-).
> Suggested text:

One general comment: Please include line breaks between paragraphs. That
makes things much easier to read.

> This is a (very) rough guide on how to submit patches to qemu, what is expected
> of you and what to expect from the process.
> Patches should go to the qemu-devel@nongnu.org list, subscription is possible
> via http://lists.nongnu.org/mailman/listinfo/qemu-devel
> The subject for any emails containing patches should start with [PATCH] so it is
> obvious that there is a patch included.
> Whenever you send a new patch or a new version of a patch, you should start a new
> thread - i.e. do _not_ reply to any email but write a new one.
> Patches are preferred inline (i.e. not as attachments - but be careful your mailer
> does not mangle them e.g. by adding line breaks).
> Emails generated via "git format-patch" are even better.
> Also be aware that emails are often used as-is as log messages, so try to write
> your emails in a way that is suitable for this, in particular they should in an
> understandable and not too verbose way describe what change was made and
> why.

You could add the points I mentioned above here.

> Also do not forget to add the appropriate Signed-off-by lines.

An example here wouldn't hurt.

> Do not expect an immediate reply to your patches, reacting to patches simply
> takes some time. If there is no reaction and you checked that the patch was
> not already applied (there currently is no notification about this) try sending
> the patch once again, it might just have got lost or forgotten at some
> point.

Please mention the commits-list where notifications are sent when
patches get applied to the master branch of the git tree.

Also mention Anthony's staging queue URL if someone really wants to know
if their patch has been picked up by a bot for testing.

In addition to the linefeed comment I gave above, the above list could
be separated by bullets (since it is a list).

Also, please make the text fit in 80 columns.

And send it as a patch so that it's easy to apply. :-)

		Amit
Anthony Liguori - Sept. 11, 2009, 12:56 p.m.
Jan Kiszka wrote:
> Avi Kivity wrote:
>   
>> On 09/10/2009 04:56 PM, Anthony Liguori wrote:
>>     
>>> The problem is patch volume.  We often see hundreds of patches a day. 
>>> If typing a mail for each patch takes 2 minutes, that's potentially
>>> hours spent just on sending these mails.
>>>
>>>       
>> You exaggerate.  The average rate is 13 patches per calendar day.  The
>> bulk of the patches are in patchsets which can be acked as a set, not
>> once per patch.
>>
>>     
>>> What I really need is some way to automatically generate these
>>> notifications.  It's pretty easy to send a mail when a patch enters
>>> the queue but it's more difficult to send a mail when a patch is
>>> removed from the queue via a rebase.  Often times, I remove patches
>>> from the queue simply because I'm not the right path for the patches
>>> to be committed from (like linux-user).
>>>       
>> I think more per-patch attention is needed, not less, for example see
>> this commit:
>>
>> commit e09a5267adf0af25b55d2abaf06e288b2d9537ea
>> Author: Dustin Kirkland <kirkland@canonical.com>
>> Date:   Thu Sep 3 12:31:33 2009 -0500
>>
>>     qemu-kvm: fix segfault when running kvm without /dev/kvm, falling
>> back to non-accelerated mode
>>
>>     qemu-kvm: fix segfault when running kvm without /dev/kvm, falling back
>>     to non-accelerated mode
>>
>>     We're seeing segfaults on systems without access to /dev/kvm.  It
>>     looks like the global kvm_allowed is being set just a little too late
>>     in vl.c.  This patch moves the kvm initialization a bit higher in the
>>     vl.c main, just after options processing, and solves the segfaults.
>>     We're carrying this patch in Ubuntu 9.10 Alpha.  Please apply
>>     upstream, or advise if and why this might not be the optimal solution.
>>
>>     Signed-off-by: Dustin Kirkland <kirkland@canonical.com>
>>
>>     Move the kvm_init() call a bit higher to fix a segfault when
>>     /dev/kvm is not available.  The kvm_allowed global needs
>>     to be set correctly a little earlier.
>>
>>     Signed-off-by: Dustin Kirkland <kirkland@canonical.com>
>>     Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>>
>> There are many examples like this in the tree which is a pity.  Others
>> include parts of an email conversation.  I'd like history to look better
>> than this.
>>     
>
> Even worse, I think this patch does not belong into upstream as it fixed
> a qemu-kvm-only bug. I think this was caused by Dustin CC'ing qemu, right?
>
> Did anyone test properly if the change has no side effects on upstream
> kvm (which has a different initialization scheme)?
>   

It doesn't break upstream qemu's -enable-kvm.

Regards,

Anthony Liguori
Jan Kiszka - Sept. 11, 2009, 1:04 p.m.
Anthony Liguori wrote:
> Jan Kiszka wrote:
>> Avi Kivity wrote:
>>   
>>> On 09/10/2009 04:56 PM, Anthony Liguori wrote:
>>>     
>>>> The problem is patch volume.  We often see hundreds of patches a day. 
>>>> If typing a mail for each patch takes 2 minutes, that's potentially
>>>> hours spent just on sending these mails.
>>>>
>>>>       
>>> You exaggerate.  The average rate is 13 patches per calendar day.  The
>>> bulk of the patches are in patchsets which can be acked as a set, not
>>> once per patch.
>>>
>>>     
>>>> What I really need is some way to automatically generate these
>>>> notifications.  It's pretty easy to send a mail when a patch enters
>>>> the queue but it's more difficult to send a mail when a patch is
>>>> removed from the queue via a rebase.  Often times, I remove patches
>>>> from the queue simply because I'm not the right path for the patches
>>>> to be committed from (like linux-user).
>>>>       
>>> I think more per-patch attention is needed, not less, for example see
>>> this commit:
>>>
>>> commit e09a5267adf0af25b55d2abaf06e288b2d9537ea
>>> Author: Dustin Kirkland <kirkland@canonical.com>
>>> Date:   Thu Sep 3 12:31:33 2009 -0500
>>>
>>>     qemu-kvm: fix segfault when running kvm without /dev/kvm, falling
>>> back to non-accelerated mode
>>>
>>>     qemu-kvm: fix segfault when running kvm without /dev/kvm, falling back
>>>     to non-accelerated mode
>>>
>>>     We're seeing segfaults on systems without access to /dev/kvm.  It
>>>     looks like the global kvm_allowed is being set just a little too late
>>>     in vl.c.  This patch moves the kvm initialization a bit higher in the
>>>     vl.c main, just after options processing, and solves the segfaults.
>>>     We're carrying this patch in Ubuntu 9.10 Alpha.  Please apply
>>>     upstream, or advise if and why this might not be the optimal solution.
>>>
>>>     Signed-off-by: Dustin Kirkland <kirkland@canonical.com>
>>>
>>>     Move the kvm_init() call a bit higher to fix a segfault when
>>>     /dev/kvm is not available.  The kvm_allowed global needs
>>>     to be set correctly a little earlier.
>>>
>>>     Signed-off-by: Dustin Kirkland <kirkland@canonical.com>
>>>     Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>>>
>>> There are many examples like this in the tree which is a pity.  Others
>>> include parts of an email conversation.  I'd like history to look better
>>> than this.
>>>     
>> Even worse, I think this patch does not belong into upstream as it fixed
>> a qemu-kvm-only bug. I think this was caused by Dustin CC'ing qemu, right?
>>
>> Did anyone test properly if the change has no side effects on upstream
>> kvm (which has a different initialization scheme)?
>>   
> 
> It doesn't break upstream qemu's -enable-kvm.

Yep, that's what I can confirm now too - after unbreaking -enable-kvm again.

Jan
Blue Swirl - Sept. 12, 2009, 5:55 a.m.
On Thu, Sep 10, 2009 at 8:19 PM, Reimar Döffinger
<Reimar.Doeffinger@gmx.de> wrote:
> On Thu, Sep 10, 2009 at 07:46:23PM +0300, Avi Kivity wrote:
>> On 09/10/2009 07:38 PM, Anthony Liguori wrote:
>> > Well the question is, should I/you edit this, or reject the patch
>> > requesting a better changelog?
>> >
>> > Certainly, the later is what akpm often does.
>>
>> I'm happy to reject patches for whitespace but I will edit changelogs.
>> My rationale is that many people don't care about that and I can't make
>> them care; further the log is mostly for my own benefit - I spend quite
>> a lot of time reading it when hunting regressions or preparing
>> patchqueues for upstream.
>
> I'd also add that anyone used to projects using SVN will probably be
> used to writing only a general explanation of the patch while the real
> commit message is left to whoever commits it. Maybe they ideally should
> be identical, but I expect that quite a few people _won't_ expect their
> email to appear 1:1 in the repository as log message (I usually feel
> quite embarrassed when my hastily written email by which I only wanted
> to get first comments ends up in a project's permanent history).
> I am sure it misses a lot of stuff and there might even be objections to
> some parts, but I wrote this draft of a "PATCHES" (or "CONTRIBUTING" or
> whatever) file that might help newcomers (and even some long-term
> developers might not know all the unwritten rules ;-).
> Suggested text:
>
> This is a (very) rough guide on how to submit patches to qemu, what is expected
> of you and what to expect from the process.
> Patches should go to the qemu-devel@nongnu.org list, subscription is possible
> via http://lists.nongnu.org/mailman/listinfo/qemu-devel
> The subject for any emails containing patches should start with [PATCH] so it is
> obvious that there is a patch included.
> Whenever you send a new patch or a new version of a patch, you should start a new
> thread - i.e. do _not_ reply to any email but write a new one.
> Patches are preferred inline (i.e. not as attachments - but be careful your mailer
> does not mangle them e.g. by adding line breaks).
> Emails generated via "git format-patch" are even better.
> Also be aware that emails are often used as-is as log messages, so try to write
> your emails in a way that is suitable for this, in particular they should in an
> understandable and not too verbose way describe what change was made and
> why.
> Also do not forget to add the appropriate Signed-off-by lines.
> Do not expect an immediate reply to your patches, reacting to patches simply
> takes some time. If there is no reaction and you checked that the patch was
> not already applied (there currently is no notification about this) try sending
> the patch once again, it might just have got lost or forgotten at some
> point.

We had a discussion about this earlier this year:
http://lists.gnu.org/archive/html/qemu-devel/2009-04/msg01184.html

Since that we have switched to git, so a lot of it could be
simplified. Instead of the lengthy formatting specification, we could
just require that the patch should apply with "git-am" to git HEAD
without any editing, merging or even apply.whitespace=fix.
Avi Kivity - Sept. 13, 2009, 3:44 p.m.
On 09/12/2009 08:55 AM, Blue Swirl wrote:
> We had a discussion about this earlier this year:
> http://lists.gnu.org/archive/html/qemu-devel/2009-04/msg01184.html
>
> Since that we have switched to git, so a lot of it could be
> simplified. Instead of the lengthy formatting specification, we could
> just require that the patch should apply with "git-am" to git HEAD
> without any editing, merging or even apply.whitespace=fix.
>    

That doesn't tell people what information to include, and if they don't 
care how the log looks like, they'll do the minimum amount of work to 
make git am like it.
Avi Kivity - Sept. 13, 2009, 3:49 p.m.
On 09/10/2009 10:31 PM, Anthony Liguori wrote:
> Avi Kivity wrote:
>> For qemu.git I'd agree since it's undergoing a lot of churn.  
>> Unfortunately it also feeds qemu-kvm.git which I try very hard to 
>> keep regression-free (and finding and fixing regressions during a 
>> merge is quite horrible), so I'd really appreciate it if qemu.git 
>> quality didn't deteriorate.
>
> Or more accurately, you'd prefer if there were no bugs in qemu so that 
> you only had to deal with the bugs that were introduced in qemu-kvm.

Yes.  Anyone who pulls qemu.git and develops or uses it would agree.

>
> That would be nice for you of course :-)  But it's unrealistic to 
> compare the two.
> $ git log --since='1 Month ago' --no-merges qemu-kvm/master 
> --committer='Avi Kivity' --pretty=format:'%an' | wc -l
> 5
> $ git log --since='1 Month ago' --no-merges qemu-kvm/master 
> --committer='Marcelo Tosatti' --pretty=format:'%an' | wc -l
> 5
>
> $ git log --since='1 Month ago' --no-merges origin/master 
> --committer='Anthony Liguori' --pretty=format:'%an' | wc -l
> 251
>
> So there are going to be at least 25x more regressions introduced from 
> upstream qemu than what are introduced in qemu-kvm.

Well, if we define a regression as something that blocks me from pushing 
(kvm-autotest), then both numbers are zero.  Of course qemu.git is not 
run purely for my enjoyment, but a large number of patches are targeted 
at qemu-kvm.git.  Further, the tests that I run (installing and booting 
some popular OSes) are really the basic minimum functionality, nothing 
fancy there.

>
>> (and we're quite far from catching every regression btw).
>>
>> Anthony, how long are your test cycles?
>
> Depends on the number of regressions.  I can usually get through 
> testing in 4-5 hours when everything works.  Everything usually 
> doesn't work though.

We definitely need to improve this and the 80% reject rate.  Can you 
start being a lot noisier about rejects?  that will at least give some 
visibility into the problem.
Avi Kivity - Sept. 13, 2009, 4:19 p.m.
On 09/10/2009 11:36 PM, Mark McLoughlin wrote:
> On Thu, 2009-09-10 at 21:40 +0300, Avi Kivity wrote:
>
>    
>> (and we're quite far from catching every regression btw).
>>      
> That's kind of my point. A lot of regressions are only found after
> they've been pushed. Delaying a push delays finding those regressions.
>    

That makes sense if the regression tests don't find any regressions.  If 
they do, then pushing despite those regressions means everyone is now 
hitting the regressions, swearing, and trying to work around them.  We 
get massive parallelism but little progress.

> Don't get me wrong - we certainly want to avoid regressions and doing
> some testing before pushing is a good idea, but there is a balance to be
> struck.
>
> It's also the case that not all patches are equal. It should be possible
> to short-cut the process for small patches, or regression fixes, or
> patches from a trusted author who has done significant testing already.
>    

I think reducing the batch size will also help.  If the batch contains 
100 patches there's a high likelihood of a regression in there.  If 
you're testing 10-15 patches at a time it may actually work, and if not, 
you can easily guess who the offender is.
Blue Swirl - Sept. 13, 2009, 4:30 p.m.
On Sun, Sep 13, 2009 at 6:44 PM, Avi Kivity <avi@redhat.com> wrote:
> On 09/12/2009 08:55 AM, Blue Swirl wrote:
>>
>> We had a discussion about this earlier this year:
>> http://lists.gnu.org/archive/html/qemu-devel/2009-04/msg01184.html
>>
>> Since that we have switched to git, so a lot of it could be
>> simplified. Instead of the lengthy formatting specification, we could
>> just require that the patch should apply with "git-am" to git HEAD
>> without any editing, merging or even apply.whitespace=fix.
>>
>
> That doesn't tell people what information to include, and if they don't care
> how the log looks like, they'll do the minimum amount of work to make git am
> like it.

1. Format of e-mail subject
[clip]
SP1.3: The summary must be suitable for changelog as is (start with a
capital letter etc.) after deleting the bracketed text, white space
after it and any text preceding it
[clip]
2. Format of e-mail body
[clip]
SP2.2: The patch description must be selfstanding (not depend on other
patches, other messages or background discussion)

What information should people include? Description of the problem,
step-by-step description how to reproduce it and the solution for the
problem? Brief analysis of the alternative solutions considered so
that if the commit is found buggy, corrective action is easier because
we can try one of the alternatives instead of reverting? Link to
hardware manual chapter where the correct behaviour is described?
Mark McLoughlin - Sept. 14, 2009, 7:49 a.m.
On Sun, 2009-09-13 at 19:19 +0300, Avi Kivity wrote:
> On 09/10/2009 11:36 PM, Mark McLoughlin wrote:
> > On Thu, 2009-09-10 at 21:40 +0300, Avi Kivity wrote:
> >
> >    
> >> (and we're quite far from catching every regression btw).
> >>      
> > That's kind of my point. A lot of regressions are only found after
> > they've been pushed. Delaying a push delays finding those regressions.
> >    
> 
> That makes sense if the regression tests don't find any regressions.

Or if the regression tests take too long - rather than regression tests
that take 5 hours to run, I'd prefer to see a subset of those which
takes e.g. 30 minutes be used to sanity check a tree before pushing it.

This would allow smaller batches to be pushed more regularly, while the
more in-depth testing can be done less regularly on batches not much
larger than are being pushed now.

> If 
> they do, then pushing despite those regressions means everyone is now 
> hitting the regressions, swearing, and trying to work around them.  We 
> get massive parallelism but little progress.

Not all regressions are equal. I wouldn't mind some regressions in the
tree so long as they are being tracked as "must be fixed" before the
release.

Clearly anything causing people to swear should be reverted. Assuming
you can tell which patch caused the problem.

> > Don't get me wrong - we certainly want to avoid regressions and doing
> > some testing before pushing is a good idea, but there is a balance to be
> > struck.
> >
> > It's also the case that not all patches are equal. It should be possible
> > to short-cut the process for small patches, or regression fixes, or
> > patches from a trusted author who has done significant testing already.
> >    
> 
> I think reducing the batch size will also help.  If the batch contains 
> 100 patches there's a high likelihood of a regression in there.  If 
> you're testing 10-15 patches at a time it may actually work, and if not, 
> you can easily guess who the offender is.

The batch size is determined by the length of time the testing takes,
right?

But agree, with a large batch, you're more likely to find at least one
regression, to struggle to figure out which patch caused the regression
and to go through several iterations before you have something to push.

Cheers,
Mark.
Avi Kivity - Sept. 14, 2009, 7:59 a.m.
On 09/14/2009 10:49 AM, Mark McLoughlin wrote:
> On Sun, 2009-09-13 at 19:19 +0300, Avi Kivity wrote:
>    
>> On 09/10/2009 11:36 PM, Mark McLoughlin wrote:
>>      
>>> On Thu, 2009-09-10 at 21:40 +0300, Avi Kivity wrote:
>>>
>>>
>>>        
>>>> (and we're quite far from catching every regression btw).
>>>>
>>>>          
>>> That's kind of my point. A lot of regressions are only found after
>>> they've been pushed. Delaying a push delays finding those regressions.
>>>
>>>        
>> That makes sense if the regression tests don't find any regressions.
>>      
> Or if the regression tests take too long - rather than regression tests
> that take 5 hours to run, I'd prefer to see a subset of those which
> takes e.g. 30 minutes be used to sanity check a tree before pushing it.
>    

I'm guessing the reason the tests take so long is lack of automation.  
My kvm-autotest takes 1.5-2 hours, and it runs everything serially.  
Once it starts running tests in parallel, latency can probably be 
reduces to 30 minutes on a fairly standard 8-core.

>> If
>> they do, then pushing despite those regressions means everyone is now
>> hitting the regressions, swearing, and trying to work around them.  We
>> get massive parallelism but little progress.
>>      
> Not all regressions are equal. I wouldn't mind some regressions in the
> tree so long as they are being tracked as "must be fixed" before the
> release.
>
> Clearly anything causing people to swear should be reverted. Assuming
> you can tell which patch caused the problem.
>    

Right, a regression that breaks some esoteric feature used by two people 
is not too bad.  Of course, if we can detect it it's better to drop the 
patch and make the submitter fix it.

The regressions I'm talking about are "common OS won't boot" things.

>> I think reducing the batch size will also help.  If the batch contains
>> 100 patches there's a high likelihood of a regression in there.  If
>> you're testing 10-15 patches at a time it may actually work, and if not,
>> you can easily guess who the offender is.
>>      
> The batch size is determined by the length of time the testing takes,
> right?
>
> But agree, with a large batch, you're more likely to find at least one
> regression, to struggle to figure out which patch caused the regression
> and to go through several iterations before you have something to push.
>    

Exactly.  Then I undergo exactly the same nightmare when merging into 
qemu-kvm.  Even worse, fixes I apply during a merge are often of 
substandard quality and cause more regressions.

Patch

diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index 2022548..2b040a7 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -421,9 +421,10 @@  static void rtc_update_second2(void *opaque)
     }

     /* update ended interrupt */
+    s->cmos_data[RTC_REG_C] |= REG_C_UF;
     if (s->cmos_data[RTC_REG_B] & REG_B_UIE) {
-        s->cmos_data[RTC_REG_C] |= 0x90;
-        rtc_irq_raise(s->irq);
+      s->cmos_data[RTC_REG_C] |= REG_C_IRQF;
+      rtc_irq_raise(s->irq);
     }

     /* clear update in progress bit */