Anything else anyone can think of? Any objections to any of these?
I based them off of Linus's original list.
thanks,
greg k-h
------
Rules on what kind of patches are accepted, and what ones are not, into
the "linux-release" tree.
- It can not bigger than 100 lines, with context.
- It must fix only one thing.
- It must fix a real bug that bothers people (not a, "This could be a
problem..." type thing.)
- It must fix a problem that causes a build error (but not for things
marked CONFIG_BROKEN), an oops, a hang, or a real security issue.
- No "theoretical race condition" issues, unless an explanation of how
the race can be exploited.
- It can not contain any "trivial" fixes in it (spelling changes,
whitespace cleanups, etc.)
Greg KH wrote:
> Anything else anyone can think of? Any objections to any of these?
> I based them off of Linus's original list.
Must already be in Linus tree (i.e. 2.6.X+1)?
--
========================================================================
Ian Pilcher [email protected]
========================================================================
On Fri, 2005-03-04 at 23:08 -0600, Ian Pilcher wrote:
> Greg KH wrote:
> > Anything else anyone can think of? Any objections to any of these?
> > I based them off of Linus's original list.
>
> Must already be in Linus tree (i.e. 2.6.X+1)?
No, it's cleaner in bitkeeper terms for the patches to be pulled into
the linux-releases tree first, and then Linus pulls from that. Linus
has said that that is what he intends to do.
Shaggy
--
David Kleikamp
IBM Linux Technology Center
On Fri, 04 Mar 2005 23:08:55 CST, Ian Pilcher said:
> Greg KH wrote:
> > Anything else anyone can think of? Any objections to any of these?
> > I based them off of Linus's original list.
>
> Must already be in Linus tree (i.e. 2.6.X+1)?
Not workable. There's a high probability that we hit a bug where Linus
commits a more extensive "correct" solution, but 2.6.X.1 includes a much
simpler band-aid that's technically bogus, but stops a panic-on-boot bug
from manifesting.
Greg KH <[email protected]> writes:
> - It must fix a problem that causes a build error (but not for things
> marked CONFIG_BROKEN), an oops, a hang, or a real security issue.
So a trivial patch that fixed a data corruption issue wouldn't be
accepted?
--
Adam Sampson <[email protected]> <http://offog.org/>
On Fri, Mar 04, 2005 at 02:21:46PM -0800, Greg KH wrote:
> Anything else anyone can think of? Any objections to any of these?
> I based them off of Linus's original list.
>
> thanks,
>
> greg k-h
>
> ------
>
> Rules on what kind of patches are accepted, and what ones are not, into
> the "linux-release" tree.
>
> - It can not bigger than 100 lines, with context.
> - It must fix only one thing.
> - It must fix a real bug that bothers people (not a, "This could be a
> problem..." type thing.)
> - It must fix a problem that causes a build error (but not for things
> marked CONFIG_BROKEN), an oops, a hang, or a real security issue.
> - No "theoretical race condition" issues, unless an explanation of how
> the race can be exploited.
> - It can not contain any "trivial" fixes in it (spelling changes,
> whitespace cleanups, etc.)
Objections - no. Anything else - yes.
I would like the requirement: "It must be obviously correct".
In a hundred lines one can put a lot of tricky code and subtle changes.
For example, if a security problem necessitates a nontrivial change,
it should cause an earlier release of 2.6.x+1 instead of a 2.6.x.y+1.
Andries
On Saturday 05 March 2005 00:08, Ian Pilcher wrote:
> Greg KH wrote:
> > Anything else anyone can think of? Any objections to any of these?
> > I based them off of Linus's original list.
>
> Must already be in Linus tree (i.e. 2.6.X+1)?
How about must be logicily fixed in the Linus tree - Linus does not have to have the
same patch...
Ed
On Fri, Mar 04, 2005 at 02:21:46PM -0800, Greg KH wrote:
> Anything else anyone can think of? Any objections to any of these?
> I based them off of Linus's original list.
Are these 100% fixed rules or just guidelines you use?
An example that doesn't fit:
A patch of me to remove an unused function was accepted into 2.6.11 .
Today, someone mailed that there's an external GPL'ed module that uses
this function.
A patch to re-add this function as it was in 2.6.10 does not fulfill
your criteria, but it is a low-risk way to fix a regression compared to
2.6.10 .
> thanks,
>
> greg k-h
>
> ------
>
> Rules on what kind of patches are accepted, and what ones are not, into
> the "linux-release" tree.
>
> - It can not bigger than 100 lines, with context.
> - It must fix only one thing.
> - It must fix a real bug that bothers people (not a, "This could be a
> problem..." type thing.)
> - It must fix a problem that causes a build error (but not for things
> marked CONFIG_BROKEN), an oops, a hang, or a real security issue.
> - No "theoretical race condition" issues, unless an explanation of how
> the race can be exploited.
> - It can not contain any "trivial" fixes in it (spelling changes,
> whitespace cleanups, etc.)
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
On Sat, Mar 05, 2005 at 11:43:05AM +0100, Andries Brouwer wrote:
> On Fri, Mar 04, 2005 at 02:21:46PM -0800, Greg KH wrote:
> Objections - no. Anything else - yes.
> I would like the requirement: "It must be obviously correct".
Doh, forgot that one, I'll go add it.
thanks,
greg k-h
On Sat, Mar 05, 2005 at 02:59:17PM +0100, Adrian Bunk wrote:
> On Fri, Mar 04, 2005 at 02:21:46PM -0800, Greg KH wrote:
>
> > Anything else anyone can think of? Any objections to any of these?
> > I based them off of Linus's original list.
>
> Are these 100% fixed rules or just guidelines you use?
Guidelines of course :)
> An example that doesn't fit:
>
> A patch of me to remove an unused function was accepted into 2.6.11 .
> Today, someone mailed that there's an external GPL'ed module that uses
> this function.
>
> A patch to re-add this function as it was in 2.6.10 does not fulfill
> your criteria, but it is a low-risk way to fix a regression compared to
> 2.6.10 .
Yes, I wouldn't have a problem with adding this kind of fix. Do others
disagree?
thanks,
greg k-h
On Sat, Mar 05, 2005 at 09:58:00AM +0000, Adam Sampson wrote:
> Greg KH <[email protected]> writes:
>
> > - It must fix a problem that causes a build error (but not for things
> > marked CONFIG_BROKEN), an oops, a hang, or a real security issue.
>
> So a trivial patch that fixed a data corruption issue wouldn't be
> accepted?
Good point, I've now added that.
thanks,
greg k-h
Adam Sampson wrote:
> Greg KH <[email protected]> writes:
>
>
>> - It must fix a problem that causes a build error (but not for things
>> marked CONFIG_BROKEN), an oops, a hang, or a real security issue.
>
>
> So a trivial patch that fixed a data corruption issue wouldn't be
> accepted?
>
That's called a critical patch.
--
~Randy
Greg KH wrote:
> On Sat, Mar 05, 2005 at 02:59:17PM +0100, Adrian Bunk wrote:
>>An example that doesn't fit:
>>
>>A patch of me to remove an unused function was accepted into 2.6.11 .
>>Today, someone mailed that there's an external GPL'ed module that uses
>>this function.
>>
>>A patch to re-add this function as it was in 2.6.10 does not fulfill
>>your criteria, but it is a low-risk way to fix a regression compared to
>>2.6.10 .
>
>
> Yes, I wouldn't have a problem with adding this kind of fix. Do others
> disagree?
Depends. Is Linus going to push it back into his tree? If it's just
something like the remap_page_range going away, fix the the module
instead, I'd say.
Greg KH wrote:
> On Sat, Mar 05, 2005 at 02:59:17PM +0100, Adrian Bunk wrote:
>
>
>>An example that doesn't fit:
>>
>>A patch of me to remove an unused function was accepted into 2.6.11 .
>>Today, someone mailed that there's an external GPL'ed module that uses
>>this function.
>>
>>A patch to re-add this function as it was in 2.6.10 does not fulfill
>>your criteria, but it is a low-risk way to fix a regression compared to
>>2.6.10 .
>
>
> Yes, I wouldn't have a problem with adding this kind of fix. Do others
> disagree?
>
Something about major functional regressions?
--
========================================================================
Ian Pilcher [email protected]
========================================================================
On Fri, 4 Mar 2005, Greg KH wrote:
> Anything else anyone can think of? Any objections to any of these?
> I based them off of Linus's original list.
>
How about feature regressions? What made me think of this is the thread
about the 8x0 alsa breakage in 2.6.11. 2.6.10 and 2.6.9 worked well for
people, but 2.6.11 is aparently broken for some. When that bug gets fixed
would such a fix be appropriate for 2.6.10.y (assuming it doesn't violate
any of your other guidelines) ?
--
Jesper
On Fri, Mar 04, 2005 at 02:21:46PM -0800, Greg KH wrote:
> Anything else anyone can think of? Any objections to any of these?
Well, Linus's rules regarding signoff are good as well. I'd
suggest, once there is a set of $sucker-evaluators, we could use a good
rule like requiring 5 folks to signoff (Linus's original proposal), or
3 including the subsystem maintainer (who doesn't have to be part of
$sucker-evaluators) - whichever comes first. Any patch that affects an
area where it isn't easy to determine who is the authorative maintainer
just has to wait for the full 5 person signoff.
Joel
--
"Behind every successful man there's a lot of unsuccessful years."
- Bob Brown
Joel Becker
Senior Member of Technical Staff
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
On Fri, Mar 04, 2005 at 02:21:46PM -0800, Greg KH wrote:
> Anything else anyone can think of? Any objections to any of these?
> I based them off of Linus's original list.
>
> thanks,
>
> greg k-h
>
> ------
>
> Rules on what kind of patches are accepted, and what ones are not, into
> the "linux-release" tree.
>
> - It can not bigger than 100 lines, with context.
> - It must fix only one thing.
> - It must fix a real bug that bothers people (not a, "This could be a
> problem..." type thing.)
> - It must fix a problem that causes a build error (but not for things
> marked CONFIG_BROKEN), an oops, a hang, or a real security issue.
"and breakage of previously working functionality" ?
> - No "theoretical race condition" issues, unless an explanation of how
> the race can be exploited.
> - It can not contain any "trivial" fixes in it (spelling changes,
> whitespace cleanups, etc.)
On Sat, 05 Mar 2005 11:43:05 +0100, Andries Brouwer wrote:
> On Fri, Mar 04, 2005 at 02:21:46PM -0800, Greg KH wrote:
>
>> Anything else anyone can think of? Any objections to any of these?
>> I based them off of Linus's original list.
>>
>> thanks,
>>
>> greg k-h
>>
>> ------
>>
>> Rules on what kind of patches are accepted, and what ones are not, into
>> the "linux-release" tree.
>>
>> - It can not bigger than 100 lines, with context.
>> - It must fix only one thing.
>> - It must fix a real bug that bothers people (not a, "This could be a
>> problem..." type thing.)
An obvious fix is an obvious fix. It shouldn't matter whether people have
triggered a bug or not; why discriminate?
>> - It must fix a problem that causes a build
error (but not for things
>> marked CONFIG_BROKEN), an oops, a hang, or a real security issue.
>> - No "theoretical race condition" issues, unless an explanation of how
>> the race can be exploited.
I disagree w/ this; if it's an obvious fix, there should be no need for
this. Either it's a race that is clearly incorrect (after tracing through
the relevant code), or it's not.
>> - It can not contain any "trivial" fixes in it (spelling changes,
>> whitespace cleanups, etc.)
This and the "it must fix a problem" are basically saying the same thing.
>
> Objections - no. Anything else - yes. I would like the requirement: "It
> must be obviously correct".
>
It seems like this would be the primary thing that matters. The rest of
them (aside from the "fixes only one thing" and "no cleanups" rules)
overlap considerably.
> In a hundred lines one can put a lot of tricky code and subtle
changes.
> For example, if a security problem necessitates a nontrivial change, it
> should cause an earlier release of 2.6.x+1 instead of a 2.6.x.y+1.
>
> Andries
Andres Salomon <[email protected]> wrote:
> On Sat, 05 Mar 2005 11:43:05 +0100, Andries Brouwer wrote:
> > On Fri, Mar 04, 2005 at 02:21:46PM -0800, Greg KH wrote:
> >> - It must fix a real bug that bothers people (not a, "This could be a
> >> problem..." type thing.)
>
> An obvious fix is an obvious fix. It shouldn't matter whether people have
> triggered a bug or not; why discriminate?
Because the sucker tree is purposely driven by real bug reports, not by
developers who happen across a theoretical problem while traversing the
code. If users aren't hitting it today, the fix can wait for 2.6.n+1.
> >> - It must fix a problem that causes a build error (but not for things
> >> marked CONFIG_BROKEN), an oops, a hang, or a real security issue.
> >> - No "theoretical race condition" issues, unless an explanation of how
> >> the race can be exploited.
>
> I disagree w/ this; if it's an obvious fix, there should be no need for
> this. Either it's a race that is clearly incorrect (after tracing through
> the relevant code), or it's not.
The sucker tree is not a dumping ground for every fix under the sun
(even obvious ones). It's for solving problems hit by real users, right
now.
> >> - It can not contain any "trivial" fixes in it (spelling changes,
> >> whitespace cleanups, etc.)
>
> This and the "it must fix a problem" are basically saying the same
> thing.
No. There's an important distinction and the key word is "contain". This
rule specifically forbids patches that do fix a real problem but _also_
contain unrelated trivial changes. See "setup_per_zone_lowmem_reserve()
oops fix" for an example of a patch that could theoretically be rejected
due to this rule.
--Adam
Andres wrote:
> An obvious fix is an obvious fix.
Perhaps in theory. But in practice, any fix bears some risk.
They have nothing against "obvious" fixes. But unless additional
criteria are also met, such fixes are for someone else to apply.
> >> - It can not contain any "trivial" fixes in it (spelling changes,
> >> whitespace cleanups, etc.)
>
> This and the "it must fix a problem" are basically saying the same thing.
Not at all. Let me put it this way.
If a change that fixes a problem is included in a patch with another
change that makes trivial changes (typo fix, say), the patch will
be rejected.
The statement:
"It must fix a problem and it must _not_ contain anything else,
such as 'trivial' fixes."
is _obviously_ not the same as:
"It must fix a problem."
(Notice how quickly even the obvious becomes unobvious ...;).
--
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <[email protected]> 1.650.933.1373, 1.925.600.0401
On Sun, 2005-03-06 at 23:50 -0800, Paul Jackson wrote:
> Andres wrote:
> > An obvious fix is an obvious fix.
>
> Perhaps in theory. But in practice, any fix bears some risk.
>
Of course; no one's arguing that (things that depend on broken behavior,
corner cases, etc). In practice, however; an obvious fix is still an
obvious fix. :)
What I mean is, if there's an unknown (ie, due to hardware behavior,
user input, etc), and it's not adding sanity checking or somehow
ensuring that the data it's dealing with is of a certain form, then it's
not really an obvious fix.
Of course, part of the problem is that "obvious" is subjective. I know
what *I* consider obvious; someone who works for a hardware company, and
has access to hardware, specifications, and errata would have different
criteria for what they consider obvious. Here's where that
signed-off-by-5-people thing comes into play.
> They have nothing against "obvious" fixes. But unless additional
> criteria are also met, such fixes are for someone else to apply.
>
That's fine; I agree w/ the additional criteria of "it must be a build,
oops, hang, or race fix".
> > >> - It can not contain any "trivial" fixes in it (spelling changes,
> > >> whitespace cleanups, etc.)
> >
> > This and the "it must fix a problem" are basically saying the same thing.
>
> Not at all. Let me put it this way.
>
> If a change that fixes a problem is included in a patch with another
> change that makes trivial changes (typo fix, say), the patch will
> be rejected.
>
That's fine as well; that's covered by the "it must only fix one thing"
rule, which I also agree w/. The "no trivial fixes" thing is still
redundant; a) a patch must contain only one fix, and b) a patch may only
fix a build error, oops, hang, or race.
> The statement:
>
> "It must fix a problem and it must _not_ contain anything else,
> such as 'trivial' fixes."
>
> is _obviously_ not the same as:
>
> "It must fix a problem."
>
> (Notice how quickly even the obvious becomes unobvious ...;).
>
--
Andres Salomon <[email protected]>
On Sun, 2005-03-06 at 15:10 -0500, Adam Kropelin wrote:
> Andres Salomon <[email protected]> wrote:
> > On Sat, 05 Mar 2005 11:43:05 +0100, Andries Brouwer wrote:
> > > On Fri, Mar 04, 2005 at 02:21:46PM -0800, Greg KH wrote:
> > >> - It must fix a real bug that bothers people (not a, "This could be a
> > >> problem..." type thing.)
> >
> > An obvious fix is an obvious fix. It shouldn't matter whether people have
> > triggered a bug or not; why discriminate?
>
> Because the sucker tree is purposely driven by real bug reports, not by
> developers who happen across a theoretical problem while traversing the
> code. If users aren't hitting it today, the fix can wait for 2.6.n+1.
>
Here's an example; if there's a theoretical integer under/overflow in
some part of the kernel, but no one is hitting it because (by chance,
not by design) there's no way for a user to stuff an incorrect value in
there.
Does it get fixed in 2.6.x.y? According to the above rule, it does not.
However, it may be the case where a third party patch end up modifying
things such that the value in the sign integer is now not properly
sanity checked (ignore any security issues for the moment; assume only
root can stuff an incorrect value in there). If it's a core function, a
third party module may end up calling it without checking the integer
value it's passing. So, it's not a problem in 2.6.x; it becomes a
problem at some later point, thanks to an external patch or module.
Why not just fix it? It still falls under the category of an obvious
fix; just because a user isn't triggering it now, doesn't mean they
won't be triggering it later. An argument could be made that this would
mean a lot of extra work for the Suckers, but it's only up to the point
at which the next 2.6.x kernel is released.
> > >> - It must fix a problem that causes a build error (but not for things
> > >> marked CONFIG_BROKEN), an oops, a hang, or a real security issue.
> > >> - No "theoretical race condition" issues, unless an explanation of how
> > >> the race can be exploited.
> >
> > I disagree w/ this; if it's an obvious fix, there should be no need for
> > this. Either it's a race that is clearly incorrect (after tracing through
> > the relevant code), or it's not.
>
> The sucker tree is not a dumping ground for every fix under the sun
> (even obvious ones). It's for solving problems hit by real users, right
> now.
>
I'm not saying fix every problem, but I would think that those that fix
a (potential) race, oops, hang, or security issue would be worth fixing.
But then, maybe I'm reading too much into this (as it's been stated
these are guidelines, not rules..)
> > >> - It can not contain any "trivial" fixes in it (spelling changes,
> > >> whitespace cleanups, etc.)
> >
> > This and the "it must fix a problem" are basically saying the same
> > thing.
>
> No. There's an important distinction and the key word is "contain". This
> rule specifically forbids patches that do fix a real problem but _also_
> contain unrelated trivial changes. See "setup_per_zone_lowmem_reserve()
> oops fix" for an example of a patch that could theoretically be rejected
> due to this rule.
Ah, yes.
--
Andres Salomon <[email protected]>
On Sun, Mar 06, 2005 at 06:44:51AM -0300, Marcelo Tosatti wrote:
> > - It must fix a problem that causes a build error (but not for things
> > marked CONFIG_BROKEN), an oops, a hang, or a real security issue.
>
> "and breakage of previously working functionality" ?
I'd vote to include this one...
John
--
John W. Linville
[email protected]