login
Search
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;
--
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. --
Sweeping through them right now. Should have something for you tonight. Glenn --
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 --
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
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. ]
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
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
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. --
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 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 --
Speaking of driver, could authors please comment all those barrier() calls and remove trailing "return;" at the end of void functions. --
Drat, you beat me to that response..... :) --
Sorry, I'm not touching it with an eigthy six foot pole. :^) --
checkpatch would never allow a patch to patch checkpatch. --
Perhaps we should increase line length limit, 132 should be fine. Especially useful with long printk() lines and long arithmetic expressions. -- Krzysztof Halasa --
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 --
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 --
Every time this discussion comes up, people point out that it remains highly common to open multiple 80-column terminal windows, making the Quite true. Jeff --
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, 22 Feb 2008 01:05:26 +0100 Even a vt132 serial terminal or later can do 132. Decades not years. --
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 --
Come on, are you doing Linux kernel development on PDA? -- Krzysztof Halasa --
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 --
Do you actually get 80 columns wide on it? --
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 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 --
Then it's a silly example to raise in a serious discussion of this type. --
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 ...
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
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.)
--
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, No, that's not it at all. We don't indent 'case' because it matches with 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 --
I take it the "sometimes" is the key word :-) -- Krzysztof Halasa --
On Fri, 22 Feb 2008 00:38:14 +0100 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 --
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;
}
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
--
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. --
... 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 --
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. --
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 --
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. --
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 --
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 --
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 ;-), --
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.
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 Thu, Feb 21, 2008 at 7:13 PM, Linus Torvalds
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;
}
--
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;
}
}
--
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 --
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. --
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 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) --
Yes, ditto. And since most of my patch review is done inside mutt... -- John W. Linville linville@tuxdriver.com --
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 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 --
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... --
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 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 --
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 ...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. --
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 --
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 --
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 --
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;
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
--
On Thu, 21 Feb 2008 23:01:24 +0200 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) --
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 --
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
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
--
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 --
