* Cyrill Gorcunov <[email protected]> wrote:
> +/* Comparision type */
> + * We don't expose real in-memory order of objects for security
> + * reasons, still the comparision results should be suitable for
> + * sorting. Thus, we obfuscate kernel pointers values (using random
> + * cookies obtaned at early boot stage) and compare the production
> + * instead.
> + * 0 - equal
> + * 1 - less than
> + * 2 - greater than
> + * 3 - not equal but ordering unavailable (reserved for future)
Broken spelling in each of those comment blocks. Are these
comments write-only?
> + /*
> + * Tasks are looked up in caller's
> + * PID namespace only.
> + */
Could be a single line.
> +
> + task1 = find_task_by_vpid(pid1);
> + if (!task1) {
> + rcu_read_unlock();
> + return -ESRCH;
> + }
> +
> + task2 = find_task_by_vpid(pid2);
> + if (!task2) {
> + put_task_struct(task1);
> + rcu_read_unlock();
> + return -ESRCH;
> + }
This is not the standard pattern of how we do error paths ...
> + /*
> + * Note for all cases but the KCMP_FILE we
> + * don't take any locks in a sake of speed.
> + */
Spelling.
> + get_random_bytes(&cookies[i][j],
> + sizeof(cookies[i][j]));
ugly line break.
> +late_initcall(kcmp_cookie_init);
any particular reason why this needs to be a late initcall?
> --- /dev/null
> +++ linux-2.6.git/tools/testing/selftests/kcmp/Makefile
> @@ -0,0 +1,35 @@
> +ifeq ($(strip $(V)),)
> + E = @echo
> + Q = @
> +else
> + E = @\#
> + Q =
> +endif
> +export E Q
> +
> +uname_M := $(shell uname -m 2>/dev/null || echo not)
> +ARCH ?= $(shell echo $(uname_M) | sed -e s/i.86/i386/)
> +ifeq ($(ARCH),i386)
> + ARCH := X86
> + CFLAGS := -DCONFIG_X86_32
> +endif
> +ifeq ($(ARCH),x86_64)
> + ARCH := X86
> + CFLAGS := -DCONFIG_X86_64
> +endif
> +
> +CFLAGS += -I../../../../arch/x86/include/generated/
> +CFLAGS += -I../../../../include/
> +
> +all:
> +ifeq ($(ARCH),X86)
> + $(E) " CC run_test"
> + $(Q) gcc $(CFLAGS) kcmp_test.c -o run_test
> +else
> + $(E) "Not an x86 target, can't build breakpoints selftests"
> +endif
> +
> +clean:
> + $(E) " CLEAN"
> + $(Q) rm -fr ./run_test
> + $(Q) rm -fr ./test-file
Needs buy-in from the kbuild guys.
> +#ifdef CONFIG_X86_64
> +#include <asm/unistd_64.h>
> +#else
> +#include <asm/unistd_32.h>
> +#endif
Why is asm/unistd.h not good?
> +static long sys_kcmp(int pid1, int pid2, int type, int fd1, int fd2)
> +{
> + return syscall(__NR_kcmp, (long)pid1, (long)pid2,
> + (long)type, (long)fd1, (long)fd2);
> +}
Why is a syscall that takes long arguments defined and called
with int and then cast over to long again?
> + int pid2 = getpid();
> + int ret;
> +
> + fd2 = open("test-file", O_RDWR, 0644);
> + if (fd2 < 0) {
> + perror("Can't open file");
> + exit(1);
> + }
> +
> + /* An example of output and arguments */
> + printf("pid1: %6d pid2: %6d FD: %2d FILES: %2d VM: %2d FS: %2d "
> + "SIGHAND: %2d IO: %2d SYSVSEM: %2d INV: %2d\n",
Visibly stray whitespaces.
> + /* This one should return same fd */
> + ret = sys_kcmp(pid1, pid2, KCMP_FILE, fd1, fd1);
> + if (ret) {
> + printf("FAIL: 0 expected but %d returned\n", ret);
> + ret = -1;
> + } else
> + printf("PASS: 0 returned as expected\n");
> + exit(ret);
this is main(), what's wrong with the standard pattern of return
ret?
I don't know whether this code is correct, but the high amount
of basic cleanliness problems makes me worry about that.
Thanks,
Ingo
On Fri, Feb 03, 2012 at 08:46:56AM +0100, Ingo Molnar wrote:
>
> * Cyrill Gorcunov <[email protected]> wrote:
>
> > +/* Comparision type */
>
> > + * We don't expose real in-memory order of objects for security
> > + * reasons, still the comparision results should be suitable for
> > + * sorting. Thus, we obfuscate kernel pointers values (using random
> > + * cookies obtaned at early boot stage) and compare the production
> > + * instead.
>
> > + * 0 - equal
> > + * 1 - less than
> > + * 2 - greater than
> > + * 3 - not equal but ordering unavailable (reserved for future)
>
> Broken spelling in each of those comment blocks. Are these
> comments write-only?
No, they are not write-only. I've fixed typos in first comment block,
though I don't understand what is wrong with 0,1,2,3 comments.
>
> > + /*
> > + * Tasks are looked up in caller's
> > + * PID namespace only.
> > + */
>
> Could be a single line.
>
Ok, will do so.
> > +
> > + task1 = find_task_by_vpid(pid1);
> > + if (!task1) {
> > + rcu_read_unlock();
> > + return -ESRCH;
> > + }
> > +
> > + task2 = find_task_by_vpid(pid2);
> > + if (!task2) {
> > + put_task_struct(task1);
> > + rcu_read_unlock();
> > + return -ESRCH;
> > + }
>
> This is not the standard pattern of how we do error paths ...
OK, I'll try to make it in standart way.
>
> > + /*
> > + * Note for all cases but the KCMP_FILE we
> > + * don't take any locks in a sake of speed.
> > + */
>
> Spelling.
Not sure what you mean here, but I'll drop this comment
to eliminate this problem.
>
> > + get_random_bytes(&cookies[i][j],
> > + sizeof(cookies[i][j]));
>
> ugly line break.
>
Why? Looks pretty good to me. But sure I'll change it.
> > +late_initcall(kcmp_cookie_init);
>
> any particular reason why this needs to be a late initcall?
>
Grr! The late_initcall remained here from versions where I've
been playing with crypto hashes. Thanks, Ingo, I'll fix!
> > +
> > +clean:
> > + $(E) " CLEAN"
> > + $(Q) rm -fr ./run_test
> > + $(Q) rm -fr ./test-file
>
> Needs buy-in from the kbuild guys.
I took breakpoint test as example. Maybe I should
send this test case code as a separate patch?
>
> > +#ifdef CONFIG_X86_64
> > +#include <asm/unistd_64.h>
> > +#else
> > +#include <asm/unistd_32.h>
> > +#endif
>
> Why is asm/unistd.h not good?
>
With asm/unistd.h it fails to build because it requires the headers
to be installed first (ie headers_install target) so I though this
way would be more convenient, no?
> > +static long sys_kcmp(int pid1, int pid2, int type, int fd1, int fd2)
> > +{
> > + return syscall(__NR_kcmp, (long)pid1, (long)pid2,
> > + (long)type, (long)fd1, (long)fd2);
> > +}
>
> Why is a syscall that takes long arguments defined and called
> with int and then cast over to long again?
>
Just a habit, the args will be converted to long anyway,
so I don't see a problem here. Still I can drop them.
> > + int pid2 = getpid();
> > + int ret;
> > +
> > + fd2 = open("test-file", O_RDWR, 0644);
> > + if (fd2 < 0) {
> > + perror("Can't open file");
> > + exit(1);
> > + }
> > +
> > + /* An example of output and arguments */
> > + printf("pid1: %6d pid2: %6d FD: %2d FILES: %2d VM: %2d FS: %2d "
> > + "SIGHAND: %2d IO: %2d SYSVSEM: %2d INV: %2d\n",
>
> Visibly stray whitespaces.
>
> > + /* This one should return same fd */
> > + ret = sys_kcmp(pid1, pid2, KCMP_FILE, fd1, fd1);
> > + if (ret) {
> > + printf("FAIL: 0 expected but %d returned\n", ret);
> > + ret = -1;
> > + } else
> > + printf("PASS: 0 returned as expected\n");
> > + exit(ret);
>
> this is main(), what's wrong with the standard pattern of return
> ret?
>
It's fork'ed children.
> I don't know whether this code is correct, but the high amount
> of basic cleanliness problems makes me worry about that.
>
Code is correct. I'll clean up the nits you pointed.
Cyrill
* Cyrill Gorcunov <[email protected]> wrote:
> > > + get_random_bytes(&cookies[i][j],
> > > + sizeof(cookies[i][j]));
> >
> > ugly line break.
> >
>
> Why? Looks pretty good to me. But sure I'll change it.
It's ugly because it serves no purpose other than pacifying
checkpatch and makes the code *uglier*.
It's a disease. When checkpatch tells you "this line is too
long" then consider it a code cleanliness warning!
And code
readability and cleanliness
is not improved
by random line-
breaks, right?
'breaking the line' is the *wrong fix* in roughly 90% of the
cases.
So instead of dumbly breaking the line you need to think about
*WHY* the line got too long, not just mechanically work around
the checkpatch warning!
Too long lines can have many reasons, it's usually one of
several reasons:
- too much nesting due to too large function.
solution: break up the function
or: - too verbose statements with not enough abbreviation
solution: find a more compact way to write it
or if the code looks compact enough and is not over-nested then
*leave the line alone*. By breaking it you have not improved -
you have made it worse.
Thanks,
Ingo
On Fri, 3 Feb 2012 10:09:29 +0100 Ingo Molnar <[email protected]> wrote:
>
> * Cyrill Gorcunov <[email protected]> wrote:
>
> > > > + get_random_bytes(&cookies[i][j],
> > > > + sizeof(cookies[i][j]));
> > >
> > > ugly line break.
> > >
> >
> > Why? Looks pretty good to me. But sure I'll change it.
>
> It's ugly because it serves no purpose other than pacifying
> checkpatch and makes the code *uglier*.
No it doesn't. For 80-col displays the code is *already wrapped*. And
that wrapping to column 0 is vastly worse than the above.
If we want to increase the standard to (say) 96 cols then fine, I'd be
happy with that. But until we do that we should not create such a
gruesome mess for those who use 80 cols.
> It's a disease. When checkpatch tells you "this line is too
> long" then consider it a code cleanliness warning!
Well yes, if it can be fixed by other means then great.
On Fri, Feb 03, 2012 at 01:22:41AM -0800, Andrew Morton wrote:
> On Fri, 3 Feb 2012 10:09:29 +0100 Ingo Molnar <[email protected]> wrote:
>
> >
> > * Cyrill Gorcunov <[email protected]> wrote:
> >
> > > > > + get_random_bytes(&cookies[i][j],
> > > > > + sizeof(cookies[i][j]));
> > > >
> > > > ugly line break.
> > > >
> > >
> > > Why? Looks pretty good to me. But sure I'll change it.
> >
> > It's ugly because it serves no purpose other than pacifying
> > checkpatch and makes the code *uglier*.
>
> No it doesn't. For 80-col displays the code is *already wrapped*. And
> that wrapping to column 0 is vastly worse than the above.
>
> If we want to increase the standard to (say) 96 cols then fine, I'd be
> happy with that. But until we do that we should not create such a
> gruesome mess for those who use 80 cols.
>
> > It's a disease. When checkpatch tells you "this line is too
> > long" then consider it a code cleanliness warning!
>
> Well yes, if it can be fixed by other means then great.
>
Guys, I simply made it as
static __init int kcmp_cookie_init(void)
{
int i, j;
for (i = 0; i < KCMP_TYPES; i++) {
for (j = 0; j < 2; j++)
get_random_bytes(&cookies[i][j], sizeof(long));
cookies[i][1] |= (~(~0UL >> 1) | 1);
}
return 0;
}
I thought in first place that sizeof(cookies[i][j]) would allow me
to change type of cookies in one place (ie at declaration) but
if cookies type will be changed -- the code will need careful review
anway, so sizeof(long) will be enough I think.
On the other hands, maybe more clean variant will be
static __init int kcmp_cookie_init(void)
{
const int size = sizeof(cookies[0][0]);
int i, j;
for (i = 0; i < KCMP_TYPES; i++) {
for (j = 0; j < 2; j++)
get_random_bytes(&cookies[i][j], size);
cookies[i][1] |= (~(~0UL >> 1) | 1);
}
return 0;
}
Hm?
Cyrill
* Andrew Morton <[email protected]> wrote:
> On Fri, 3 Feb 2012 10:09:29 +0100 Ingo Molnar <[email protected]> wrote:
>
> >
> > * Cyrill Gorcunov <[email protected]> wrote:
> >
> > > > > + get_random_bytes(&cookies[i][j],
> > > > > + sizeof(cookies[i][j]));
> > > >
> > > > ugly line break.
> > > >
> > >
> > > Why? Looks pretty good to me. But sure I'll change it.
> >
> > It's ugly because it serves no purpose other than pacifying
> > checkpatch and makes the code *uglier*.
>
> No it doesn't. For 80-col displays the code is *already
> wrapped*. And that wrapping to column 0 is vastly worse than
> the above.
Have you actually checked how this actual line would look like
in an 80 cols terminal, if not broken up? I have, it's exactly
80 cols so it looks just fine.
( It was probably broken up when it was longer and then left
this way - making things permanently worse not just by the
linebreak but also by the unnecessary curly braces around the
inner loop. )
But more importantly, even if the line was genuinely longer, how
many people are looking at things in an 80-col display? By my
experience, from looking at what kinds of terminals kernel
people use, it's below 1%. (I was one of the last ones holding
out because text consoles are so much faster than just about any
usable xterm app - but I switched to a larger terminal some two
years ago.)
Shouldnt't we concentrate on the 99% case which gets uglified by
the systematic linebreaks?
Also, there are clearly cases where breaking the line
intelligently improves things. Such as:
+ /* An example of output and arguments */
+ printf("pid1: %6d pid2: %6d FD: %2d FILES: %2d VM: %2d FS: %2d "
+ "SIGHAND: %2d IO: %2d SYSVSEM: %2d INV: %2d\n",
+ pid1, pid2,
+ sys_kcmp(pid1, pid2, KCMP_FILE, fd1, fd2),
+ sys_kcmp(pid1, pid2, KCMP_FILES, 0, 0),
+ sys_kcmp(pid1, pid2, KCMP_VM, 0, 0),
+ sys_kcmp(pid1, pid2, KCMP_FS, 0, 0),
+ sys_kcmp(pid1, pid2, KCMP_SIGHAND, 0, 0),
+ sys_kcmp(pid1, pid2, KCMP_IO, 0, 0),
+ sys_kcmp(pid1, pid2, KCMP_SYSVSEM, 0, 0),
+
+ /* This one should fail */
+ sys_kcmp(pid1, pid2, KCMP_TYPES + 1, 0, 0));
this is vastly more readable because the arguments are lined up
vertically not just at the beginning but nicely tabulated along
the way.
Oh, and note that
Breaking lines is a tool that should be used on a case by case
basis, not a hard limit.
> If we want to increase the standard to (say) 96 cols then
> fine, I'd be happy with that. But until we do that we should
> not create such a gruesome mess for those who use 80 cols.
The kernel has *already* become a gruesome mess for 80 col users
long ago. That was the main reason why I stopped using 80 col
terminals two years ago ...
So lets stop the pretense.
> > It's a disease. When checkpatch tells you "this line is too
> > long" then consider it a code cleanliness warning!
>
> Well yes, if it can be fixed by other means then great.
Yes it can.
Thanks,
Ingo
* Ingo Molnar <[email protected]> wrote:
> > If we want to increase the standard to (say) 96 cols then
> > fine, I'd be happy with that. But until we do that we
> > should not create such a gruesome mess for those who use 80
> > cols.
>
> The kernel has *already* become a gruesome mess for 80 col
> users long ago. That was the main reason why I stopped using
> 80 col terminals two years ago ...
>
> So lets stop the pretense.
in other words:
--------------->
[PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums
The overwhelming majority of kernel developers have stopped
using 80 col terminals years ago.
As far as I'm aware I was the last regular kernel contributor
who still used a standard VGA text console, but both text
consoles and using them to read the kernel source code has
become increasingly gruesome years ago so I switched to a wider
terminal two years ago.
Worse than that, people are actively uglifying the kernel code
to fit things into 80 cols mechanically. They are using
checkpatch and are interpreting the 80 col warnings the wrong
way again and again, sucking up reviewer bandwidth that could be
utilized better.
So lets increase the limit to 100 cols - this is a nice round
limit, and it also happens to match with most developer xterm
sizes. Code that goes over 100 cols for no good reasons will be
arguably something worth fixing. (100 cols is also arguably
closer to various brain limits such as vision of field and
resolution restrictions, so we'll likely not have to increase
this limit for a couple of million years, for all retro human
genome users.)
Signed-off-by: Ingo Molnar <[email protected]>
---
diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
index 2b90d32..e87370f 100644
--- a/Documentation/CodingStyle
+++ b/Documentation/CodingStyle
@@ -27,7 +27,7 @@ how the indentation works if you have large indentations.
Now, some people will claim that having 8-character indentations makes
the code move too far to the right, and makes it hard to read on a
-80-character terminal screen. The answer to that is that if you need
+100-character terminal screen. The answer to that is that if you need
more than 3 levels of indentation, you're screwed anyway, and should fix
your program.
@@ -77,11 +77,11 @@ Get a decent editor and don't leave whitespace at the end of lines.
Coding style is all about readability and maintainability using commonly
available tools.
-The limit on the length of lines is 80 columns and this is a strongly
+The limit on the length of lines is 100 columns and this is a strongly
preferred limit.
-Statements longer than 80 columns will be broken into sensible chunks, unless
-exceeding 80 columns significantly increases readability and does not hide
+Statements longer than 100 columns will be broken into sensible chunks, unless
+exceeding 100 columns significantly increases readability and does not hide
information. Descendants are always substantially shorter than the parent and
are placed substantially to the right. The same applies to function headers
with a long argument list. However, never break user-visible strings such as
@@ -344,8 +344,7 @@ be directly accessed should _never_ be a typedef.
Chapter 6: Functions
Functions should be short and sweet, and do just one thing. They should
-fit on one or two screenfuls of text (the ISO/ANSI screen size is 80x24,
-as we all know), and do one thing and do that well.
+fit on one or two screenfuls of text, and do one thing and do that well.
The maximum length of a function is inversely proportional to the
complexity and indentation level of that function. So, if you have a
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e3bfcbe..2d0f093 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1726,15 +1726,14 @@ sub process {
# check we are in a valid source file if not then ignore this hunk
next if ($realfile !~ /\.(h|c|s|S|pl|sh)$/);
-#80 column limit
+#100 column limit
if ($line =~ /^\+/ && $prevrawline !~ /\/\*\*/ &&
$rawline !~ /^.\s*\*\s*\@$Ident\s/ &&
!($line =~ /^\+\s*$logFunctions\s*\(\s*(?:(KERN_\S+\s*|[^"]*))?"[X\t]*"\s*(?:|,|\)\s*;)\s*$/ ||
$line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) &&
- $length > 80)
+ $length > 100)
{
- WARN("LONG_LINE",
- "line over 80 characters\n" . $herecurr);
+ WARN("LONG_LINE", "line over 100 characters\n" . $herecurr);
}
# check for spaces before a quoted newline
On Fri, Feb 3, 2012 at 12:07 PM, Ingo Molnar <[email protected]> wrote:
>> The kernel has *already* become a gruesome mess for 80 col
>> users long ago. That was the main reason why I stopped using
>> 80 col terminals two years ago ...
>>
>> So lets stop the pretense.
>
> in other words:
>
> --------------->
> [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums
>
> The overwhelming majority of kernel developers have stopped
> using 80 col terminals years ago.
>
> As far as I'm aware I was the last regular kernel contributor
> who still used a standard VGA text console, but both text
> consoles and using them to read the kernel source code has
> become increasingly gruesome years ago so I switched to a wider
> terminal two years ago.
>
> Worse than that, people are actively uglifying the kernel code
> to fit things into 80 cols mechanically. They are using
> checkpatch and are interpreting the 80 col warnings the wrong
> way again and again, sucking up reviewer bandwidth that could be
> utilized better.
>
> So lets increase the limit to 100 cols - this is a nice round
> limit, and it also happens to match with most developer xterm
> sizes. Code that goes over 100 cols for no good reasons will be
> arguably something worth fixing. (100 cols is also arguably
> closer to various brain limits such as vision of field and
> resolution restrictions, so we'll likely not have to increase
> this limit for a couple of million years, for all retro human
> genome users.)
>
> Signed-off-by: Ingo Molnar <[email protected]>
The rationale in the changelog is somewhat over the top but I agree
with the change :-)
Acked-by: Pekka Enberg <[email protected]>
On Fri, Feb 03, 2012 at 12:17:43PM +0200, Pekka Enberg wrote:
> >
> > Signed-off-by: Ingo Molnar <[email protected]>
>
> The rationale in the changelog is somewhat over the top but I agree
> with the change :-)
>
> Acked-by: Pekka Enberg <[email protected]>
>
FWIW, agreed as well, thanks Ingo!
Acked-by: Cyrill Gorcunov <[email protected]>
Cyrill
On Fri, Feb 3, 2012 at 1:07 PM, Ingo Molnar <[email protected]> wrote:
> [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums
Better don't mention line length at all.
Hello,
On Fri, Feb 03, 2012 at 11:07:43AM +0100, Ingo Molnar wrote:
>
> * Ingo Molnar <[email protected]> wrote:
>
> > > If we want to increase the standard to (say) 96 cols then
> > > fine, I'd be happy with that. But until we do that we
> > > should not create such a gruesome mess for those who use 80
> > > cols.
> >
> > The kernel has *already* become a gruesome mess for 80 col
> > users long ago. That was the main reason why I stopped using
> > 80 col terminals two years ago ...
> >
> > So lets stop the pretense.
I don't know. In my experience, a lot of code, especially core part,
mostly follows 80 col limit. It shouldn't be too difficult to write
up a script to count >80col lines in different parts of the kernel.
> [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums
>
> The overwhelming majority of kernel developers have stopped
> using 80 col terminals years ago.
>
> As far as I'm aware I was the last regular kernel contributor
> who still used a standard VGA text console, but both text
> consoles and using them to read the kernel source code has
> become increasingly gruesome years ago so I switched to a wider
> terminal two years ago.
People usually place multiple windows horizontally so it's not like
all those extra pixels go wasted. 80col might even have the benefit
of giving overall higher density in terms of pixel usage.
> Worse than that, people are actively uglifying the kernel code
> to fit things into 80 cols mechanically. They are using
> checkpatch and are interpreting the 80 col warnings the wrong
> way again and again, sucking up reviewer bandwidth that could be
> utilized better.
>
> So lets increase the limit to 100 cols - this is a nice round
> limit, and it also happens to match with most developer xterm
> sizes. Code that goes over 100 cols for no good reasons will be
> arguably something worth fixing. (100 cols is also arguably
> closer to various brain limits such as vision of field and
> resolution restrictions, so we'll likely not have to increase
> this limit for a couple of million years, for all retro human
> genome users.)
That said, yeah, 80col is a pain in the ass and lessening the pressure
a bit might make it a non-problem and 100 is one of the nicer numbers
which aren't power of two.
For me, the biggest reason to stick to 80col has been that, while
being widely disliked, it still was the most common limit people were
using and consistency tends to be more beneficial on these issues. If
we're gonna do this, and I hope we do, let's proactively encourage /
enforce it - ie. let's collectively nag so that 100col quickly becomes
the standard.
Acked-by: Tejun Heo <[email protected]>
Thanks.
--
tejun
96 might be a better limit. Why? Because 80 and 96 are the easiest line lengths to achieve on a printer. 80 is typically the default, but with a 12 cpi font you get 96.
Tejun Heo <[email protected]> wrote:
>Hello,
>
>On Fri, Feb 03, 2012 at 11:07:43AM +0100, Ingo Molnar wrote:
>>
>> * Ingo Molnar <[email protected]> wrote:
>>
>> > > If we want to increase the standard to (say) 96 cols then
>> > > fine, I'd be happy with that. But until we do that we
>> > > should not create such a gruesome mess for those who use 80
>> > > cols.
>> >
>> > The kernel has *already* become a gruesome mess for 80 col
>> > users long ago. That was the main reason why I stopped using
>> > 80 col terminals two years ago ...
>> >
>> > So lets stop the pretense.
>
>I don't know. In my experience, a lot of code, especially core part,
>mostly follows 80 col limit. It shouldn't be too difficult to write
>up a script to count >80col lines in different parts of the kernel.
>
>> [PATCH] SubmittingPatches: Increase the line length limit from 80 to
>100 colums
>>
>> The overwhelming majority of kernel developers have stopped
>> using 80 col terminals years ago.
>>
>> As far as I'm aware I was the last regular kernel contributor
>> who still used a standard VGA text console, but both text
>> consoles and using them to read the kernel source code has
>> become increasingly gruesome years ago so I switched to a wider
>> terminal two years ago.
>
>People usually place multiple windows horizontally so it's not like
>all those extra pixels go wasted. 80col might even have the benefit
>of giving overall higher density in terms of pixel usage.
>
>> Worse than that, people are actively uglifying the kernel code
>> to fit things into 80 cols mechanically. They are using
>> checkpatch and are interpreting the 80 col warnings the wrong
>> way again and again, sucking up reviewer bandwidth that could be
>> utilized better.
>>
>> So lets increase the limit to 100 cols - this is a nice round
>> limit, and it also happens to match with most developer xterm
>> sizes. Code that goes over 100 cols for no good reasons will be
>> arguably something worth fixing. (100 cols is also arguably
>> closer to various brain limits such as vision of field and
>> resolution restrictions, so we'll likely not have to increase
>> this limit for a couple of million years, for all retro human
>> genome users.)
>
>That said, yeah, 80col is a pain in the ass and lessening the pressure
>a bit might make it a non-problem and 100 is one of the nicer numbers
>which aren't power of two.
>
>For me, the biggest reason to stick to 80col has been that, while
>being widely disliked, it still was the most common limit people were
>using and consistency tends to be more beneficial on these issues. If
>we're gonna do this, and I hope we do, let's proactively encourage /
>enforce it - ie. let's collectively nag so that 100col quickly becomes
>the standard.
>
>Acked-by: Tejun Heo <[email protected]>
>
>Thanks.
>
>--
>tejun
--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
On 02/03/2012 01:28 AM, Cyrill Gorcunov wrote:
> static __init int kcmp_cookie_init(void)
> {
> int i, j;
>
> for (i = 0; i< KCMP_TYPES; i++) {
> for (j = 0; j< 2; j++)
> get_random_bytes(&cookies[i][j], sizeof(long));
> cookies[i][1] |= (~(~0UL>> 1) | 1);
> }
>
> return 0;
> }
>
> I thought in first place that sizeof(cookies[i][j]) would allow me
> to change type of cookies in one place (ie at declaration) but
> if cookies type will be changed -- the code will need careful review
> anway, so sizeof(long) will be enough I think.
>
> On the other hands, maybe more clean variant will be
>
> static __init int kcmp_cookie_init(void)
> {
> const int size = sizeof(cookies[0][0]);
> int i, j;
>
> for (i = 0; i< KCMP_TYPES; i++) {
> for (j = 0; j< 2; j++)
> get_random_bytes(&cookies[i][j], size);
> cookies[i][1] |= (~(~0UL>> 1) | 1);
> }
>
How about:
const int size = sizeof(cookies[0]);
get_random_bytes(&cookies[i], size);
... and skip the completely unnecessary for loop?
-hpa
On 02/03/2012 09:32 AM, H. Peter Anvin wrote:
> On 02/03/2012 01:28 AM, Cyrill Gorcunov wrote:
>> static __init int kcmp_cookie_init(void)
>> {
>> int i, j;
>>
>> for (i = 0; i< KCMP_TYPES; i++) {
>> for (j = 0; j< 2; j++)
>> get_random_bytes(&cookies[i][j], sizeof(long));
>> cookies[i][1] |= (~(~0UL>> 1) | 1);
>> }
>>
>> return 0;
>> }
>>
>> I thought in first place that sizeof(cookies[i][j]) would allow me
>> to change type of cookies in one place (ie at declaration) but
>> if cookies type will be changed -- the code will need careful review
>> anway, so sizeof(long) will be enough I think.
>>
>> On the other hands, maybe more clean variant will be
>>
>> static __init int kcmp_cookie_init(void)
>> {
>> const int size = sizeof(cookies[0][0]);
>> int i, j;
>>
>> for (i = 0; i< KCMP_TYPES; i++) {
>> for (j = 0; j< 2; j++)
>> get_random_bytes(&cookies[i][j], size);
>> cookies[i][1] |= (~(~0UL>> 1) | 1);
>> }
>>
>
> How about:
>
> const int size = sizeof(cookies[0]);
>
> get_random_bytes(&cookies[i], size);
>
> ... and skip the completely unnecessary for loop?
>
Even better:
static __init int kcmp_cookie_init(void)
{
int i;
get_random_bytes(cookies, sizeof cookies);
for (i = 0; i < KCMP_TYPES; i++)
cookies[i][1] |= ~(~0UL >> 1) | 1;
}
On Fri, Feb 03, 2012 at 09:35:05AM -0800, H. Peter Anvin wrote:
...
> >How about:
> >
> >const int size = sizeof(cookies[0]);
> >
> >get_random_bytes(&cookies[i], size);
> >
> >... and skip the completely unnecessary for loop?
> >
>
> Even better:
>
> static __init int kcmp_cookie_init(void)
> {
> int i;
>
> get_random_bytes(cookies, sizeof cookies);
>
> for (i = 0; i < KCMP_TYPES; i++)
> cookies[i][1] |= ~(~0UL >> 1) | 1;
> }
>
Oh, cool! It's a way simplier (I somehow forgot that
get_random_bytes operates with stream of bytes).
Cyrill
---
From: Cyrill Gorcunov <[email protected]>
Subject: syscalls, x86: Add __NR_kcmp syscall v8
While doing the checkpoint-restore in the user space one need to determine
whether various kernel objects (like mm_struct-s of file_struct-s) are shared
between tasks and restore this state.
The 2nd step can be solved by using appropriate CLONE_ flags and the unshare
syscall, while there's currently no ways for solving the 1st one.
One of the ways for checking whether two tasks share e.g. mm_struct is to
provide some mm_struct ID of a task to its proc file, but showing such
info considered to be not that good for security reasons.
Thus after some debates we end up in conclusion that using that named
'comparison' syscall might be the best candidate. So here is it --
__NR_kcmp.
It takes up to 5 arguments - the pids of the two tasks (which
characteristics should be compared), the comparison type and
(in case of comparison of files) two file descriptors.
Lookups for pids are done in the caller's PID namespace only.
At moment only x86 is supported and tested.
Signed-off-by: Cyrill Gorcunov <[email protected]>
CC: "Eric W. Biederman" <[email protected]>
CC: Pavel Emelyanov <[email protected]>
CC: Andrey Vagin <[email protected]>
CC: KOSAKI Motohiro <[email protected]>
CC: Ingo Molnar <[email protected]>
CC: H. Peter Anvin <[email protected]>
CC: Thomas Gleixner <[email protected]>
CC: Glauber Costa <[email protected]>
CC: Andi Kleen <[email protected]>
CC: Tejun Heo <[email protected]>
CC: Matt Helsley <[email protected]>
CC: Pekka Enberg <[email protected]>
CC: Eric Dumazet <[email protected]>
CC: Vasiliy Kulikov <[email protected]>
CC: Andrew Morton <[email protected]>
CC: Alexey Dobriyan <[email protected]>
CC: [email protected]
CC: Michal Marek <[email protected]>
---
arch/x86/syscalls/syscall_32.tbl | 1
arch/x86/syscalls/syscall_64.tbl | 1
include/linux/kcmp.h | 17 +++
include/linux/syscalls.h | 2
kernel/Makefile | 3
kernel/kcmp.c | 155 +++++++++++++++++++++++++++++++
kernel/sys_ni.c | 3
tools/testing/selftests/kcmp/Makefile | 36 +++++++
tools/testing/selftests/kcmp/kcmp_test.c | 84 ++++++++++++++++
tools/testing/selftests/run_tests | 2
10 files changed, 303 insertions(+), 1 deletion(-)
Index: linux-2.6.git/arch/x86/syscalls/syscall_32.tbl
===================================================================
--- linux-2.6.git.orig/arch/x86/syscalls/syscall_32.tbl
+++ linux-2.6.git/arch/x86/syscalls/syscall_32.tbl
@@ -355,3 +355,4 @@
346 i386 setns sys_setns
347 i386 process_vm_readv sys_process_vm_readv compat_sys_process_vm_readv
348 i386 process_vm_writev sys_process_vm_writev compat_sys_process_vm_writev
+349 i386 kcmp sys_kcmp
Index: linux-2.6.git/arch/x86/syscalls/syscall_64.tbl
===================================================================
--- linux-2.6.git.orig/arch/x86/syscalls/syscall_64.tbl
+++ linux-2.6.git/arch/x86/syscalls/syscall_64.tbl
@@ -318,3 +318,4 @@
309 64 getcpu sys_getcpu
310 64 process_vm_readv sys_process_vm_readv
311 64 process_vm_writev sys_process_vm_writev
+312 64 kcmp sys_kcmp
Index: linux-2.6.git/include/linux/kcmp.h
===================================================================
--- /dev/null
+++ linux-2.6.git/include/linux/kcmp.h
@@ -0,0 +1,17 @@
+#ifndef _LINUX_KCMP_H
+#define _LINUX_KCMP_H
+
+/* Comparison type */
+enum kcmp_type {
+ KCMP_FILE,
+ KCMP_VM,
+ KCMP_FILES,
+ KCMP_FS,
+ KCMP_SIGHAND,
+ KCMP_IO,
+ KCMP_SYSVSEM,
+
+ KCMP_TYPES,
+};
+
+#endif /* _LINUX_KCMP_H */
Index: linux-2.6.git/include/linux/syscalls.h
===================================================================
--- linux-2.6.git.orig/include/linux/syscalls.h
+++ linux-2.6.git/include/linux/syscalls.h
@@ -857,4 +857,6 @@ asmlinkage long sys_process_vm_writev(pi
unsigned long riovcnt,
unsigned long flags);
+asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
+ unsigned long idx1, unsigned long idx2);
#endif
Index: linux-2.6.git/kernel/Makefile
===================================================================
--- linux-2.6.git.orig/kernel/Makefile
+++ linux-2.6.git/kernel/Makefile
@@ -25,6 +25,9 @@ endif
obj-y += sched/
obj-y += power/
+ifeq ($(CONFIG_CHECKPOINT_RESTORE),y)
+obj-$(CONFIG_X86) += kcmp.o
+endif
obj-$(CONFIG_FREEZER) += freezer.o
obj-$(CONFIG_PROFILING) += profile.o
obj-$(CONFIG_SYSCTL_SYSCALL_CHECK) += sysctl_check.o
Index: linux-2.6.git/kernel/kcmp.c
===================================================================
--- /dev/null
+++ linux-2.6.git/kernel/kcmp.c
@@ -0,0 +1,155 @@
+#include <linux/kernel.h>
+#include <linux/syscalls.h>
+#include <linux/fdtable.h>
+#include <linux/string.h>
+#include <linux/random.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/cache.h>
+#include <linux/bug.h>
+#include <linux/err.h>
+#include <linux/kcmp.h>
+
+#include <asm/unistd.h>
+
+/*
+ * We don't expose real in-memory order of objects for security
+ * reasons, still the comparison results should be suitable for
+ * sorting. Thus, we obfuscate kernel pointers values and compare
+ * the production instead.
+ */
+static unsigned long cookies[KCMP_TYPES][2] __read_mostly;
+
+static long kptr_obfuscate(long v, int type)
+{
+ return (v ^ cookies[type][0]) * cookies[type][1];
+}
+
+/*
+ * 0 - equal, i.e. v1 = v2
+ * 1 - less than, i.e. v1 < v2
+ * 2 - greater than, i.e. v1 > v2
+ * 3 - not equal but ordering unavailable (reserved for future)
+ */
+static int kcmp_ptr(void *v1, void *v2, enum kcmp_type type)
+{
+ long ret;
+
+ ret = kptr_obfuscate((long)v1, type) - kptr_obfuscate((long)v2, type);
+
+ return (ret < 0) | ((ret > 0) << 1);
+}
+
+/* The caller must have pinned the task */
+static struct file *
+get_file_raw_ptr(struct task_struct *task, unsigned int idx)
+{
+ struct fdtable *fdt;
+ struct file *file;
+
+ spin_lock(&task->files->file_lock);
+ fdt = files_fdtable(task->files);
+ if (idx < fdt->max_fds)
+ file = fdt->fd[idx];
+ else
+ file = NULL;
+ spin_unlock(&task->files->file_lock);
+
+ return file;
+}
+
+SYSCALL_DEFINE5(kcmp, pid_t, pid1, pid_t, pid2, int, type,
+ unsigned long, idx1, unsigned long, idx2)
+{
+ struct task_struct *task1, *task2;
+ int ret;
+
+ rcu_read_lock();
+
+ /*
+ * Tasks are looked up in caller's PID namespace only.
+ */
+ task1 = find_task_by_vpid(pid1);
+ task2 = find_task_by_vpid(pid2);
+ if (!task1 || !task2)
+ goto err_no_task;
+
+ get_task_struct(task1);
+ get_task_struct(task2);
+
+ rcu_read_unlock();
+
+ /*
+ * One should have enough rights to inspect task details.
+ */
+ if (!ptrace_may_access(task1, PTRACE_MODE_READ) ||
+ !ptrace_may_access(task2, PTRACE_MODE_READ)) {
+ ret = -EACCES;
+ goto err;
+ }
+
+ switch (type) {
+ case KCMP_FILE: {
+ struct file *filp1, *filp2;
+
+ filp1 = get_file_raw_ptr(task1, idx1);
+ filp2 = get_file_raw_ptr(task2, idx2);
+
+ if (filp1 && filp2)
+ ret = kcmp_ptr(filp1, filp2, KCMP_FILE);
+ else
+ ret = -EBADF;
+ break;
+ }
+ case KCMP_VM:
+ ret = kcmp_ptr(task1->mm, task2->mm, KCMP_VM);
+ break;
+ case KCMP_FILES:
+ ret = kcmp_ptr(task1->files, task2->files, KCMP_FILES);
+ break;
+ case KCMP_FS:
+ ret = kcmp_ptr(task1->fs, task2->fs, KCMP_FS);
+ break;
+ case KCMP_SIGHAND:
+ ret = kcmp_ptr(task1->sighand, task2->sighand, KCMP_SIGHAND);
+ break;
+ case KCMP_IO:
+ ret = kcmp_ptr(task1->io_context, task2->io_context, KCMP_IO);
+ break;
+ case KCMP_SYSVSEM:
+#ifdef CONFIG_SYSVIPC
+ ret = kcmp_ptr(task1->sysvsem.undo_list,
+ task2->sysvsem.undo_list,
+ KCMP_SYSVSEM);
+#else
+ ret = -EOPNOTSUP;
+#endif
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+err:
+ put_task_struct(task1);
+ put_task_struct(task2);
+
+ return ret;
+
+err_no_task:
+ rcu_read_unlock();
+ return -ESRCH;
+}
+
+static __init int kcmp_cookies_init(void)
+{
+ int i;
+
+ get_random_bytes(cookies, sizeof(cookies));
+
+ for (i = 0; i < KCMP_TYPES; i++)
+ cookies[i][1] |= (~(~0UL >> 1) | 1);
+
+ return 0;
+}
+arch_initcall(kcmp_cookies_init);
Index: linux-2.6.git/kernel/sys_ni.c
===================================================================
--- linux-2.6.git.orig/kernel/sys_ni.c
+++ linux-2.6.git/kernel/sys_ni.c
@@ -203,3 +203,6 @@ cond_syscall(sys_fanotify_mark);
cond_syscall(sys_name_to_handle_at);
cond_syscall(sys_open_by_handle_at);
cond_syscall(compat_sys_open_by_handle_at);
+
+/* compare kernel pointers */
+cond_syscall(sys_kcmp);
Index: linux-2.6.git/tools/testing/selftests/kcmp/Makefile
===================================================================
--- /dev/null
+++ linux-2.6.git/tools/testing/selftests/kcmp/Makefile
@@ -0,0 +1,36 @@
+ifeq ($(strip $(V)),)
+ E = @echo
+ Q = @
+else
+ E = @\#
+ Q =
+endif
+export E Q
+
+uname_M := $(shell uname -m 2>/dev/null || echo not)
+ARCH ?= $(shell echo $(uname_M) | sed -e s/i.86/i386/)
+ifeq ($(ARCH),i386)
+ ARCH := X86
+ CFLAGS := -DCONFIG_X86_32 -D__i386__
+endif
+ifeq ($(ARCH),x86_64)
+ ARCH := X86
+ CFLAGS := -DCONFIG_X86_64 -D__x86_64__
+endif
+
+CFLAGS += -I../../../../arch/x86/include/generated/
+CFLAGS += -I../../../../include/
+CFLAGS += -I../../../../usr/include/
+
+all:
+ifeq ($(ARCH),X86)
+ $(E) " CC run_test"
+ $(Q) gcc $(CFLAGS) kcmp_test.c -o run_test
+else
+ $(E) "Not an x86 target, can't build kcmp selftest"
+endif
+
+clean:
+ $(E) " CLEAN"
+ $(Q) rm -fr ./run_test
+ $(Q) rm -fr ./test-file
Index: linux-2.6.git/tools/testing/selftests/kcmp/kcmp_test.c
===================================================================
--- /dev/null
+++ linux-2.6.git/tools/testing/selftests/kcmp/kcmp_test.c
@@ -0,0 +1,84 @@
+#define _GNU_SOURCE
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <signal.h>
+#include <limits.h>
+#include <unistd.h>
+#include <errno.h>
+#include <string.h>
+#include <fcntl.h>
+
+#include <linux/unistd.h>
+#include <linux/kcmp.h>
+
+#include <sys/syscall.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <sys/wait.h>
+
+static long sys_kcmp(int pid1, int pid2, int type, int fd1, int fd2)
+{
+ return syscall(__NR_kcmp, pid1, pid2, type, fd1, fd2);
+}
+
+int main(int argc, char **argv)
+{
+ const char kpath[] = "kcmp-test-file";
+ int pid1, pid2;
+ int fd1, fd2;
+ int status;
+
+ fd1 = open(kpath, O_RDWR | O_CREAT | O_TRUNC, 0644);
+ pid1 = getpid();
+
+ if (fd1 < 0) {
+ perror("Can't create file");
+ exit(1);
+ }
+
+ pid2 = fork();
+ if (pid2 < 0) {
+ perror("fork failed");
+ exit(1);
+ }
+
+ if (!pid2) {
+ int pid2 = getpid();
+ int ret;
+
+ fd2 = open(kpath, O_RDWR, 0644);
+ if (fd2 < 0) {
+ perror("Can't open file");
+ exit(1);
+ }
+
+ /* An example of output and arguments */
+ printf("pid1: %6d pid2: %6d FD: %2d FILES: %2d VM: %2d FS: %2d "
+ "SIGHAND: %2d IO: %2d SYSVSEM: %2d INV: %2d\n",
+ pid1, pid2,
+ sys_kcmp(pid1, pid2, KCMP_FILE, fd1, fd2),
+ sys_kcmp(pid1, pid2, KCMP_FILES, 0, 0),
+ sys_kcmp(pid1, pid2, KCMP_VM, 0, 0),
+ sys_kcmp(pid1, pid2, KCMP_FS, 0, 0),
+ sys_kcmp(pid1, pid2, KCMP_SIGHAND, 0, 0),
+ sys_kcmp(pid1, pid2, KCMP_IO, 0, 0),
+ sys_kcmp(pid1, pid2, KCMP_SYSVSEM, 0, 0),
+
+ /* This one should fail */
+ sys_kcmp(pid1, pid2, KCMP_TYPES + 1, 0, 0));
+
+ /* This one should return same fd */
+ ret = sys_kcmp(pid1, pid2, KCMP_FILE, fd1, fd1);
+ if (ret) {
+ printf("FAIL: 0 expected but %d returned\n", ret);
+ ret = -1;
+ } else
+ printf("PASS: 0 returned as expected\n");
+ exit(ret);
+ }
+
+ waitpid(pid2, &status, P_ALL);
+
+ return 0;
+}
Index: linux-2.6.git/tools/testing/selftests/run_tests
===================================================================
--- linux-2.6.git.orig/tools/testing/selftests/run_tests
+++ linux-2.6.git/tools/testing/selftests/run_tests
@@ -1,6 +1,6 @@
#!/bin/bash
-TARGETS=breakpoints
+TARGETS="breakpoints kcmp"
for TARGET in $TARGETS
do
> So lets increase the limit to 100 cols - this is a nice round
> limit, and it also happens to match with most developer xterm
> sizes. Code that goes over 100 cols for no good reasons will be
> arguably something worth fixing. (100 cols is also arguably
> closer to various brain limits such as vision of field and
> resolution restrictions, so we'll likely not have to increase
> this limit for a couple of million years, for all retro human
> genome users.)
I suppose the future computer to brain interfaces will just
do away with the concept of lines @)
>
> Signed-off-by: Ingo Molnar <[email protected]>
Acked-by: Andi Kleen <[email protected]>
-Andi
On Fri, 3 Feb 2012 11:07:43 +0100
Ingo Molnar <[email protected]> wrote:
> [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums
>
> The overwhelming majority of kernel developers have stopped
> using 80 col terminals years ago.
>
> As far as I'm aware I was the last regular kernel contributor
> who still used a standard VGA text console, but both text
> consoles and using them to read the kernel source code has
> become increasingly gruesome years ago so I switched to a wider
> terminal two years ago.
I always use 80-cols, everywhere. Not because I particularly like it -
I find it a bit too small. I use it because it is the standard, and
using it helps me see where and how badly we violate the standard.
We've actually done pretty well - linewrap in 80 cols rarely causes me
problems. It's sufficiently rare that when it *does* happen, it really
stands out.
IOW, the changelog is quite the exaggeration.
> So lets increase the limit to 100 cols
I think that's going too far - 96 will be enough and it's a multiple of 8.
The multiple-of-8 thing seems pleasing but probably doesn't matter
much. It means that things like
if (foo) {
if (foo) {
if (foo) {
if (foo) {
if (foo) {
if (foo) {
if (foo) {
if (foo) {
if (foo) {
if (foo) {
if (foo) {
if (foo) {
if (foo) {
if (foo) {
if (foo) {
will line up properly.
If we really want to improve the world we should jump into a time
machine and set tabstops to 4. Sigh.
On 02/03/2012 12:57 PM, Andrew Morton wrote:
>
> If we really want to improve the world we should jump into a time
> machine and set tabstops to 4. Sigh.
>
Most editors these days will happy do 4-space stops expanded to 8-space
tab characters.
I have adopted the "Linux kernel formatting with 4-space indentation"
for most of my projects these days.
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
On 02/03/2012 12:57 PM, Andrew Morton wrote:
>
> I always use 80-cols, everywhere. Not because I particularly like it -
> I find it a bit too small. I use it because it is the standard, and
> using it helps me see where and how badly we violate the standard.
>
Width matters to me more for printing than it does on the screen.
80-column text can be printed on a landscape piece of paper two columns
wide without being too hard to read (e.g. enscript -2r). I haven't
tried that with 96 columns, but more than that would almost certainly be
illegible.
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
On Fri, Feb 3, 2012 at 2:07 AM, Ingo Molnar <[email protected]> wrote:
>
> The overwhelming majority of kernel developers have stopped
> using 80 col terminals years ago.
Quite frankly, I think we should still keep it at 80 columns.
The problem is not the 80 columns, it's that damn patch-check script
that warns about people *occasionally* going over 80 columns.
But usually it's bettre to have the *occasional* 80+ column line, than
try to split it up. So we do have lines that are longer than 80
columns, but that's not because 100 columns is ok - it's because 80+
columns is better than the alternative.
So it's a trade-off. Thinking that there is a hard limit is the
problem. And extending that hard limit (and thinking that it's "ok" to
be over 80 columns) is *also* a problem.
So no, 100-char columns are not ok.
Linus
Overly indented code should be refactored.
Suggest refactoring excessive indentation of
of if/else/for/do/while/switch statements.
For example:
$ cat t.c
#include <stdio.h>
#include <stdlib.h>
int main(int argc, char **argv)
{
if (1)
if (2)
if (3)
if (4)
if (5)
if (6)
if (7)
if (8)
;
return 0;
}
$ ./scripts/checkpatch.pl -f t.c
WARNING: Too many leading tabs - consider code refactoring
#12: FILE: t.c:12:
+ if (6)
WARNING: Too many leading tabs - consider code refactoring
#13: FILE: t.c:13:
+ if (7)
WARNING: Too many leading tabs - consider code refactoring
#14: FILE: t.c:14:
+ if (8)
total: 0 errors, 3 warnings, 17 lines checked
t.c has style problems, please review.
If any of these errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
Signed-off-by: Joe Perches <[email protected]>
---
On Fri, 2012-02-03 at 13:27 -0800, Linus Torvalds wrote:
> So no, 100-char columns are not ok.
Perhaps this is a reasonable alternative.
Another might be to limit variable name length to something
shorter than say 10 characters.
scripts/checkpatch.pl | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2b52aeb..89d24b3 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1924,6 +1924,12 @@ sub process {
my $pre_ctx = "$1$2";
my ($level, @ctx) = ctx_statement_level($linenr, $realcnt, 0);
+
+ if ($line =~ /^\+\t{6,}/) {
+ WARN("DEEP_INDENTATION",
+ "Too many leading tabs - consider code refactoring\n" . $herecurr);
+ }
+
my $ctx_cnt = $realcnt - $#ctx - 1;
my $ctx = join("\n", @ctx);
On 02/03/2012 01:27 PM, Linus Torvalds wrote:
> On Fri, Feb 3, 2012 at 2:07 AM, Ingo Molnar <[email protected]> wrote:
>>
>> The overwhelming majority of kernel developers have stopped
>> using 80 col terminals years ago.
There was more interesting text there. ;)
> Quite frankly, I think we should still keep it at 80 columns.
>
...
>
> So no, 100-char columns are not ok.
Thank you.
--
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
On Fri, Feb 3, 2012 at 3:20 PM, Joe Perches <[email protected]> wrote:
> Overly indented code should be refactored.
>
> Suggest refactoring excessive indentation of
> of if/else/for/do/while/switch statements.
I hate this patch.
Why? Because mindless checks like this would just lead to people
making things worse and intermixing spaces there instead.
That's the same reason the 80-character check has been a total
disaster. People shut it up by splitting long strings etc, and we've
had to change that 80-character test many times just to avoid the
crazy workarounds.
Don't warn about things that will just result in people working around
the warnings with worse code!
Linus
On Fri, 2012-02-03 at 17:27 -0800, Linus Torvalds wrote:
> On Fri, Feb 3, 2012 at 3:20 PM, Joe Perches <[email protected]> wrote:
> > Overly indented code should be refactored.
> > Suggest refactoring excessive indentation of
> > of if/else/for/do/while/switch statements.
> I hate this patch.
You liked the same premise, but a worse
implementation, a couple years ago.
https://lkml.org/lkml/2009/12/18/289
> Why? Because mindless checks like this would just lead to people
> making things worse and intermixing spaces there instead.
Can't happen.
They'll get a different warning about mixing tabs
and spaces or starting a line with spaces only.
> Don't warn about things that will just result in people working around
> the warnings with worse code!
It merely suggests refactoring. ie: better code.
On Fri, 3 Feb 2012 17:27:36 -0800 Linus Torvalds <[email protected]> wrote:
> On Fri, Feb 3, 2012 at 3:20 PM, Joe Perches <[email protected]> wrote:
> > Overly indented code should be refactored.
> >
> > Suggest refactoring excessive indentation of
> > of if/else/for/do/while/switch statements.
>
> I hate this patch.
>
> Why? Because mindless checks like this would just lead to people
> making things worse and intermixing spaces there instead.
>
> That's the same reason the 80-character check has been a total
> disaster. People shut it up by splitting long strings etc, and we've
> had to change that 80-character test many times just to avoid the
> crazy workarounds.
>
> Don't warn about things that will just result in people working around
> the warnings with worse code!
>
Sampling bias ;) You notice the 80-col fixups which resulted in
poor-looking code and not the fixups which resulted in decent-looking
code.
Joe Perches <[email protected]> writes:
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 2b52aeb..89d24b3 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1924,6 +1924,12 @@ sub process {
> my $pre_ctx = "$1$2";
>
> my ($level, @ctx) = ctx_statement_level($linenr, $realcnt, 0);
> +
> + if ($line =~ /^\+\t{6,}/) {
> + WARN("DEEP_INDENTATION",
> + "Too many leading tabs - consider code refactoring\n" . $herecurr);
> + }
By any chance have you run this patch against itself? I find it comical
that there is a line 104 characters long suggesting people use shorter
lines.
> my $ctx_cnt = $realcnt - $#ctx - 1;
> my $ctx = join("\n", @ctx);
>
Eric
On Fri, 2012-02-03 at 18:40 -0800, Eric W. Biederman wrote:
> Joe Perches <[email protected]> writes:
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> > @@ -1924,6 +1924,12 @@ sub process {
[]
> > +
> > + if ($line =~ /^\+\t{6,}/) {
> > + WARN("DEEP_INDENTATION",
> > + "Too many leading tabs - consider code refactoring\n" . $herecurr);
> > + }
> By any chance have you run this patch against itself?
Nope. perl isn't a c. Logic doesn't apply.
Some might claim perl logic an oxymoron.
checkpatch should be used for c sources only.
> I find it comical
> that there is a line 104 characters long suggesting people use shorter
> lines.
You're welcome to line wrap it if you like...
I don't actually use checkpatch much.
cheers, Joe
On Fri, Feb 3, 2012 at 5:33 PM, Joe Perches <[email protected]> wrote:
>
> You liked the same premise, but a worse
> implementation, a couple years ago.
> https://lkml.org/lkml/2009/12/18/289
I have grown to dislike debatable parts of checkpatch over the years,
which probably colors my reaction to things like this.
I'd prefer for checkpatch to check things that are fairly black-and-white.
>> Why? Because mindless checks like this would just lead to people
>> making things worse and intermixing spaces there instead.
>
> Can't happen.
> They'll get a different warning about mixing tabs
> and spaces or starting a line with spaces only.
Ok,. fair enough. That makes it more likely to work.
That said, I did a grep for six tabs, and we do seem to have quite a
bit of code like that. 67 _thousand_ lines just in C files, if I did
my grep right.
And many of them seem reasonable. Quite a lot of them seem to be to
just line things up with the previous line. Although some really are
due to excessively deep indentation. But it doesn't seem exactly
black-and-white to me.
Linus
On Fri, 2012-02-03 at 19:09 -0800, Linus Torvalds wrote:
> On Fri, Feb 3, 2012 at 5:33 PM, Joe Perches <[email protected]> wrote:
> > You liked the same premise, but a worse
> > implementation, a couple years ago.
> > https://lkml.org/lkml/2009/12/18/289
> I have grown to dislike debatable parts of checkpatch over the years,
> which probably colors my reaction to things like this.
> I'd prefer for checkpatch to check things that are fairly black-and-white.
> >> Why? Because mindless checks like this would just lead to people
> >> making things worse and intermixing spaces there instead.
> > Can't happen.
> > They'll get a different warning about mixing tabs
> > and spaces or starting a line with spaces only.
> Ok,. fair enough. That makes it more likely to work.
> That said, I did a grep for six tabs, and we do seem to have quite a
> bit of code like that. 67 _thousand_ lines just in C files, if I did
> my grep right
I think that's a bad test.
It finds a _lot_ of line continuations.
The right test is _only_ for 6 or more tabs
followed by (if|for|while|do|else|switch)
$ grep -rP --include=*.[ch] "^\t{6,}(if|for|while|do|else|switch)" * | \
wc -l
1509
On Fri, Feb 3, 2012 at 7:21 PM, Joe Perches <[email protected]> wrote:
>
> I think that's a bad test.
> It finds a _lot_ of line continuations.
>
> The right test is _only_ for 6 or more tabs
> followed by (if|for|while|do|else|switch)
Fair enough.
And I have to admit that doing your grep with an additional -4 to see
some context does make a fairly strong argument for your patch.
The code in drivers/isdn/hisax/hfc_usb.c that triggers is absolutely
disgusting, and does crazy things due to the long lines.
As is some other code that grep shows.
In fact, I think that grep convinced me that you are right about this
particular pattern. Brr. Now I need to go dig my eyes out with a
spoon.
Linus
>
> $ grep -rP --include=*.[ch] "^\t{6,}(if|for|while|do|else|switch)" * | \
> ?wc -l
> 1509
>
>
On Fri, 2012-02-03 at 19:35 -0800, Linus Torvalds wrote:
> I think that grep convinced me that you are right about this
> particular pattern.
Oh good.
> Brr. Now I need to go dig my eyes out with a
> spoon.
Maybe you can keep that thought for your next
Halloween display for the local urchins...
On Fri, Feb 3, 2012 at 3:20 PM, Joe Perches <[email protected]> wrote:
> Another might be to limit variable name length to something
> shorter than say 10 characters.
10? We have a few cases of variables over 100 (not sure how you are supposed
to use them with an 80 character max line length). Current longest is:
eOpenLogicalChannelAck_reverseLogicalChannelParameters_multiplexParameters_h2250LogicalChannelParameters
at 104 characters.
-Tony
On Fri, 2012-02-03 at 20:45 -0800, Tony Luck wrote:
> On Fri, Feb 3, 2012 at 3:20 PM, Joe Perches <[email protected]> wrote:
> > Another might be to limit variable name length to something
> > shorter than say 10 characters.
>
> 10? We have a few cases of variables over 100 (not sure how you are supposed
> to use them with an 80 character max line length). Current longest is:
>
> eOpenLogicalChannelAck_reverseLogicalChannelParameters_multiplexParameters_h2250LogicalChannelParameters
EUey. (err, well maybe ITUey)
Remind me never to look at that code again.
* Joe Perches <[email protected]> wrote:
> Overly indented code should be refactored.
_AND_ the 80 cols warning should be removed. The overwhelming
majority of developers either ignore the 80 cols warning or make
the code worse as a result of the warning.
So something like the patch below.
Thanks,
Ingo
--------------------->
Subject: checkpatch: Warn on code with 6+ tab indentation, remove 80col warning
It's better to warn about too deeply indented code than about
too long lines, as the too long line tends to cause people to
think about *that line*, instead of the surrounding code, fixing
it by breaking the line unnecessarily, etc.
If we warn about too deep indentation then the fix will be a
natural one: people will reduce code complexity, which is an
almost black and white good thing.
The few false positives can be ignored.
Signed-off-by: Ingo Molnar <[email protected]>
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e3bfcbe..5406011 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1726,17 +1726,6 @@ sub process {
# check we are in a valid source file if not then ignore this hunk
next if ($realfile !~ /\.(h|c|s|S|pl|sh)$/);
-#80 column limit
- if ($line =~ /^\+/ && $prevrawline !~ /\/\*\*/ &&
- $rawline !~ /^.\s*\*\s*\@$Ident\s/ &&
- !($line =~ /^\+\s*$logFunctions\s*\(\s*(?:(KERN_\S+\s*|[^"]*))?"[X\t]*"\s*(?:|,|\)\s*;)\s*$/ ||
- $line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) &&
- $length > 80)
- {
- WARN("LONG_LINE",
- "line over 80 characters\n" . $herecurr);
- }
-
# check for spaces before a quoted newline
if ($rawline =~ /^.*\".*\s\\n/) {
WARN("QUOTED_WHITESPACE_BEFORE_NEWLINE",
@@ -1924,6 +1913,12 @@ sub process {
my $pre_ctx = "$1$2";
my ($level, @ctx) = ctx_statement_level($linenr, $realcnt, 0);
+
+ if ($line =~ /^\+\t{6,}/) {
+ WARN("DEEP_INDENTATION",
+ "Too many leading tabs - code should probably be split up\n" . $herecurr);
+ }
+
my $ctx_cnt = $realcnt - $#ctx - 1;
my $ctx = join("\n", @ctx);
* Andrew Morton <[email protected]> wrote:
> On Fri, 3 Feb 2012 11:07:43 +0100
> Ingo Molnar <[email protected]> wrote:
>
> > [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums
> >
> > The overwhelming majority of kernel developers have stopped
> > using 80 col terminals years ago.
> >
> > As far as I'm aware I was the last regular kernel
> > contributor who still used a standard VGA text console, but
> > both text consoles and using them to read the kernel source
> > code has become increasingly gruesome years ago so I
> > switched to a wider terminal two years ago.
>
> I always use 80-cols, everywhere. Not because I particularly
> like it - I find it a bit too small. I use it because it is
> the standard, and using it helps me see where and how badly we
> violate the standard.
>
> We've actually done pretty well - linewrap in 80 cols rarely
> causes me problems. It's sufficiently rare that when it
> *does* happen, it really stands out.
Maybe it got better after the introduction of checkpatch - I
stopped using 80col terminals because the situation *was*
getting so bad and because i did not feel like fighting a
thousand other kernel developers who had different preferences
;-)
> IOW, the changelog is quite the exaggeration.
You are right about that.
> > So lets increase the limit to 100 cols
>
> I think that's going too far - 96 will be enough and it's a
> multiple of 8.
>
> The multiple-of-8 thing seems pleasing but probably doesn't
> matter much. It means that things like
>
>
> if (foo) {
> if (foo) {
> if (foo) {
> if (foo) {
> if (foo) {
> if (foo) {
> if (foo) {
> if (foo) {
> if (foo) {
> if (foo) {
> if (foo) {
> if (foo) {
> if (foo) {
> if (foo) {
> if (foo) {
>
>
> will line up properly.
>
> If we really want to improve the world we should jump into a
> time machine and set tabstops to 4. Sigh.
I think that would be a distinctly bad decision - people could
have crazy, 10 levels nesting in a function and be technically
'compliant'.
8 col tabs _forces clean code_ more often than not. I know about
very few functions in the kernel that legitimately need more
than 3 or 4 levels of nesting.
And that is why I agree with the 6-tab based warning approach -
then we can remove the 80col warning which is really making
things actively worse.
Thanks,
Ingo
On Sat, 2012-02-04 at 14:03 +0100, Ingo Molnar wrote:
> * Joe Perches <[email protected]> wrote:
> > Overly indented code should be refactored.
> _AND_ the 80 cols warning should be removed. The overwhelming
> majority of developers either ignore the 80 cols warning or make
> the code worse as a result of the warning.
Perhaps, but it should be a separate patch
and you'd still need to update CodingStyle.
* Joe Perches <[email protected]> wrote:
> On Sat, 2012-02-04 at 14:03 +0100, Ingo Molnar wrote:
> > * Joe Perches <[email protected]> wrote:
> > > Overly indented code should be refactored.
> > _AND_ the 80 cols warning should be removed. The overwhelming
> > majority of developers either ignore the 80 cols warning or make
> > the code worse as a result of the warning.
>
> Perhaps, but it should be a separate patch
> and you'd still need to update CodingStyle.
Why would I have to update CodingStyle? The 80col limit remains,
it's just removed from *checkpatch*.
Thanks,
Ingo
On Sat, 2012-02-04 at 19:02 +0100, Ingo Molnar wrote:
> * Joe Perches <[email protected]> wrote:
>
> > On Sat, 2012-02-04 at 14:03 +0100, Ingo Molnar wrote:
> > > * Joe Perches <[email protected]> wrote:
> > > > Overly indented code should be refactored.
> > > _AND_ the 80 cols warning should be removed. The overwhelming
> > > majority of developers either ignore the 80 cols warning or make
> > > the code worse as a result of the warning.
> >
> > Perhaps, but it should be a separate patch
> > and you'd still need to update CodingStyle.
>
> Why would I have to update CodingStyle? The 80col limit remains,
> it's just removed from *checkpatch*.
If that's your intent, I disagree with your patch.
There are some truly _ugly_ > 80 char lines that
people attempt where checkpatch should issue some
"don't do that" message.
On Sat, Feb 4, 2012 at 8:48 PM, Joe Perches <[email protected]> wrote:
> There are some truly _ugly_ > 80 char lines that
> people attempt where checkpatch should issue some
> "don't do that" message.
The practical downsides of the current warning are much more severe. I
personally don't use checkpatch much these days because of the silly
fixed limit.
On Sat, 2012-02-04 at 20:54 +0200, Pekka Enberg wrote:
> On Sat, Feb 4, 2012 at 8:48 PM, Joe Perches <[email protected]> wrote:
> > There are some truly _ugly_ > 80 char lines that
> > people attempt where checkpatch should issue some
> > "don't do that" message.
>
> The practical downsides of the current warning are much more severe. I
> personally don't use checkpatch much these days because of the silly
> fixed limit.
More likely you developed a keen sense of
kernel style appropriateness and don't need
any checkpatch nannying.
I don't disagree with something like setting
the normally allowed line length to 100 and
maybe keeping a --strict limit to 80.
Something like:
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2b52aeb..7602bce 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -34,6 +34,9 @@ my @ignore = ();
my $help = 0;
my $configuration_file = ".checkpatch.conf";
+my $max_line_length_warn = 100; # normal cases
+my $max_line_length_strict = 80; # when using --strict
+
sub help {
my ($exitcode) = @_;
@@ -1726,15 +1729,19 @@ sub process {
# check we are in a valid source file if not then ignore this hunk
next if ($realfile !~ /\.(h|c|s|S|pl|sh)$/);
-#80 column limit
+# check line length limit
if ($line =~ /^\+/ && $prevrawline !~ /\/\*\*/ &&
$rawline !~ /^.\s*\*\s*\@$Ident\s/ &&
!($line =~ /^\+\s*$logFunctions\s*\(\s*(?:(KERN_\S+\s*|[^"]*))?"[X\t]*"\s*(?:|,|\)\s*;)\s*$/ ||
- $line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) &&
- $length > 80)
- {
- WARN("LONG_LINE",
- "line over 80 characters\n" . $herecurr);
+ $line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) {
+ if ($length > $max_line_len_strict) {
+ CHK("LONG_LINE",
+ "line over $max_line_len_strict characters\n" . $herecurr);
+ }
+ if ($length > $max_line_len_warn) {
+ WARN("LONG_LINE",
+ "line over $max_line_len_warn characters\n" . $herecurr);
+ }
}
# check for spaces before a quoted newline
On Sat, Feb 4, 2012 at 9:27 PM, Joe Perches <[email protected]> wrote:
>> The practical downsides of the current warning are much more severe. I
>> personally don't use checkpatch much these days because of the silly
>> fixed limit.
>
> More likely you developed a keen sense of
> kernel style appropriateness and don't need
> any checkpatch nannying.
No, it just means more trivially fixable issues slip through the cracks.
On Sat, Feb 4, 2012 at 9:27 PM, Joe Perches <[email protected]> wrote:
> I don't disagree with something like setting
> the normally allowed line length to 100 and
> maybe keeping a --strict limit to 80.
>
> Something like:
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 2b52aeb..7602bce 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -34,6 +34,9 @@ my @ignore = ();
> ?my $help = 0;
> ?my $configuration_file = ".checkpatch.conf";
>
> +my $max_line_length_warn = 100; ? ? ? ? ? ? ? ?# normal cases
> +my $max_line_length_strict = 80; ? ? ? # when using --strict
> +
> ?sub help {
> ? ? ? ?my ($exitcode) = @_;
>
> @@ -1726,15 +1729,19 @@ sub process {
> ?# check we are in a valid source file if not then ignore this hunk
> ? ? ? ? ? ? ? ?next if ($realfile !~ /\.(h|c|s|S|pl|sh)$/);
>
> -#80 column limit
> +# check line length limit
> ? ? ? ? ? ? ? ?if ($line =~ /^\+/ && $prevrawline !~ /\/\*\*/ &&
> ? ? ? ? ? ? ? ? ? ?$rawline !~ /^.\s*\*\s*\@$Ident\s/ &&
> ? ? ? ? ? ? ? ? ? ?!($line =~ /^\+\s*$logFunctions\s*\(\s*(?:(KERN_\S+\s*|[^"]*))?"[X\t]*"\s*(?:|,|\)\s*;)\s*$/ ||
> - ? ? ? ? ? ? ? ? ? $line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) &&
> - ? ? ? ? ? ? ? ? ? $length > 80)
> - ? ? ? ? ? ? ? {
> - ? ? ? ? ? ? ? ? ? ? ? WARN("LONG_LINE",
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ?"line over 80 characters\n" . $herecurr);
> + ? ? ? ? ? ? ? ? ? $line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) {
> + ? ? ? ? ? ? ? ? ? ? ? if ($length > $max_line_len_strict) {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? CHK("LONG_LINE",
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?"line over $max_line_len_strict characters\n" . $herecurr);
> + ? ? ? ? ? ? ? ? ? ? ? }
> + ? ? ? ? ? ? ? ? ? ? ? if ($length > $max_line_len_warn) {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? WARN("LONG_LINE",
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?"line over $max_line_len_warn characters\n" . $herecurr);
> + ? ? ? ? ? ? ? ? ? ? ? }
> ? ? ? ? ? ? ? ?}
>
> ?# check for spaces before a quoted newline
I'm fine with something like this too.
* Joe Perches <[email protected]> wrote:
> + $line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) {
> + if ($length > $max_line_len_strict) {
> + CHK("LONG_LINE",
> + "line over $max_line_len_strict characters\n" . $herecurr);
> + }
> + if ($length > $max_line_len_warn) {
> + WARN("LONG_LINE",
> + "line over $max_line_len_warn characters\n" . $herecurr);
> + }
In practice patch submitters take warnings just as seriously.
If it is emitted by the default checkpatch run, it's acted
upon. That is the human behavior that is a given.
If warnings are often crap and should not be acted upon then
frankly, don't emit them by default.
I don't care *how* that warning is removed - whether it's
removed from checkpatch or just hidden from the default view -
but it needs to be removed or people like Pekka (or me) stop
using it.
Thanks,
Ingo
On Sun, 2012-02-05 at 12:38 +0100, Ingo Molnar wrote:
> In practice patch submitters take warnings just as seriously.
In practice, that's not necessarily bad.
I do think there should be some line length limit.
80 might be a bit short.
> If it is emitted by the default checkpatch run, it's acted
> upon. That is the human behavior that is a given.
Look at some of the lines in drivers/staging
that have _really_ long lines that would not
get any warning if this were removed.
$ git ls-files "drivers/staging/*.[ch]" | xargs cat | \
awk '{print length($0),$0}' | sort -rn | head -50
gotta love the first few...
A few lines in arch/x86 might benefit from
some wrapping too.
* Joe Perches <[email protected]> wrote:
> On Sun, 2012-02-05 at 12:38 +0100, Ingo Molnar wrote:
> >
> > In practice patch submitters take warnings just as
> > seriously.
>
> In practice, that's not necessarily bad.
In practice it *is* bad, and I say that as a maintainer who is
receiving many checkpatch 'fixes' on a daily basis. Many
checkpatch warnings are legitimate - but the col80 one is bogus
in many cases.
Bogus warnings pollute the output of the tool, reducing the
utility of the *other* warnings.
( GCC frequently made this mistake in the past, emitting dubious
warnings by default - it's been getting somewhat better
lately. )
So if your patch emits no warning for the col80 thing then
that's a step forward in the right direction in my opinion.
Thanks,
Ingo
80 column line lengths can be a bit constraining.
Make the default 100 and still emit a warning
when that length is exceeded.
When option --strict is set, emit a check when
the length is > 80 too.
Add a command line option --line-length to set
the standard line length allowed.
Using command line option --ignore=LINE_LENGTH
will not emit any line-length messages.
Adding a .checkpatch.conf file with appropriate
command line options would also avoid line-length
messages.
Signed-off-by: Joe Perches <[email protected]>
---
scripts/checkpatch.pl | 21 +++++++++++++++------
1 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2b52aeb..6923270 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -34,6 +34,9 @@ my @ignore = ();
my $help = 0;
my $configuration_file = ".checkpatch.conf";
+my $max_line_length_warn = 100; # normal cases
+my $max_line_length_strict = 80; # when using --strict
+
sub help {
my ($exitcode) = @_;
@@ -50,6 +53,7 @@ Options:
--terse one line per report
-f, --file treat FILE as regular source file
--subjective, --strict enable more subjective tests
+ --line-length=val set line length to emit a warning when exceeded
--ignore TYPE(,TYPE2...) ignore various comma separated message types
--show-types show the message "types" in the output
--root=PATH PATH to the kernel tree root
@@ -105,6 +109,7 @@ GetOptions(
'f|file!' => \$file,
'subjective!' => \$check,
'strict!' => \$check,
+ 'line-length=i' => \$max_line_length_warn,
'ignore=s' => \@ignore,
'show-types!' => \$show_types,
'root=s' => \$root,
@@ -1726,15 +1731,19 @@ sub process {
# check we are in a valid source file if not then ignore this hunk
next if ($realfile !~ /\.(h|c|s|S|pl|sh)$/);
-#80 column limit
+# check line length limits
if ($line =~ /^\+/ && $prevrawline !~ /\/\*\*/ &&
$rawline !~ /^.\s*\*\s*\@$Ident\s/ &&
!($line =~ /^\+\s*$logFunctions\s*\(\s*(?:(KERN_\S+\s*|[^"]*))?"[X\t]*"\s*(?:|,|\)\s*;)\s*$/ ||
- $line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) &&
- $length > 80)
- {
- WARN("LONG_LINE",
- "line over 80 characters\n" . $herecurr);
+ $line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/)) {
+ if ($length > $max_line_length_strict) {
+ CHK("LONG_LINE",
+ "line longer than $max_line_length_strict characters\n" . $herecurr);
+ }
+ if ($length > $max_line_length_warn) {
+ WARN("LONG_LINE",
+ "line longer than $max_line_length_warn characters\n" . $herecurr);
+ }
}
# check for spaces before a quoted newline
It doesn't make sense for everyone to pick a different standard.
I do hate the check, but if it were disabled by default when people
used the -f option, that would make me happy. The -f option is
mostly used by newbies trying to write a My First Kernel Patch.
regards,
dan carpenter
On Friday 2012-02-03 11:07, Ingo Molnar wrote:
>[PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums
>
>As far as I'm aware I was the last regular kernel contributor who
>still used a standard VGA text console, but both text (100 cols is
>also arguably closer to various brain limits such as vision of field
>and resolution restrictions
Please, no.
(Corollary 1) The older you get, the larger you like your glyphs to
be (...and the FOV is likely to lower as well).
14pt and up usually. And there is not a whole lot that fit on
10.1-inch netbook screens that way.
(Corollary 2) Travelers desire minimizing size and weight.
-----
As for checkpatch, I am throwing in the suggestion to augment checkpatch
such that it does not look at whether single lines are over $limit, but
whether a certain percentage of lines of a file is over $limit. That,
together with a badness value that is e.g. following some power law to
the amount of chars too much, but not when the line cannot be broken
in the first place. Maybe along the lines of
#perlish#
foreach (<>) {
/^\s+\S+/;
if (length($_) > length($&)) {
push @candidate, $_;
$badness += (length($_) - length($&)) ** 1.5;
}
}
if ($badness > $threshold) {
warn about @candidate_lines;
}
On Thu, 2012-02-09 at 22:55 +0100, Jan Engelhardt wrote:
> I am throwing in the suggestion to augment checkpatch
> such that it does not look at whether single lines are over $limit, but
> whether a certain percentage of lines of a file is over $limit. That,
> together with a badness value that is e.g. following some power law to
> the amount of chars too much, but not when the line cannot be broken
> in the first place. Maybe along the lines of
>
> #perlish#
> foreach (<>) {
> /^\s+\S+/;
> if (length($_) > length($&)) {
> push @candidate, $_;
> $badness += (length($_) - length($&)) ** 1.5;
> }
> }
> if ($badness > $threshold) {
> warn about @candidate_lines;
> }
I'd be OK with something like this while making the
current line length check a --strict only option.
On Thu, Feb 09, 2012 at 10:55:45PM +0100, Jan Engelhardt wrote:
> On Friday 2012-02-03 11:07, Ingo Molnar wrote:
> >[PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums
> >As far as I'm aware I was the last regular kernel contributor who
> >still used a standard VGA text console, but both text (100 cols is
> >also arguably closer to various brain limits such as vision of field
> >and resolution restrictions
> Please, no.
> (Corollary 1) The older you get, the larger you like your glyphs to
> be (...and the FOV is likely to lower as well).
> 14pt and up usually. And there is not a whole lot that fit on
> 10.1-inch netbook screens that way.
> (Corollary 2) Travelers desire minimizing size and weight.
Or your vision is a bit better but you like multiple things over the
screen width...