[2.6 patch] infiniband/hw/nes/nes_verbs.c: fix off-by-one

[2.6 patch] infiniband/hw/nes/nes_verbs.c: fix off-by-one

[Adrian Bunk]

This patch fixes an off-by-one spotted by the Coverity checker.

Signed-off-by: Adrian Bunk <bunk@kernel.org>

---

 drivers/infiniband/hw/nes/nes_verbs.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- linux-2.6/drivers/infiniband/hw/nes/nes_verbs.c.old	2008-02-20 00:20:47.000000000 +0200
+++ linux-2.6/drivers/infiniband/hw/nes/nes_verbs.c	2008-02-20 00:21:09.000000000 +0200
@@ -916,33 +916,33 @@ static struct ib_pd *nes_alloc_pd(struct
 	if (!nespd) {
 		nes_free_resource(nesadapter, nesadapter->allocated_pds, pd_num);
 		return ERR_PTR(-ENOMEM);
 	}
 
 	nes_debug(NES_DBG_PD, "Allocating PD (%p) for ib device %s\n",
 			nespd, nesvnic->nesibdev->ibdev.name);
 
 	nespd->pd_id = (pd_num << (PAGE_SHIFT-12)) + nesadapter->base_pd;
 
 	if (context) {
 		nesucontext = to_nesucontext(context);
 		nespd->mmap_db_index = find_next_zero_bit(nesucontext->allocated_doorbells,
 				NES_MAX_USER_DB_REGIONS, nesucontext->first_free_db);
 		nes_debug(NES_DBG_PD, "find_first_zero_biton doorbells returned %u, mapping pd_id %u.\n",
 				nespd->mmap_db_index, nespd->pd_id);
-		if (nespd->mmap_db_index > NES_MAX_USER_DB_REGIONS) {
+		if (nespd->mmap_db_index >= NES_MAX_USER_DB_REGIONS) {
 			nes_debug(NES_DBG_PD, "mmap_db_index > MAX\n");
 			nes_free_resource(nesadapter, nesadapter->allocated_pds, pd_num);
 			kfree(nespd);
 			return ERR_PTR(-ENOMEM);
 		}
 
 		uresp.pd_id = nespd->pd_id;
 		uresp.mmap_db_index = nespd->mmap_db_index;
 		if (ib_copy_to_udata(udata, &uresp, sizeof (struct nes_alloc_pd_resp))) {
 			nes_free_resource(nesadapter, nesadapter->allocated_pds, pd_num);
 			kfree(nespd);
 			return ERR_PTR(-EFAULT);
 		}
 
 		set_bit(nespd->mmap_db_index, nesucontext->allocated_doorbells);
 		nesucontext->mmap_db_index[nespd->mmap_db_index] = nespd->pd_id;

Re: [ofa-general] [2.6 patch] infiniband/hw/nes/nes_verbs.c: fix off-by-one

[Roland Dreier]

Thanks, this is already upstream as 51af33e8

Re: [ofa-general] [2.6 patch] infiniband/hw/nes/nes_verbs.c: fix off-by-one

[Adrian Bunk]

On Tue, Feb 19, 2008 at 08:23:19PM -0800, Roland Dreier wrote:
> Thanks, this is already upstream as 51af33e8

No, 51af33e8 was for a similar same bug 400 lines below this bug...

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

Re: [ofa-general] [2.6 patch] infiniband/hw/nes/nes_verbs.c: fix off-by-one

[Roland Dreier]

 > No, 51af33e8 was for a similar same bug 400 lines below this bug...

Heh, sorry.

Glenn -- please review Adrian's patches and let me know which ones are
good to apply.

RE: [ofa-general] [2.6 patch] infiniband/hw/nes/nes_verbs.c: fix off-by-one

[Glenn Streiff]

> -----Original Message-----
> From: Roland Dreier [mailto:rdreier@cisco.com]
> Sent: Wednesday, February 20, 2008 5:22 PM
> To: Adrian Bunk
> Cc: Faisal Latif; Glenn Streiff; linux-kernel@vger.kernel.org;
> general@lists.openfabrics.org
> Subject: Re: [ofa-general] [2.6 patch] infiniband/hw/nes/nes_verbs.c:
> fix off-by-one
> 
> 
>  > No, 51af33e8 was for a similar same bug 400 lines below this bug...
> 
> Heh, sorry.
> 
> Glenn -- please review Adrian's patches and let me know which ones are
> good to apply.
> 

Sweeping through them right now.  Should have something for you
tonight.

Glenn

RE: [ofa-general] [2.6 patch] infiniband/hw/nes/nes_verbs.c: fix off-by-one

[Glenn Streiff]

> >  > No, 51af33e8 was for a similar same bug 400 lines below 
> this bug...
> > 
> > Heh, sorry.
> > 
> > Glenn -- please review Adrian's patches and let me know 
> which ones are
> > good to apply.
> > 
> 

I went ahead and created a patch series and attributed Adrian
for the patches of his I liked.  There were a couple that
I tweaked.  Wasn't sure if all the hunks would apply nicely
after that if we mixed and matched his and mine, hence the series.

Hope that's okay.  Should I have gotten his ack for the ones
I rewrote?  The fixes were pretty small so I figured they didn't
really need more review.

The patch series is on the way...

Glenn

Re: [ofa-general] [2.6 patch] infiniband/hw/nes/nes_verbs.c: fix off-by-one

[Adrian Bunk]

On Thu, Feb 21, 2008 at 06:39:45AM -0600, Glenn Streiff wrote:
> 
> > >  > No, 51af33e8 was for a similar same bug 400 lines below 
> > this bug...
> > > 
> > > Heh, sorry.
> > > 
> > > Glenn -- please review Adrian's patches and let me know 
> > which ones are
> > > good to apply.
> > > 
> > 
> 
> I went ahead and created a patch series and attributed Adrian
> for the patches of his I liked.  There were a couple that
> I tweaked.  Wasn't sure if all the hunks would apply nicely
> after that if we mixed and matched his and mine, hence the series.
> 
> Hope that's okay.  Should I have gotten his ack for the ones
> I rewrote?  The fixes were pretty small so I figured they didn't
> really need more review.
>...

Looking at the patches what you did seems OK.


But regarding "review" I have a different criticism directed at Roland:

This driver should really have gotten some review before being included 
in the kernel.

Even a simple checkpatch run finds more than > 250 stylistic errors
(not code bugs but cases where the driver violates the standard code 
formatting rules of kernel code).

And I'm not talking about the > 2000 checkpatch warnings that are mostly 
about too long lines (which should arguably also be fixed).

And many more issues that could have been foung during a review.
E.g. when you look at 3/8 from this series the code
        if (!cm_node)
                return -EINVAL;
        new_send = kzalloc(sizeof(*new_send), GFP_ATOMIC);
        if (!new_send)
                return -1;
doesn't look good since the -1 should most likely better be something 
like -ENOMEM (I haven't checked whether you can immediately change it 
at this specific place).

And these are just comments from someone with zero knowledge about 
InfiniBand, but I'd expect InfiniBand-specifig bugs might be found 
before they hit users if an InfiniBand maintainer would review the 
complete driver.

Note that this is not meant as a criticism against Glenn - it's 
normal that submitted code contains bugs, but a code review can help to 
cope with this.

> Glenn

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

Re: [ofa-general] [2.6 patch] infiniband/hw/nes/nes_verbs.c: fix off-by-one

[Roland Dreier]

 > This driver should really have gotten some review before being included 
 > in the kernel.

 > Even a simple checkpatch run finds more than > 250 stylistic errors
 > (not code bugs but cases where the driver violates the standard code 
 > formatting rules of kernel code).

Linus has strongly stated that we should merge hardware drivers early,
and I agree: although the nes driver clearly needs more work, there's
no advantage to users with the hardware in forcing them to wait for
2.6.26 to merge the driver, since they'll just have to patch the
grungy code in themselves anyway.  And by merging the driver early, we
get fixed up for any tree-wide changes and allow janitors to help with
the cleanup.

(By the way, the code is not that pretty but it a lot closer to
upstream style than most driver submissions)

 > And these are just comments from someone with zero knowledge about 
 > InfiniBand, but I'd expect InfiniBand-specifig bugs might be found 
 > before they hit users if an InfiniBand maintainer would review the 
 > complete driver.

Just for the record, although this driver is under drivers/infiniband,
it is actually for a device that does iWARP/10 Gb ethernet.  At some
point we may want to rename drivers/infiniband to drivers/rdma, but so
far the churn hasn't seemed worth it for what is basically a cosmetic
issue.

 - R.

Merging of completely unreviewed drivers

[Adrian Bunk]

[ Linus Added to the To: since I want to hear his opinion on this issue. ]

On Thu, Feb 21, 2008 at 12:28:55PM -0800, Roland Dreier wrote:
>  > This driver should really have gotten some review before being included 
>  > in the kernel.
> 
>  > Even a simple checkpatch run finds more than > 250 stylistic errors
>  > (not code bugs but cases where the driver violates the standard code 
>  > formatting rules of kernel code).
> 
> Linus has strongly stated that we should merge hardware drivers early,
> and I agree: although the nes driver clearly needs more work, there's
> no advantage to users with the hardware in forcing them to wait for
> 2.6.26 to merge the driver, since they'll just have to patch the
> grungy code in themselves anyway.  And by merging the driver early, we
> get fixed up for any tree-wide changes and allow janitors to help with
> the cleanup.

Is it really intended to merge drivers without _any_ kind of review?

This driver even lacks a basic "please fix the > 250 checkpatch errors" [1]
and similar low hanging fruits that could easily be spotted and then 
fixed by the submitter within a short amount of time.

I see the point that it might make sense to not prevent the merging of 
drivers infinitely when they have some hard-to-fix issues, but was this 
really meant as an excuse for maintainers to no longer any review of 
what they merge at all?

> (By the way, the code is not that pretty but it a lot closer to
> upstream style than most driver submissions)
>...

There might be worse code being submitted, but when looking at what gets 
merged into Linus' tree this driver beats all other drivers I remember 
in both number of stylistic problems and bugs. [2]

>  - R.

cu
Adrian

BTW: Greg, you are Cc'ed for your joke in [3]...

[1] not to mention the > 2000 checkpatch warnings
[2] as already said, that's not meant against the driver submitter
    I'm complaining about the complete lack of review that would have 
    brought this driver into shape
[3] http://lkml.org/lkml/2008/2/12/427

-- 

       "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

Re: Merging of completely unreviewed drivers

[Roland Dreier]

 > Is it really intended to merge drivers without _any_ kind of review?
 > 
 > This driver even lacks a basic "please fix the > 250 checkpatch errors" [1]
 > and similar low hanging fruits that could easily be spotted and then 
 > fixed by the submitter within a short amount of time.

Just to be clear, this driver was reviewed.  Many issues were found,
and many were fixed while others are being worked on.

It's a judgement call when to merge things, but in this case given the
good engagement from the vendor, I didn't see anything to be gained by
delaying the merge.

 - R.

Re: Merging of completely unreviewed drivers

[Linus Torvalds]

On Thu, 21 Feb 2008, Adrian Bunk wrote:
> 
> Is it really intended to merge drivers without _any_ kind of review?

I'd really rather have the driver merged, and then *other* people can send 
patches!

The thing is, that's what merging really means - people can work on it 
sanely together. Before it's merged, it's a lot harder for people to work 
on it unless they are really serious about that driver, so before 
merging, the janitorial kind of things seldom happen.

So yes, I really do believe that we should merge drivers in particular a 
lot more aggressively. I'd like to see *testing* feedback, in order to not 
merge drivers that simply don't work well enough, but anything else? I 
suspect other feedback is as likely to cause problems as it is to fix 
things.

> This driver even lacks a basic "please fix the > 250 checkpatch errors" [1]
> and similar low hanging fruits that could easily be spotted and then 
> fixed by the submitter within a short amount of time.

Quite frankly, I've several times been *this* close (holds up fingers so 
you can't even see between them) to just remove checkpatch entirely.

I'm personally of the opinion that a lot of checkpatch "fixes" are 
anything but. That mainly concerns fixing overlong lines (where the 
"fixed" version is usually worse than the original), but it's been true 
for some other warnings too.

		Linus

Re: Merging of completely unreviewed drivers

[Alexey Dobriyan]

On Thu, Feb 21, 2008 at 01:14:55PM -0800, Linus Torvalds wrote:
> On Thu, 21 Feb 2008, Adrian Bunk wrote:
> > Is it really intended to merge drivers without _any_ kind of review?
> 
> I'd really rather have the driver merged, and then *other* people can send 
> patches!
> 
> The thing is, that's what merging really means - people can work on it 
> sanely together. Before it's merged, it's a lot harder for people to work 
> on it unless they are really serious about that driver, so before 
> merging, the janitorial kind of things seldom happen.
> 
> So yes, I really do believe that we should merge drivers in particular a 
> lot more aggressively. I'd like to see *testing* feedback, in order to not 
> merge drivers that simply don't work well enough, but anything else? I 
> suspect other feedback is as likely to cause problems as it is to fix 
> things.
> 
> > This driver even lacks a basic "please fix the > 250 checkpatch errors" [1]
> > and similar low hanging fruits that could easily be spotted and then 
> > fixed by the submitter within a short amount of time.
> 
> Quite frankly, I've several times been *this* close (holds up fingers so 
> you can't even see between them) to just remove checkpatch entirely.

Agrh! What stopped you?!

> I'm personally of the opinion that a lot of checkpatch "fixes" are 
> anything but. That mainly concerns fixing overlong lines (where the 
> "fixed" version is usually worse than the original), but it's been true 
> for some other warnings too.

Speaking of driver, could authors please comment all those barrier()
calls and remove trailing "return;" at the end of void functions.

Re: Merging of completely unreviewed drivers

[Greg KH]

On Fri, Feb 22, 2008 at 01:33:03AM +0300, Alexey Dobriyan wrote:
> On Thu, Feb 21, 2008 at 01:14:55PM -0800, Linus Torvalds wrote:
> > Quite frankly, I've several times been *this* close (holds up fingers so 
> > you can't even see between them) to just remove checkpatch entirely.
> 
> Agrh! What stopped you?!
> 
> > I'm personally of the opinion that a lot of checkpatch "fixes" are 
> > anything but. That mainly concerns fixing overlong lines (where the 
> > "fixed" version is usually worse than the original), but it's been true 
> > for some other warnings too.
> 
> Speaking of driver, could authors please comment all those barrier()
> calls and remove trailing "return;" at the end of void functions.

Why don't you make a patch to checkpatch.pl for those types of things?
:)

Re: Merging of completely unreviewed drivers

[Jeff Garzik]

Greg KH wrote:
> On Fri, Feb 22, 2008 at 01:33:03AM +0300, Alexey Dobriyan wrote:
>> Speaking of driver, could authors please comment all those barrier()
>> calls and remove trailing "return;" at the end of void functions.
> 
> Why don't you make a patch to checkpatch.pl for those types of things?
> :)

Drat, you beat me to that response.....  :)

Re: Merging of completely unreviewed drivers

[Alexey Dobriyan]

On Thu, Feb 21, 2008 at 02:43:15PM -0800, Greg KH wrote:
> On Fri, Feb 22, 2008 at 01:33:03AM +0300, Alexey Dobriyan wrote:
> > On Thu, Feb 21, 2008 at 01:14:55PM -0800, Linus Torvalds wrote:
> > > Quite frankly, I've several times been *this* close (holds up fingers so 
> > > you can't even see between them) to just remove checkpatch entirely.
> > 
> > Agrh! What stopped you?!
> > 
> > > I'm personally of the opinion that a lot of checkpatch "fixes" are 
> > > anything but. That mainly concerns fixing overlong lines (where the 
> > > "fixed" version is usually worse than the original), but it's been true 
> > > for some other warnings too.
> > 
> > Speaking of driver, could authors please comment all those barrier()
> > calls and remove trailing "return;" at the end of void functions.
> 
> Why don't you make a patch to checkpatch.pl for those types of things?
> :)

Sorry, I'm not touching it with an eigthy six foot pole. :^)

Re: Merging of completely unreviewed drivers

[Jan Engelhardt]

On Feb 21 2008 14:43, Greg KH wrote:
>On Fri, Feb 22, 2008 at 01:33:03AM +0300, Alexey Dobriyan wrote:
>> On Thu, Feb 21, 2008 at 01:14:55PM -0800, Linus Torvalds wrote:
>> > Quite frankly, I've several times been *this* close (holds up fingers so 
>> > you can't even see between them) to just remove checkpatch entirely.
>> 
>> Agrh! What stopped you?!
>> 
>> > I'm personally of the opinion that a lot of checkpatch "fixes" are 
>> > anything but. That mainly concerns fixing overlong lines (where the 
>> > "fixed" version is usually worse than the original), but it's been true 
>> > for some other warnings too.
>> 
>> Speaking of driver, could authors please comment all those barrier()
>> calls and remove trailing "return;" at the end of void functions.
>
>Why don't you make a patch to checkpatch.pl for those types of things?
>:)

checkpatch would never allow a patch to patch checkpatch.

Re: Merging of completely unreviewed drivers

[Krzysztof Halasa]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> I'm personally of the opinion that a lot of checkpatch "fixes" are 
> anything but. That mainly concerns fixing overlong lines

Perhaps we should increase line length limit, 132 should be fine.
Especially useful with long printk() lines and long arithmetic
expressions.
-- 
Krzysztof Halasa

Re: Merging of completely unreviewed drivers

[Alan Cox]

On Fri, 22 Feb 2008 00:38:14 +0100
Krzysztof Halasa <khc@pm.waw.pl> wrote:

> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
> > I'm personally of the opinion that a lot of checkpatch "fixes" are 
> > anything but. That mainly concerns fixing overlong lines
> 
> Perhaps we should increase line length limit, 132 should be fine.
> Especially useful with long printk() lines and long arithmetic
> expressions.

Agreed. The fact I'm having to fix bugs introduced by incorrect printk
wrapping confirms that for printk strings at least it is overzealous.

I'm all for it complaining about

  printk(KERN_FOO "<90 chars>", foo, bar + 37);

type bits when the foo, bar should be underneath to be visible but for
straight quoted text too long it should not warn and try to get the text
folded.

Alan

Re: Merging of completely unreviewed drivers

[Adrian Bunk]

On Thu, Feb 21, 2008 at 11:31:44PM +0000, Alan Cox wrote:
> On Fri, 22 Feb 2008 00:38:14 +0100
> Krzysztof Halasa <khc@pm.waw.pl> wrote:
> 
> > Linus Torvalds <torvalds@linux-foundation.org> writes:
> > 
> > > I'm personally of the opinion that a lot of checkpatch "fixes" are 
> > > anything but. That mainly concerns fixing overlong lines
> > 
> > Perhaps we should increase line length limit, 132 should be fine.
> > Especially useful with long printk() lines and long arithmetic
> > expressions.
> 
> Agreed. The fact I'm having to fix bugs introduced by incorrect printk
> wrapping confirms that for printk strings at least it is overzealous.
> 
> I'm all for it complaining about
> 
>   printk(KERN_FOO "<90 chars>", foo, bar + 37);
> 
> type bits when the foo, bar should be underneath to be visible but for
> straight quoted text too long it should not warn and try to get the text
> folded.

I think it should warn, but people have to be aware of the following:
- checkpatch errors are for stuff that really has to be fixed
- checkpatch warnings are for stuff that should be looked at
- the goal is not 0 checkpatch warnings but readable and bugfree code

A nice property of checkpatch is that it encourages to look closer at 
code like the following (it warns about the volatile):

		if (!netif_queue_stopped(netdev)) {
			netif_stop_queue(netdev);
			barrier();
			if ((((((volatile u16)nesnic->sq_tail)+(nesnic->sq_size*2))-nesnic->sq_head) & (nesnic->sq_size - 1)) != 1) {
				netif_start_queue(netdev);
				goto sq_no_longer_full;
			}
		}

> Alan

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

Re: Merging of completely unreviewed drivers

[Jeff Garzik]

Krzysztof Halasa wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
>> I'm personally of the opinion that a lot of checkpatch "fixes" are 
>> anything but. That mainly concerns fixing overlong lines
> 
> Perhaps we should increase line length limit, 132 should be fine.

I think checkpatch is useful, but I've agreed from the beginning that 
the line length complaint is completely silly.

If a driver is full of lines of length >80, that's a problem.  If it's 
just a few, that's more of a developer decision based on the individual 
line of code.

	Jeff

Re: Merging of completely unreviewed drivers

[Krzysztof Halasa]

Jeff Garzik <jeff@garzik.org> writes:

> If a driver is full of lines of length >80, that's a problem.

I'm not sure.
We all have more than 80-chars wide displays for years, don't we? The
problem is not the number of characters but code which is too
complex and which may sometimes have too many levels of indentation.

Unfortunately expressing code complexity in terms of line lengths
doesn't seem to work at all.

The 80-chars limit harms development, it makes the code less readable,
sometimes far less readable.

I think we should increase length limit to 132 for the whole kernel
code. Obviously printk() _output_ etc. should stay at 80.
-- 
Krzysztof Halasa

Re: Merging of completely unreviewed drivers

[Jeff Garzik]

Krzysztof Halasa wrote:
> Jeff Garzik <jeff@garzik.org> writes:
> 
>> If a driver is full of lines of length >80, that's a problem.
> 
> I'm not sure.
> We all have more than 80-chars wide displays for years, don't we? The

Every time this discussion comes up, people point out that it remains 
highly common to open multiple 80-column terminal windows, making the 
80-column limit still highly relevant in modern times.


> The
> problem is [...] code which is too
> complex and which may sometimes have too many levels of indentation.

Quite true.

	Jeff

Re: Merging of completely unreviewed drivers

[Krzysztof Halasa]

Jeff Garzik <jeff@garzik.org> writes:

> Every time this discussion comes up, people point out that it remains
> highly common to open multiple 80-column terminal windows, making the
> 80-column limit still highly relevant in modern times.

I guess only because of the limit :-)
Raise the limit, terminal windows will follow.
I'm using 80-column windows, too.
-- 
Krzysztof Halasa

Re: Merging of completely unreviewed drivers

[Alan Cox]

On Fri, 22 Feb 2008 01:05:26 +0100
Krzysztof Halasa <khc@pm.waw.pl> wrote:

> Jeff Garzik <jeff@garzik.org> writes:
> 
> > If a driver is full of lines of length >80, that's a problem.
> 
> I'm not sure.
> We all have more than 80-chars wide displays for years, don't we? The

Even a vt132 serial terminal or later can do 132. Decades not years.

Re: Merging of completely unreviewed drivers

[Pavel Machek]

On Fri 2008-02-22 01:05:26, Krzysztof Halasa wrote:
> Jeff Garzik <jeff@garzik.org> writes:
> 
> > If a driver is full of lines of length >80, that's a problem.
> 
> I'm not sure.
> We all have more than 80-chars wide displays for years, don't we? The

No.

Zaurus is one example, second is small screen where you need big font
to keep it readable (x60 on desk).
							Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: Merging of completely unreviewed drivers

[Krzysztof Halasa]

Pavel Machek <pavel@ucw.cz> writes:

> Zaurus is one example, second is small screen where you need big font
> to keep it readable (x60 on desk).

Come on, are you doing Linux kernel development on PDA?
-- 
Krzysztof Halasa

Re: Merging of completely unreviewed drivers

[Pavel Machek]

On Fri 2008-02-22 23:44:09, Krzysztof Halasa wrote:
> Pavel Machek <pavel@ucw.cz> writes:
> 
> > Zaurus is one example, second is small screen where you need big font
> > to keep it readable (x60 on desk).
> 
> Come on, are you doing Linux kernel development on PDA?

I review patches on it, sometimes, yes.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: Merging of completely unreviewed drivers

[David Newall]

Pavel Machek wrote:
> On Fri 2008-02-22 23:44:09, Krzysztof Halasa wrote:
>   
>> Pavel Machek <pavel@ucw.cz> writes:
>>
>>     
>>> Zaurus is one example, second is small screen where you need big font
>>> to keep it readable (x60 on desk).
>>>       
>> Come on, are you doing Linux kernel development on PDA?
>>     
>
> I review patches on it, sometimes, yes.
>   

Do you actually get 80 columns wide on it?

Re: Merging of completely unreviewed drivers

[Pavel Machek]

On Sat 2008-02-23 23:08:58, David Newall wrote:
> Pavel Machek wrote:
> > On Fri 2008-02-22 23:44:09, Krzysztof Halasa wrote:
> >   
> >> Pavel Machek <pavel@ucw.cz> writes:
> >>
> >>     
> >>> Zaurus is one example, second is small screen where you need big font
> >>> to keep it readable (x60 on desk).
> >>>       
> >> Come on, are you doing Linux kernel development on PDA?

Actually, I'd like to. There's a lot to fix on zaurus. Bit corruption
while sleeping is high on list, but I guess I should move out of
2.6.16, first.

> > I review patches on it, sometimes, yes.
> >   
> 
> Do you actually get 80 columns wide on it?

No, something like 62... but it is usually enough. x60 is about 100
columns wide (big font needed).

							Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: Merging of completely unreviewed drivers

[David Newall]

Pavel Machek wrote:
> On Sat 2008-02-23 23:08:58, David Newall wrote:
>   
>> Pavel Machek wrote:
>>     
>>> On Fri 2008-02-22 23:44:09, Krzysztof Halasa wrote:
>>>   
>>>       
>>>> Pavel Machek <pavel@ucw.cz> writes:
>>>>
>>>>     
>>>>         
>>>>> Zaurus is one example, second is small screen where you need big font
>>>>> to keep it readable (x60 on desk).
>>>>>       
>>>>>           
>>>> Come on, are you doing Linux kernel development on PDA?
>>>>         
>
> Actually, I'd like to. There's a lot to fix on zaurus. Bit corruption
> while sleeping is high on list, but I guess I should move out of
> 2.6.16, first.
>
>   
>>> I review patches on it, sometimes, yes.
>>>   
>>>       
>> Do you actually get 80 columns wide on it?
>>     
>
> No, something like 62... but it is usually enough. x60 is about 100
> columns wide (big font needed).

Then it's a silly example to raise in a serious discussion of this type.

Re: Merging of completely unreviewed drivers

[Linus Torvalds]

On Sat, 23 Feb 2008, David Newall wrote:
> 
> Do you actually get 80 columns wide on it?

Do people really care that deeply?

I still sometimes use small terminal windows - for a while I had my 
default terminal come up as 100x40, but I'm back to the standard 80x24, 
and while I often resize them, I certainly don't always.

And do I find lines longer than 80 charactes unreadable? Hell no.

Quite frankly, on a 80x24 display, I'll take long lines over split-up ones 
*any* day. For things like doing "git grep xyzzy", I'd *much* rather get 
the occasional long line that wraps (or, if I'm in "less -S", that I have 
to press right-arrow to see), than see just a meaningless fragment because 
somebody decided that they should always fit in 80 characters.

So *consistently* long lines are the problem, not the occasional one. The 
occasional one is likely more readable as it is, than split up.

Here's an example from code that actually looks pretty good in general:

	static unsigned long
	calc_delta_mine(unsigned long delta_exec, unsigned long weight,
	                struct load_weight *lw)

and look around that function in general: it's doesn't match the coding 
standard, but also compare the output of

	git grep calc_delta_mine

with the output of something like

	git grep update_load_sub

which actually shows you what the calling convention is.

So putting that long function definition on one line would make it a
108-character line or somethign like that, but it would have advantages
too.  It would have advantages for anything that is line-based (I use
grep for *everything*, but maybe I'm just odd), but it would also
actually be more readable for the people who have bigger windows.

But my point is, some of those advantages remain even with small
terminals, and quite often the downsides aren't even all that huge. 
Most editors wrap or chop the line according to your preferences (mine
are personally to chop), and if it's a fairly uncommon thing, those
downsides shrink further. 

Is 108 characters perhaps *too* long? In the above case it probably is,
since the downside of splitting the patch is pretty small (it's a static
function, only used in that file, the "grep" argument is weak, yadda
yadda).  But I'm just saying that it's not 100% obvious *even*if* you're
on a 80x24 terminal, and in some other cases the downside of splitting
the line can be much bigger (strings or more spread-out function calls
and declarations etc). 

The line length problem would probably be better attacked as something
more akin to the rule

 - do a rolling window of <n> last non-empty lines (n ~ 15 or so)

 - if more than <m> of those lines were longer than 72 charactes,
   somethign is wrong (m ~ 5 or so). 

which talks more about what matters - too deep indentation. And also 
attacks the problem that is really relevant: it's that kind of code that 
ends up being unreadable because so *much* of it is cut off or wrapped.

			Linus

Re: Merging of completely unreviewed drivers

[David Newall]

Linus Torvalds wrote:
> On Sat, 23 Feb 2008, David Newall wrote:
>   
>> Do you actually get 80 columns wide on it?
>>     
>
> Do people really care that deeply?
> ...
> And do I find lines longer than 80 charactes unreadable? Hell no.
>   

I care, yes.  I've found my code looks much prettier, with attendant
improvement in ease of understanding, since I stopped being so anal
about 80 columns.  The width of the code, that is first to last
non-blank on each line, is about the same, not  because I work to keep
it narrow, but because most statements just are narrow.  Sometimes I do
get really wide statements, for example when using deep data structures,
especially as parameters in procedure calls, and this is easier to read
than having to break the line.

I honestly think the reason we used to insist on lines less than 80
characters was because on an 80 character screen, you get slightly
better readability by choosing where to break each line than simply
letting the hardware do it.  We don't have the physical limit any more,
so we don't need to impose it structurally.

It's about readability, and with due respect, people who've never tried
it aren't qualified to comment.

> which talks more about what matters - too deep indentation.
What's too deep?  Is the following too deep?  It's common enough, other
than my refusal to relax consistent indenting style for switch bodies. 
The code is readable, and breaking it into multiple procedures just to
de-indent is often impossible, and rarely readable.  With a strict 80
character limit, the meat in the sandwich is left with only 20 or so
characters in which to fit.  Add a nested switch, and there's virtually
no space left for code.

123456789012345678901234567890123456789012345678901234567890123456 (70)
int procedure(param list)
{
	switch (condition)
	{
		case value:
			if (another_condition)
			{
				if (variant)
					meat_in_sandwich;
			} else {
				code;
			}
		case value2:
			switch (sub_condition)
			{
				case sub_value:
					if (final_test)
					{
						something(	
							NULL,
							1,
							"two");
					}
			}
	}
}


(Yes, I know, "we don't indent 'case' because it consumes too much
room."  That's inconsistent with the rest of normal indenting style, and
a poor excuse to keep within an obsolete and unnecessary restriction.)

Re: Merging of completely unreviewed drivers

[Linus Torvalds]

On Sun, 24 Feb 2008, David Newall wrote:
> 
> > which talks more about what matters - too deep indentation.
>
> What's too deep?  Is the following too deep?

It would be, if it weren't artificially so, for violates several kernel 
coding standards, one being that the "case" labels indent with the switch, 
not under it (the other being the placement of braces).

> (Yes, I know, "we don't indent 'case' because it consumes too much
> room."

No, that's not it at all. We don't indent 'case' because it matches with 
the 'switch', not because of any room issues.

> That's inconsistent with the rest of normal indenting style, and
> a poor excuse to keep within an obsolete and unnecessary restriction.)

It's not at all inconsistent. It's just making clear how the parts of the 
function group together.

Indenting a case-statement an extra level is as stupid as indenting "else" 
one extra level from the "if ()" it goes together with. Do you think that 
would be sane?

The fact that the 'case' thing is technically parsed as a separate 
statement in C doesn't change anything.

		Linus

Re: Merging of completely unreviewed drivers

[Krzysztof Halasa]

Pavel Machek <pavel@ucw.cz> writes:

>> Come on, are you doing Linux kernel development on PDA?
>
> I review patches on it, sometimes, yes.

I take it the "sometimes" is the key word :-)
-- 
Krzysztof Halasa

Re: Merging of completely unreviewed drivers

[David Newall]

Krzysztof Halasa wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
>> I'm personally of the opinion that a lot of checkpatch "fixes" are 
>> anything but. That mainly concerns fixing overlong lines
>>     
>
> Perhaps we should increase line length limit, 132 should be fine.
> Especially useful with long printk() lines and long arithmetic
> expressions.
>   


Yes; or even longer.  80 characters might have made sense on a screen
when the alternative was 80 characters on a punched card, but on a
modern computer it's very restrictive.  That's especially true with the
deep indents that you quickly get in C.  Even short lines often need to
be split when you put a few tabs in front of them, and that makes
comprehension that bit harder, not to mention looks ugly.

Re: Merging of completely unreviewed drivers

[Al Viro]

On Fri, Feb 22, 2008 at 12:16:45PM +1030, David Newall wrote:
> Krzysztof Halasa wrote:
> > Linus Torvalds <torvalds@linux-foundation.org> writes:
> >> I'm personally of the opinion that a lot of checkpatch "fixes" are 
> >> anything but. That mainly concerns fixing overlong lines
> >>     
> >
> > Perhaps we should increase line length limit, 132 should be fine.
> > Especially useful with long printk() lines and long arithmetic
> > expressions.
> >   
> 
> 
> Yes; or even longer.  80 characters might have made sense on a screen
> when the alternative was 80 characters on a punched card, but on a
> modern computer it's very restrictive.  That's especially true with the
> deep indents that you quickly get in C

... if your style is lousy.  I agree that situation with printks is
not normal in that respect and I certainly have no love for the
checkpatch nonsense, but pressure to keep the fucking nesting depth
low is a Good Thing(tm).

Re: Merging of completely unreviewed drivers

[Krzysztof Halasa]

Al Viro <viro@ZenIV.linux.org.uk> writes:

> ... if your style is lousy.  I agree that situation with printks is
> not normal in that respect and I certainly have no love for the
> checkpatch nonsense, but pressure to keep the fucking nesting depth
> low is a Good Thing(tm).

Indeed. Unfortunately it is orthogonal to the line length limit.

We should limit the nesting level, though I think there is no
universally good value. What is good for one case (a function with a
short multi-level if/for/etc) is bad for another (a long switch()
where any added complexity makes it unparseable).

So I think it just have to meet the author's and reviewers' taste. We
already depend on this.
-- 
Krzysztof Halasa

Re: Merging of completely unreviewed drivers

[Al Viro]

On Fri, Feb 22, 2008 at 03:23:45AM +0100, Krzysztof Halasa wrote:
> Al Viro <viro@ZenIV.linux.org.uk> writes:
> 
> > ... if your style is lousy.  I agree that situation with printks is
> > not normal in that respect and I certainly have no love for the
> > checkpatch nonsense, but pressure to keep the fucking nesting depth
> > low is a Good Thing(tm).
> 
> Indeed. Unfortunately it is orthogonal to the line length limit.

Not quite.  Add such things as choice of sane identifiers.  And sane use of
local variables, while we are at it - things like twenty lines of
	foobar[(index + 1) % BLAH]->spork.vomit[12]->field_name = <expr>;
with the only difference in the field_name, except for one line where
we have a typo and see 11 instead of intended 12, are responsible for quite
a few of such overruns.

IMO the line length overruns make good warnings.  Not as in "here's a cheap
way to get more changesets", but as in "that code might have other problems
nearby" kind of heuristics.

Re: Merging of completely unreviewed drivers

[Krzysztof Halasa]

Al Viro <viro@ZenIV.linux.org.uk> writes:

> IMO the line length overruns make good warnings.  Not as in "here's a cheap
> way to get more changesets", but as in "that code might have other problems
> nearby" kind of heuristics.

Sure, it does. However the human looking at the code is far better at
spotting such problems. Machine-generated warnings are great when the
machine is actually better than human.

Anyway, warnings are one thing and line limit is another. We may raise
the limit leaving the 80-chars warning in place. Unless there are too
many false positives, of course.
-- 
Krzysztof Halasa

Re: Merging of completely unreviewed drivers

[Jörn Engel]

On Fri, 22 February 2008 23:28:58 +0100, Krzysztof Halasa wrote:
> Al Viro <viro@ZenIV.linux.org.uk> writes:
> 
> > IMO the line length overruns make good warnings.  Not as in "here's a cheap
> > way to get more changesets", but as in "that code might have other problems
> > nearby" kind of heuristics.
> 
> Sure, it does. However the human looking at the code is far better at
> spotting such problems. Machine-generated warnings are great when the
> machine is actually better than human.

I strongly disagree.  Machine-generated warnings are a great way of
quickly locating a large amount of questionable code in an otherwise
overwhelming haystack.  It doesn't even matter much, which warnings you
look for.  Almost all code checkers find the same hotspots.

But there is a catch.  If you have an over-eager warning police that
"fixes all the warnings", the warnings may be gone, but the very real
problems in near vicinity are not.  Not to mention new problems
introduced by those claimed "fixes".

One fun hobby in my last job was to write a new code checker and locate
those problem areas hidden behind warning-free code.  I had to write a
new checker so I would see below the polish of "fixes".  The actual
problems found by the checker were often trivial and near-meaningless.
But those warnings are not the point at all, quite the contrary.  The
only important thing was "that code might have other problems nearby".

Note one scary consequence: code checkers in the wrong hands are
actively harmful.

Jörn

-- 
One of my most productive days was throwing away 1000 lines of code.
-- Ken Thompson.

Re: Merging of completely unreviewed drivers

[Krzysztof Halasa]

Jörn Engel <joern@logfs.org> writes:

> I strongly disagree.  Machine-generated warnings are a great way of
> quickly locating a large amount of questionable code in an otherwise
> overwhelming haystack.  It doesn't even matter much, which warnings you
> look for.  Almost all code checkers find the same hotspots.

I think you misunderstood. Of course I'm not against warnings in
general. I'm rather talking about _authority_ of human vs machine,
in this specific ("measuring" code complexity) case.
-- 
Krzysztof Halasa

Re: Merging of completely unreviewed drivers

[Linus Torvalds]

On Fri, 22 Feb 2008, Al Viro wrote:
>
> ... if your style is lousy.  I agree that situation with printks is
> not normal in that respect and I certainly have no love for the
> checkpatch nonsense, but pressure to keep the fucking nesting depth
> low is a Good Thing(tm).

I do agree, but that has little to do with line length *directly*.

IOW, I'd personally be happier with a checkpatch that calculated 
"complexity" and indentation over line length.

There is definitely a correlation there: there is no question that complex 
lines with deep indentation tend to be long. So yes, "long lines are 
correlated with bad code" is certainly true to some degree.

But sometimes lines are long just because it's a function call with 
multiple parameters, and it's just three levels indented, and it had a 
string there too. It may be long, but it's not complex, and keeping it on 
one line actually makes it much easier to visually parse (and grep for, 
for that matter).

So I'd be happier with warnings about deep indentation (but how do you 
count it? Will people then try to fake things out by using 4-space indents 
and then "deep" indentations will look like just a couple of tabs?) and 
against complex expressions (ie "if ((a = xyz()) == NULL) .." should just 
be split up into "a = xyz(); if (!a) ..", but there are sometimes reasons 
for those things too!

		Linus

Re: [ofa-general] Re: Merging of completely unreviewed drivers

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> So I'd be happier with warnings about deep indentation (but how do you 
> count it? Will people then try to fake things out by using 4-space indents 
> and then "deep" indentations will look like just a couple of tabs?) and 
> against complex expressions (ie "if ((a = xyz()) == NULL) .." should just 
> be split up into "a = xyz(); if (!a) ..", but there are sometimes reasons 
> for those things too!

Deep indentation should be fairly easy, given that you
already have rules in place that says "Tabs are 8 characters".
So if you find a line that begins with more than say 4 SP, you
can flag that as already bogus (i.e. "does not indent with HT"),
more than 8 SP definitely so.

I'll leave harder "complex expressions" to sparse experts ;-),

Re: [ofa-general] Re: Merging of completely unreviewed drivers

[Adrian Bunk]

On Thu, Feb 21, 2008 at 10:29:09PM -0800, Junio C Hamano wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
> > So I'd be happier with warnings about deep indentation (but how do you 
> > count it? Will people then try to fake things out by using 4-space indents 
> > and then "deep" indentations will look like just a couple of tabs?) and 
> > against complex expressions (ie "if ((a = xyz()) == NULL) .." should just 
> > be split up into "a = xyz(); if (!a) ..", but there are sometimes reasons 
> > for those things too!
> 
> Deep indentation should be fairly easy, given that you
> already have rules in place that says "Tabs are 8 characters".
> So if you find a line that begins with more than say 4 SP, you
> can flag that as already bogus (i.e. "does not indent with HT"),
> more than 8 SP definitely so.
>...

Checkpatch already has an error "use tabs not spaces".

And people should realize that checkpatch is not a tool for janitors but 
for authors and maintainers to easily spot some of the possible problems 
in a driver and thereby automate some part of patch review.

E.g. in this driver we are talking about checkpatch warns about the
> 2000 lines over 80 characters.

And that's not a surprise and a symptom when code is 6 tabs indented.

If someone said fixing that should not delay the merge of a 16.500 lines
driver I would agree with that since fixing that would require a huge 
amount of work for a not that big gain.

But that a merged driver contains > 250 checkpatch errors is really not 
nice. Most of them are easy to fix stylistic errors that simply make the 
driver easier to read and whose fixing would only take a few hours 
altogether. [1]

And the 13 checkpatch errors about volatile usage are not stuff for 
janitors (unless you count our number one cleanup person Al as janitor) 
but indicate really fishy code.

cu
Adrian

[1] one might argue whether "easier to read" really applies when 
    checkpatch gives errors for e.g. the usage of C99 comments, but
    different from overly long lines that's at least stuff that can
    be fixed very quickly and in a quite automatic way

-- 

       "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

Re: Merging of completely unreviewed drivers

[Ray Lee]

On Thu, Feb 21, 2008 at 7:13 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>  So I'd be happier with warnings about deep indentation (but how do you
>  count it? Will people then try to fake things out by using 4-space indents
>  and then "deep" indentations will look like just a couple of tabs?)

I suspect that 90% of the cases that people really care about would
get caught successfully just by counting brace depth.

ie, by looking at { { {} {} {{{}{}}} } } I bet you can tell me which
section should have been pulled out into a separate routine.

Re: Merging of completely unreviewed drivers

[Jan Engelhardt]

On Feb 21 2008 22:37, Ray Lee wrote:
>On Thu, Feb 21, 2008 at 7:13 PM, Linus Torvalds
><torvalds@linux-foundation.org> wrote:
>>  So I'd be happier with warnings about deep indentation (but how do you
>>  count it? Will people then try to fake things out by using 4-space indents
>>  and then "deep" indentations will look like just a couple of tabs?)
>
>I suspect that 90% of the cases that people really care about would
>get caught successfully just by counting brace depth.
>
>ie, by looking at { { {} {} {{{}{}}} } } I bet you can tell me which
>section should have been pulled out into a separate routine.

Not only that. By clever branch factoring, you can possibly get yourself
rid of lots of deep levels. As in:

static void blah(void)
{
	if (foo) {
		bar;
		bar2;
	} else {
		if (this) {
			that;
			that2;
		} else {
			bad day;
			bad day2;
		}
	}
}

xfrmd:

static void blah(void)
{
	if (foo) {
		bar;
		bar2;
		return;
	}
	if (this) {
		that;
		that2;
		return;
	}
	/* yay, got rid of two levels of indent! */
	good day;
	good day2;
}

Re: Merging of completely unreviewed drivers

[David Newall]

Jan Engelhardt wrote:
> static void blah(void)
> {
> 	if (foo) {
> 		bar;
> 		bar2;
> 		return;
> 	}
> 	if (this) {
> 		that;
> 		that2;
> 		return;
> 	}
> 	/* yay, got rid of two levels of indent! */
> 	good day;
> 	good day2;
> }

I like this style.  It's more readable than the alternative that you
showed.  If you hate returns mid-procedure, as some purists do, the
following is also good:

static void blah(void)
{
	if (foo) {
		bar;
		bar2;
	} else if (this) {
		that;
		that2;
	} else {
		good day;
		good day2;
	}
}

Re: Merging of completely unreviewed drivers

[Krzysztof Halasa]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> Will people then try to fake things out by using 4-space indents 
> and then "deep" indentations will look like just a couple of tabs?)

There is no point in faking it as it's only advisory, it's to help the
author who should be free to ignore the advice. People upstream won't
be fooled by some cheap tab tricks I guess.
-- 
Krzysztof Halasa

Re: [ofa-general] Re: Merging of completely unreviewed drivers

[Bart Van Assche]

On Fri, Feb 22, 2008 at 2:46 AM, David Newall <davidn@davidnewall.com> wrote:
> Krzysztof Halasa wrote:
>  > Perhaps we should increase line length limit, 132 should be fine.
>  > Especially useful with long printk() lines and long arithmetic
>  > expressions.
>
>  Yes; or even longer.  80 characters might have made sense on a screen
>  when the alternative was 80 characters on a punched card, but on a
>  modern computer it's very restrictive.  That's especially true with the
>  deep indents that you quickly get in C.  Even short lines often need to
>  be split when you put a few tabs in front of them, and that makes
>  comprehension that bit harder, not to mention looks ugly.

There is a reason to limit line length: scientific research has shown
that readability of regular texts is optimal for a line length between
55 and 65 characters. My experience is that the readability of source
code decreases when the lines are very long (more than 160
characters).

Bart Van Assche.

Re: [ofa-general] Re: Merging of completely unreviewed drivers

[David Newall]

Bart Van Assche wrote:
> There is a reason to limit line length: scientific research has shown
> that readability of regular texts is optimal for a line length between
> 55 and 65 characters.

Putting aside the point that we're talking code, not regular text, I've
heard that said before and I don't think it's quite like that.  Perhaps
the numbers you said might assume various things such as the width of
the eye's field of view, the distance to the image and the size of each
character?


>  My experience is that the readability of source
> code decreases when the lines are very long (more than 160
> characters).

The point is that the width, excluding leading and trailing white space,
is what really matters.  Even deeply indented code can be a snap to
understand if you don't have to fight artificial line breaks.  And we've
got a much wider -- and taller! -- space available than we had in the
old 80x24 (and 80x1) days.

Re: [ofa-general] Re: Merging of completely unreviewed drivers

[Peter Zijlstra]

On Sat, 2008-02-23 at 00:55 +1030, David Newall wrote:
> Bart Van Assche wrote:
> > There is a reason to limit line length: scientific research has shown
> > that readability of regular texts is optimal for a line length between
> > 55 and 65 characters.
> 
> Putting aside the point that we're talking code, not regular text, I've
> heard that said before and I don't think it's quite like that.  Perhaps
> the numbers you said might assume various things such as the width of
> the eye's field of view, the distance to the image and the size of each
> character?

Not in my experience.

> >  My experience is that the readability of source
> > code decreases when the lines are very long (more than 160
> > characters).
> 
> The point is that the width, excluding leading and trailing white space,
> is what really matters.  Even deeply indented code can be a snap to
> understand if you don't have to fight artificial line breaks.  And we've
> got a much wider -- and taller! -- space available than we had in the
> old 80x24 (and 80x1) days.

I have 2 24" screens running at 1920x1200 with X forced to 75dpi and use
a 8pt Monospace font. (Yes, I can read that from more than 3ft away)

Using a fullscreen gvim (without the icons, but with the menu) with 3
vertical splits gives me 4 columns of 113 rows and 95 chars.

So, yes, I have the screen estate for very long lines, but I find that
long lines require more effort to read (that very much includes leading
whitespace). Also, since long lines are rare (and they should be, if you
nest too deep you have other issues) accommodating them would waste a
lot of screen estate otherwise useful for another column of text.

Even with e-mail, I can easily show over 200 characters wide with a
large font (say 11pt) but find it harder to read emails that don't
nicely wrap at 78. So much so that I often find myself not reading the
mail, or restyling it if I find it important enough to read anyway.

Please, lets keep the 80 as a guideline, and not trip over the
occasional lines that exceed it in good style (read: wrapping them would
indeed give uglier code)

Re: [ofa-general] Re: Merging of completely unreviewed drivers

[John W. Linville]

On Fri, Feb 22, 2008 at 04:17:17PM +0100, Peter Zijlstra wrote:

> Even with e-mail, I can easily show over 200 characters wide with a
> large font (say 11pt) but find it harder to read emails that don't
> nicely wrap at 78. So much so that I often find myself not reading the
> mail, or restyling it if I find it important enough to read anyway.

Yes, ditto.  And since most of my patch review is done inside mutt...

-- 
John W. Linville
linville@tuxdriver.com

Re: [ofa-general] Re: Merging of completely unreviewed drivers

[Krzysztof Halasa]

Peter Zijlstra <a.p.zijlstra@chello.nl> writes:

> So, yes, I have the screen estate for very long lines, but I find that
> long lines require more effort to read (that very much includes leading
> whitespace). Also, since long lines are rare (and they should be, if you
> nest too deep you have other issues) accommodating them would waste a
> lot of screen estate otherwise useful for another column of text.

Either they are rare and you can wrap them and still use 80 columns,
or it turns out they are not so rare and you may want to use wider
windows (not necessarily 132 but perhaps 100).

I think the question isn't if they are rare or not, or if people have
3 * 1920 pixels/line or just 1280.

The question is: is the code more readable with hard limit equal to 80
characters, or maybe is it better to limit code block complexity
instead, and let the maximum number of those small pictures in a line
alone? (Limiting at 132 would have technical sense IMHO).

Better code readability = less bugs without any additional
effort.

> Even with e-mail, I can easily show over 200 characters wide with a
> large font (say 11pt) but find it harder to read emails that don't
> nicely wrap at 78.

Sure - because email is not C code.

Actually you don't "read" C code, word by word, as you read books - do
you?
-- 
Krzysztof Halasa

Re: [ofa-general] Re: Merging of completely unreviewed drivers

[Al Viro]

On Fri, Feb 22, 2008 at 11:59:35PM +0100, Krzysztof Halasa wrote:

> Sure - because email is not C code.
> 
> Actually you don't "read" C code, word by word, as you read books - do
> you?

If it's decently written - sure, why not?  Unfortunately, more common case
is somewhere between the writing on the lavatory wall and appartment lease
agreement, with several high school essays mixed in...

Re: [ofa-general] Re: Merging of completely unreviewed drivers

[John W. Linville]

On Sat, Feb 23, 2008 at 12:55:03AM +1030, David Newall wrote:
> Bart Van Assche wrote:
> > There is a reason to limit line length: scientific research has shown
> > that readability of regular texts is optimal for a line length between
> > 55 and 65 characters.
> 
> Putting aside the point that we're talking code, not regular text, I've
> heard that said before and I don't think it's quite like that.  Perhaps
> the numbers you said might assume various things such as the width of
> the eye's field of view, the distance to the image and the size of each
> character?

I'm sure all those assumptions are baked-in to the estimate.  Yet the
fact remains that people's eyes are only so good and most people will
be reading at similar distances from the screen.  So I don't see any
reason to invalidate those assumptions.  FWIW, I find reading longer
lines to be painful -- it is easier to loose one's place in the text.

I would also echo a point Jeff Garzik made elsewhere that it is often
beneficial to have multiple windows oppen side-by-side.  Longer lines
makes it harder to do that in a useful way.  Instead the lines either
wrap or just trail off the screen.  See the output of sdiff for how
this limits usefulness.

> >  My experience is that the readability of source
> > code decreases when the lines are very long (more than 160
> > characters).
> 
> The point is that the width, excluding leading and trailing white space,
> is what really matters.  Even deeply indented code can be a snap to
> understand if you don't have to fight artificial line breaks.  And we've
> got a much wider -- and taller! -- space available than we had in the
> old 80x24 (and 80x1) days.

I'm not sure deeply indented code is ever a snap to understand.
And FWIW, I'd rather deal with "artificial" line breaks than parameter
lists that just stream off the side of the page.  The line breaks
make long parameters lists easier to digest.  I'll sacrifice the
occasional odd breakage of a long string.

John
-- 
John W. Linville
linville@tuxdriver.com

Re: Merging of completely unreviewed drivers

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> I'm personally of the opinion that a lot of checkpatch "fixes" are 
> anything but. That mainly concerns fixing overlong lines (where the 
> "fixed" version is usually worse than the original), but it's been 
> true for some other warnings too.

that was certainly the case for the earlier checkpatch releases which 
treated overlong lines as an error.

So here's a quick list of negative and positive aspects of current 
versions of checkpatch, as i see them.

But let me first declare it that when scripts/checkpatch.pl was 
initially merged last year i immediately ran it over my own files and 
became a deep sceptic of it. (check the lkml archives, i complained alot 
about it)

Now i've got more than half a year of experience with using checkpatch 
as an integral part of scheduler maintenance, and we've now got 4 months 
of experience with using checkpatch in arch/x86 maintenance.

Based on this first hand experience, my opinion about checkpatch has 
changed, rather radically: i now believe that checkpatch is almost as 
important to the long term health of our kernel development process as 
BitKeeper/Git turned out to be. If i had to stop using it today, it 
would be almost as bad of a step backwards to me as if we had to migrate 
the kernel source code control to CVS.

Lets see the Bad Side of checkpatch:

 1) checkpatch "errors" shouldnt be taken too seriously for newly 
    introduced "leaf" driver code, which code we dont at all know 
    whether we'll be maintaining in any serious manner in the future. 
    Slowing down a submission by requirig it to pass checkpatch is not 
    as clear-cut as it is for core infrastructure and architecture code.
    It's far more important to get _any_ code to users (as long as it's 
    not outright harmful) than to nitpick about style details.

 2) it still has some false positives. (They are quite rare in the 
    latest versions, about 1 out of 100 for code that is already 
    "clean". I send them over to Andy whenever i see them, and they get 
    fixed quickly. The false positives were a big annoyance in early 
    checkpatch.pl versions, these days they are not - to me at least.)

 3) it's _really_ annoying when sometimes i stumble over some old, 
    crufty piece of code that according to checkpatch is in high need of 
    some good, thorough cleanup - and when i take a look at the code it 
    turns out that the original author of that crap piece of code turns 
    out to be ... me. Those moments can be pretty embarrasing and 
    sobering ;-)

The Good Side of checkpatch (and here i'll only list the non-obvious 
advantages):

 1) 90% of the scheduler related checkpatch fixes today you'll never 
    recognize in a commit! The fixes all happen before code is 
    submitted, and the fixes are seemlessly embedded in nice looking 
    patches. (in that sense checkpatch is a bit like lockdep: 90% of the 
    errors they detect wont hit lkml, ever.)

 2) you might know that Deja-Vu moment when you look at a new patch that 
    has been submitted to lkml and you have a strange, weird "feeling" 
    that there's something wrong about the patch.

    It's totally subconscious, and you take a closer look and a few
    seconds later you find a real bug in the code.

    That "feeling" i believe comes from a fundamental property of how 
    human vision is connected to the human brain: pattern matching. 
    Really good programmers have built a "library" of patterns of "good" 
    and "bad" looking coding practices.

    If a patch or if a file has a clean _style_, bugs and deeper 
    structural problems often stand out like a sore thumb. But if the 
    code is peppered with random style noise, it's a lot harder (for me 
    at least) to notice real bugs. I can notice bugs in a squeeky clean 
    code base about 5 times easier than in a noisy codebase. This effect
    alone makes checkpatch indispensible for the scheduler and for 
    arch/x86.

    Sidenote: i dont really need fancy metrics trying to tell me how 
    good an algorithm _truly_ is (although it certainly would be 
    interesting to have). I can _see_ that at a glance - provided the
    code follows common kernel practices and a common, consistent style.
    Checkpatch makes visual code patterns universal and eases the human
    maintainance work enormously, for a 150+ KLOC subsystem like
    arch/x86. I'm not distracted (visually and mentally) by the thick
    fog of small silly details and quirks in coding style. Others might
    have radar eyes and radar brains, i dont :-)

 3) checkpatch also keeps _my_ bugs out of the kernel in an interesting
    way. I'm sure many of you are like me: i've got "weaker" moments
    when i write rather crappy code, and i've got "stronger" moments
    when i'm in the flow and can write a few thousand lines of code with
    nary a hickup. What makes things worse is it's really hard to tell
    the two apart.

    It turns out - and this surprised me a lot - that when i write new
    code that is "weaker", i tend to make more "style mistakes", without
    noticing them. Later on, when i do a checkpatch run, i see some
    weird looking code and find that it's also buggy!

    This concept also works with code written by others: when i get a
    careless patch written in a hurry, it is much more likely to have
    style errors in it, and as a maintainer i'm warned about that fact. 

    The best programmers are the ones who have a good eye for details - 
    and that subconsciously extends to "style details" too. I've yet to
    see a _single_ example of a good, experienced kernel programmer who 
    writes code that looks absolutely careless and sloppy, but which is 
    top-notch otherwise. (Newbies will make style mistakes a lot more 
    often - and for them checkpatch is a nice and easy experience at 
    reading other people's code and trying to learn the style of the 
    kernel.)

 4) there's a psychological effect as well: clean _looking_ code is 
    more attractive to coders to improve upon. Once the code _looks_ 
    clean (mechanically), the people with the real structural cleanups 
    are not far away either. Code that just looks nice is simply more of 
    a pleasure to work with and to improve, so there's a strong 
    psychological relationship between the "small, seemingly unimportant 
    details" cleanups and the real, structural cleanups.

    On the other hand, bad looking, unaesthetic code is avoided by 
    kernel developers like the pest. That is a constant skewing force 
    that is very harmful to Linux, because the "current style" of 
    subsystems is a pretty random property at the moment, and there are 
    _many_ important codebases in the kernel that are avoided by most of 
    us purely just because they look so awful.

 5) cleanups were rather hard to get upstream before, because there was 
    never any true "objective basis" for the cleanups, giving an easy 
    excuse for flames over stupid taste differences, and making it easy 
    for maintainers to reject 90%-good cleanups just based on taste
    differences. Checkpatch gives the right tool to people to write 
    consistently clean code and makes it harder for maintainers to find 
    the arguments to keep keep code unclean.

After this list of rather subjective impressions, i've also got some 
historic raw data as well about how arch/x86 cleanups progressed over 
the past 4 months.

  ( NOTE: the "errors" count below does _not_ include "lines longer than 
    80 chars" warnings nor any of the other checkpatch warnings - only 
    checkpatch "errors" which are real bona fide style errors in 99%+ of 
    the cases. )

                                          errors  lines of code  errors/KLOC
 ........................................................................
 v2.6.24-rc1      arch/x86/ [23 Oct 2007]  8695      117423         74.0
 v2.6.24-x86.git  arch/x86/ [21 Nov 2007]  5190      117156         44.2
 v2.6.24-x86.git  arch/x86/ [18 Dec 2007]  4057      117213         34.6
 v2.6.24-x86.git  arch/x86/ [ 8 Jan 2008]  3650      117987         30.9
 v2.6.24-x86.git  arch/x86/ [ 4 Feb 2008]  3334      133542         24.9
 v2.6.25-x86.git  arch/x86/ [21 Feb 2008]  2724      136963         19.8

 [ See: http://redhat.com/~mingo/x86.git/code-quality - although i guess 
   i should rename it to "style-quality" - because there is no direct
   mapping of style quality to real code quality. NOTE: some of the 
   reductions in the error count above are mechanic from things like 
   really long arrays or the math-emu changes - but most of the real
   reductions are genuine. ]

v2.6.24-rc1 was the raw arch/x86 code how we inherited it after we did 
the mechanic unification without changing any of the files. After that 
point you can see a marked reduction in the total count of style errors.

While many of the fixes are just small details and may all seem 
insignificant in isolation, IMO the sum of those small details matters 
_a lot_: in the past 4 months the code has become a lot more hackable to 
us and that process was driven in large part by checkpatch.

	Ingo

Re: [ofa-general] Re: Merging of completely unreviewed drivers

[Bart Van Assche]

On Fri, Feb 22, 2008 at 7:54 PM, Ingo Molnar <mingo@elte.hu> wrote:
>     If a patch or if a file has a clean _style_, bugs and deeper
>     structural problems often stand out like a sore thumb. But if the
>     code is peppered with random style noise, it's a lot harder (for me
>     at least) to notice real bugs. I can notice bugs in a squeeky clean
>     code base about 5 times easier than in a noisy codebase. This effect
>     alone makes checkpatch indispensible for the scheduler and for
>     arch/x86.

I also appreciate style uniformity in kernel code. My (limited)
experience with checkpatch is that most checkpatch complaints are easy
to resolve.

Bart Van Assche.

Re: Merging of completely unreviewed drivers

[Jeff Garzik]

Ingo Molnar wrote:
>  2) you might know that Deja-Vu moment when you look at a new patch that 
>     has been submitted to lkml and you have a strange, weird "feeling" 
>     that there's something wrong about the patch.
> 
>     It's totally subconscious, and you take a closer look and a few
>     seconds later you find a real bug in the code.
> 
>     That "feeling" i believe comes from a fundamental property of how 
>     human vision is connected to the human brain: pattern matching. 
>     Really good programmers have built a "library" of patterns of "good" 
>     and "bad" looking coding practices.
> 
>     If a patch or if a file has a clean _style_, bugs and deeper 
>     structural problems often stand out like a sore thumb. But if the 
[...]

>     The best programmers are the ones who have a good eye for details - 
>     and that subconsciously extends to "style details" too. I've yet to
>     see a _single_ example of a good, experienced kernel programmer who 
>     writes code that looks absolutely careless and sloppy, but which is 
>     top-notch otherwise. (Newbies will make style mistakes a lot more 
>     often - and for them checkpatch is a nice and easy experience at 
>     reading other people's code and trying to learn the style of the 
>     kernel.)
[...]

>  4) there's a psychological effect as well: clean _looking_ code is 
>     more attractive to coders to improve upon. Once the code _looks_ 
>     clean (mechanically), the people with the real structural cleanups 
>     are not far away either. Code that just looks nice is simply more of 
>     a pleasure to work with and to improve, so there's a strong 
>     psychological relationship between the "small, seemingly unimportant 
>     details" cleanups and the real, structural cleanups.

The above deserved to be quoted...  just because I agree with all of it 
so strongly :)

Bugs really do "hide" in ugly code, in part because my brain has been 
optimized to review clean code.

Like everything else in life, one must strike a balance between picking 
style nits with someone's patch, and making honest criticisms of a patch 
because said patch is too "unclean" to be reviewed by anyone.

	Jeff

Re: Merging of completely unreviewed drivers

[Greg KH]

On Fri, Feb 22, 2008 at 02:20:12PM -0500, Jeff Garzik wrote:
> Ingo Molnar wrote:
>>  2) you might know that Deja-Vu moment when you look at a new patch that   
>>   has been submitted to lkml and you have a strange, weird "feeling"     
>> that there's something wrong about the patch.
>>     It's totally subconscious, and you take a closer look and a few
>>     seconds later you find a real bug in the code.
>>     That "feeling" i believe comes from a fundamental property of how     
>> human vision is connected to the human brain: pattern matching.     Really 
>> good programmers have built a "library" of patterns of "good"     and 
>> "bad" looking coding practices.
>>     If a patch or if a file has a clean _style_, bugs and deeper     
>> structural problems often stand out like a sore thumb. But if the 
> [...]
>
>>     The best programmers are the ones who have a good eye for details -    
>>  and that subconsciously extends to "style details" too. I've yet to
>>     see a _single_ example of a good, experienced kernel programmer who    
>>  writes code that looks absolutely careless and sloppy, but which is     
>> top-notch otherwise. (Newbies will make style mistakes a lot more     
>> often - and for them checkpatch is a nice and easy experience at     
>> reading other people's code and trying to learn the style of the     
>> kernel.)
> [...]
>
>>  4) there's a psychological effect as well: clean _looking_ code is     
>> more attractive to coders to improve upon. Once the code _looks_     clean 
>> (mechanically), the people with the real structural cleanups     are not 
>> far away either. Code that just looks nice is simply more of     a 
>> pleasure to work with and to improve, so there's a strong     
>> psychological relationship between the "small, seemingly unimportant     
>> details" cleanups and the real, structural cleanups.
>
> The above deserved to be quoted...  just because I agree with all of it so 
> strongly :)
>
> Bugs really do "hide" in ugly code, in part because my brain has been 
> optimized to review clean code.
>
> Like everything else in life, one must strike a balance between picking 
> style nits with someone's patch, and making honest criticisms of a patch 
> because said patch is too "unclean" to be reviewed by anyone.

I totally agree with all of this.  checkpatch.pl is a useful tool to
use, and is quite handy for helping the kernel code for all of the above
reasons.

</aol>

thanks,

greg k-h

Re: Merging of completely unreviewed drivers

[Greg KH]

On Thu, Feb 21, 2008 at 11:01:24PM +0200, Adrian Bunk wrote:
> 
> BTW: Greg, you are Cc'ed for your joke in [3]...

> [3] http://lkml.org/lkml/2008/2/12/427

That was not a joke, I ment it.  Do you have proof that the majority of
patches going into the kernel tree are not reviewed by at least 2
people?  Now they might not be 2 people that you personally like/agree
with, but that's a totally different topic...

And I'm with Linus on this one, it's much easier to work on driver fixes
together with others, when they are in the kernel tree.

Although I do like the checkpatch.pl script, it has helped me in making
it easier to clean up some vendor-provided drivers recently, finding
some obvious coding style issues that I had missed the first pass
through.

thanks,

greg k-h

Re: Merging of completely unreviewed drivers

[Adrian Bunk]

On Thu, Feb 21, 2008 at 01:30:37PM -0800, Greg KH wrote:
> On Thu, Feb 21, 2008 at 11:01:24PM +0200, Adrian Bunk wrote:
> > 
> > BTW: Greg, you are Cc'ed for your joke in [3]...
> 
> > [3] http://lkml.org/lkml/2008/2/12/427
> 
> That was not a joke, I ment it.  Do you have proof that the majority of
> patches going into the kernel tree are not reviewed by at least 2
> people?
>...

I don't see any way for getting a proof in any direction, but no matter 
how many SOB lines a patch has my impression is that usually at a 
maximum the one person who applies a patch reviews it ("review" as in 
"understands the code in question well and reviews the patch line for 
line").

Sometimes there's even simply noone who could a patch at all, e.g. I'm 
not sure whether there is anyone at all who would be able to review a 
patch by Sam fiddling with kbuild internals.

How many lines of code get changed in the kernel per day?

And we should have for each changed line two people who are both
experienced enough in this area of the kernel and who have the time to
review this line?

Even one of our best maintained subsystems has commits that contain
bugs like

+       if ((!tid_agg_rx->reorder_buf) && net_ratelimit()) {
+               printk(KERN_ERR "can not allocate reordering buffer "
+                                               "to tid %d\n", tid);
+               goto end;
+       }

> thanks,
> 
> greg k-h

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

Re: Merging of completely unreviewed drivers

[Arjan van de Ven]

On Thu, 21 Feb 2008 23:01:24 +0200
Adrian Bunk <bunk@kernel.org> wrote:

> [ Linus Added to the To: since I want to hear his opinion on this
> issue. ]
> 
> On Thu, Feb 21, 2008 at 12:28:55PM -0800, Roland Dreier wrote:
> >  > This driver should really have gotten some review before being
> >  > included in the kernel.
> > 
> >  > Even a simple checkpatch run finds more than > 250 stylistic
> >  > errors (not code bugs but cases where the driver violates the
> >  > standard code formatting rules of kernel code).
> > 
> > Linus has strongly stated that we should merge hardware drivers
> > early, and I agree: although the nes driver clearly needs more
> > work, there's no advantage to users with the hardware in forcing
> > them to wait for 2.6.26 to merge the driver, since they'll just
> > have to patch the grungy code in themselves anyway.  And by merging
> > the driver early, we get fixed up for any tree-wide changes and
> > allow janitors to help with the cleanup.
> 
> Is it really intended to merge drivers without _any_ kind of review?

No of course not.

I totally agree we should be more agressive in merging drivers earlier.
A minimal review needs to happen so for a few things imo
1) That the driver doesn't break the build
2) That the driver has no obvious huge security holes
   (this is a big deal for unsuspecting users)
3) that there's not an obscene amount of "uses deprecated api" compiler warnings
   (since those are annoying for everyone else)
4) that people who don't have the hardware are not negatively affected
   (say crashes without the hw or so)


beyond that.. that's what EXPERIMENTAL is for
(joking; lets not open that can of fish)

Re: Merging of completely unreviewed drivers

[Jeff Garzik]

Arjan van de Ven wrote:
> On Thu, 21 Feb 2008 23:01:24 +0200
> Adrian Bunk <bunk@kernel.org> wrote:
> 
>> [ Linus Added to the To: since I want to hear his opinion on this
>> issue. ]
>>
>> On Thu, Feb 21, 2008 at 12:28:55PM -0800, Roland Dreier wrote:
>>>  > This driver should really have gotten some review before being
>>>  > included in the kernel.
>>>
>>>  > Even a simple checkpatch run finds more than > 250 stylistic
>>>  > errors (not code bugs but cases where the driver violates the
>>>  > standard code formatting rules of kernel code).
>>>
>>> Linus has strongly stated that we should merge hardware drivers
>>> early, and I agree: although the nes driver clearly needs more
>>> work, there's no advantage to users with the hardware in forcing
>>> them to wait for 2.6.26 to merge the driver, since they'll just
>>> have to patch the grungy code in themselves anyway.  And by merging
>>> the driver early, we get fixed up for any tree-wide changes and
>>> allow janitors to help with the cleanup.
>> Is it really intended to merge drivers without _any_ kind of review?
> 
> No of course not.
> 
> I totally agree we should be more agressive in merging drivers earlier.
> A minimal review needs to happen so for a few things imo
> 1) That the driver doesn't break the build
> 2) That the driver has no obvious huge security holes
>    (this is a big deal for unsuspecting users)
> 3) that there's not an obscene amount of "uses deprecated api" compiler warnings
>    (since those are annoying for everyone else)
> 4) that people who don't have the hardware are not negatively affected
>    (say crashes without the hw or so)

FWIW, my general guidelines for merging drivers in my areas are:

1) it's not fugly

2) it has an active maintainer who responds to feedback


I tend to think it is NOT in the best interests of Linux users, for us 
to merge vendor-fugly drivers with many layers of OS wrappers and 
similar obfuscation.

But similarly...  I merge drivers long before our SCSI maintainer will, 
and I value "it works" above stupid checkpatch warnings.

	Jeff

Re: Merging of completely unreviewed drivers

[Adrian Bunk]

On Thu, Feb 21, 2008 at 05:33:10PM -0500, Jeff Garzik wrote:
>...
> But similarly...  I merge drivers long before our SCSI maintainer will,  
> and I value "it works" above stupid checkpatch warnings.

I was not talking about checkpatch warnings.

I'm talking about checkpatch errors for code like
						if ((page_count!=0)&&(page_count<<12)-(region->offset&(4096-1))>=region->length)

I have to accept that Linus prefers to have the driver merged first and 
let janitors make the code readable in subsequent patches, but if GNU 
indent wasn't unable to properly cope with the fact that this driver has 
over 2000 lines that are over 80 characters long I'd simply run this 
driver through scripts/Lindent .

> 	Jeff

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

Re: Merging of completely unreviewed drivers

[Pavel Machek]

On Thu 2008-02-21 14:08:55, Arjan van de Ven wrote:
> On Thu, 21 Feb 2008 23:01:24 +0200
> Adrian Bunk <bunk@kernel.org> wrote:
> 
> > [ Linus Added to the To: since I want to hear his opinion on this
> > issue. ]
> > 
> > On Thu, Feb 21, 2008 at 12:28:55PM -0800, Roland Dreier wrote:
> > >  > This driver should really have gotten some review before being
> > >  > included in the kernel.
> > > 
> > >  > Even a simple checkpatch run finds more than > 250 stylistic
> > >  > errors (not code bugs but cases where the driver violates the
> > >  > standard code formatting rules of kernel code).
> > > 
> > > Linus has strongly stated that we should merge hardware drivers
> > > early, and I agree: although the nes driver clearly needs more
> > > work, there's no advantage to users with the hardware in forcing
> > > them to wait for 2.6.26 to merge the driver, since they'll just
> > > have to patch the grungy code in themselves anyway.  And by merging
> > > the driver early, we get fixed up for any tree-wide changes and
> > > allow janitors to help with the cleanup.
> > 
> > Is it really intended to merge drivers without _any_ kind of review?
> 
> No of course not.
> 
> I totally agree we should be more agressive in merging drivers earlier.
> A minimal review needs to happen so for a few things imo
> 1) That the driver doesn't break the build
> 2) That the driver has no obvious huge security holes
>    (this is a big deal for unsuspecting users)
> 3) that there's not an obscene amount of "uses deprecated api" compiler warnings
>    (since those are annoying for everyone else)
> 4) that people who don't have the hardware are not negatively affected
>    (say crashes without the hw or so)

5) does not introduce new and ugly user-kernel we'll have problems
fixing/removing?
							Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html