This patch fixes an off-by-one spotted by the Coverity checker.
Signed-off-by: Adrian Bunk <[email protected]>
---
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;
Thanks, this is already upstream as 51af33e8
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
> 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.
> -----Original Message-----
> From: Roland Dreier [mailto:[email protected]]
> Sent: Wednesday, February 20, 2008 5:22 PM
> To: Adrian Bunk
> Cc: Faisal Latif; Glenn Streiff; [email protected];
> [email protected]
> 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
> > > 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
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
> 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.
[ 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
> 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.
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
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
On Thu, 21 Feb 2008 23:01:24 +0200
Adrian Bunk <[email protected]> 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)
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.
Arjan van de Ven wrote:
> On Thu, 21 Feb 2008 23:01:24 +0200
> Adrian Bunk <[email protected]> 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
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?
:)
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..... :)
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. :^)
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.
Linus Torvalds <[email protected]> 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
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
Krzysztof Halasa wrote:
> Linus Torvalds <[email protected]> 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
On Fri, 22 Feb 2008 00:38:14 +0100
Krzysztof Halasa <[email protected]> wrote:
> Linus Torvalds <[email protected]> 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
Jeff Garzik <[email protected]> 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
On Thu, Feb 21, 2008 at 11:31:44PM +0000, Alan Cox wrote:
> On Fri, 22 Feb 2008 00:38:14 +0100
> Krzysztof Halasa <[email protected]> wrote:
>
> > Linus Torvalds <[email protected]> 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
Krzysztof Halasa wrote:
> Jeff Garzik <[email protected]> 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
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
Krzysztof Halasa wrote:
> Linus Torvalds <[email protected]> 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.
Jeff Garzik <[email protected]> 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
On Fri, Feb 22, 2008 at 12:16:45PM +1030, David Newall wrote:
> Krzysztof Halasa wrote:
> > Linus Torvalds <[email protected]> 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).
Al Viro <[email protected]> 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
On Fri, Feb 22, 2008 at 03:23:45AM +0100, Krzysztof Halasa wrote:
> Al Viro <[email protected]> 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.
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
Linus Torvalds <[email protected]> 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 ;-),
On Thu, Feb 21, 2008 at 7:13 PM, Linus Torvalds
<[email protected]> 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.
On Thu, Feb 21, 2008 at 10:29:09PM -0800, Junio C Hamano wrote:
> Linus Torvalds <[email protected]> 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
On Fri, 22 Feb 2008 01:05:26 +0100
Krzysztof Halasa <[email protected]> wrote:
> Jeff Garzik <[email protected]> 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.
On Fri, Feb 22, 2008 at 2:46 AM, David Newall <[email protected]> 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.
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.
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)
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
[email protected]
* Linus Torvalds <[email protected]> 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
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
[email protected]
On Fri, Feb 22, 2008 at 7:54 PM, Ingo Molnar <[email protected]> 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.
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
On Thu 2008-02-21 14:08:55, Arjan van de Ven wrote:
> On Thu, 21 Feb 2008 23:01:24 +0200
> Adrian Bunk <[email protected]> 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
On Fri 2008-02-22 01:05:26, Krzysztof Halasa wrote:
> Jeff Garzik <[email protected]> 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
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
Al Viro <[email protected]> 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
Linus Torvalds <[email protected]> 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
Pavel Machek <[email protected]> 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
Peter Zijlstra <[email protected]> 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
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...
On Fri 2008-02-22 23:44:09, Krzysztof Halasa wrote:
> Pavel Machek <[email protected]> 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
Pavel Machek wrote:
> On Fri 2008-02-22 23:44:09, Krzysztof Halasa wrote:
>
>> Pavel Machek <[email protected]> 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?
Pavel Machek <[email protected]> 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
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 <[email protected]> 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
On Feb 21 2008 22:37, Ray Lee wrote:
>On Thu, Feb 21, 2008 at 7:13 PM, Linus Torvalds
><[email protected]> 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;
}
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
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 <[email protected]> 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.
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;
}
}
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.)
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
On Fri, 22 February 2008 23:28:58 +0100, Krzysztof Halasa wrote:
> Al Viro <[email protected]> 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.
J?rn Engel <[email protected]> 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