[patch cr 0/4] [patch cr 0/@total@]
Subsystems (6)
cover [patch cr 0/4] [patch cr 0/@total@]
Hi,
this is a series where I tried to resolve all concerns and nits I've got from previous attempt:
- fs, proc: Introduce /proc/<pid>/task/<tid>/children entry v9 Now everything is under CONFIG_CHECKPOINT_RESTORE
- syscalls, x86: Add __NR_kcmp syscall v7
* EBADF returned if there is no file
* EOPNOTSUP returned if there is no sysv support compiled in
* test uses syscall() from libc instead of own syscall implementation
* man page for this syscall is under progress still
- c/r: procfs: add arg_start/end, env_start/end and exit_code members to /proc/$pid/stat
task->exit_code is shown IIF a caller has enough permissions
- c/r: prctl: Extend PR_SET_MM to set up more mm_struct entries
when restoring mm->saved_auxv the task_lock is used to serialize access
Please review, if there is something I'm still missing, ping me back.
Thanks,
Cyrill
PATCH 1/4 fs, proc: Introduce /proc/<pid>/task/<tid>/children entry v9
When we do checkpoint of a task we need to know the list of children the task, has but there is no easy and fast way to generate reverse parent->children chain from arbitrary <pid> (while a parent pid is provided in "PPid" field of /proc/<pid>/status).
So instead of walking over all pids in the system (creating one big process tree in memory, just to figure out which children a task has) -- we add explicit /proc/<pid>/task/<tid>/children entry, because the kernel already has this kind of information but it is not yet exported.
This is a first level children, not the whole process tree.
--- Documentation/filesystems/proc.txt | 18 +++++ fs/proc/array.c | 123 +++++++++++++++++++++++++++++++++++++ fs/proc/base.c | 3 fs/proc/internal.h | 1 4 files changed, 145 insertions(+)
Documentation/filesystems/proc.txt +18 -0
@@ -40,6 +40,7 @@ Table of Contents 3.4 /proc/<pid>/coredump_filter - Core dump filtering settings 3.5 /proc/<pid>/mountinfo - Information about mounts 3.6 /proc/<pid>/comm & /proc/<pid>/task/<tid>/comm + 3.7 /proc/<pid>/task/<tid>/children - Information about task children 4 Configuring procfs 4.1 Mount options
@@ -1549,6 +1550,23 @@ then the kernel's TASK_COMM_LEN (current comm value. +3.7 /proc/<pid>/task/<tid>/children - Information about task children +------------------------------------------------------------------------- +This file provides a fast way to retrieve first level children pids +of a task pointed by <pid>/<tid> pair. The format is a space separated +stream of pids. + +Note the "first level" here -- if a child has own children they will +not be listed here, one needs to read /proc/<children-pid>/task/<tid>/children +to obtain the descendants. + +Since this interface is intended to be fast and cheap it doesn't +guarantee to provide precise results and some children might be +skipped, especially if they've exited right after we printed their +pids, so one need to either stop or freeze processes being inspected +if precise results are needed. + + ------------------------------------------------------------------------------ Configuring procfs ------------------------------------------------------------------------------
fs/proc/array.c +123 -0
126 lines not on this page · raw message
fs/proc/base.c +3 -0
@@ -3384,6 +3384,9 @@ static const struct pid_entry tid_base_sONE ("stat" ,S_IRUGO , proc_tid_stat),ONE ("statm" ,S_IRUGO , proc_pid_statm),REG ("maps" ,S_IRUGO , proc_maps_operations), +#ifdef CONFIG_CHECKPOINT_RESTORE +REG ("children" ,S_IRUGO , proc_tid_children_operations), +#endif #ifdef CONFIG_NUMA REG("numa_maps", S_IRUGO, proc_numa_maps_operations), #endif
fs/proc/internal.h +1 -0
@@ -53,6 +53,7 @@ extern int proc_pid_statm(struct seq_fil struct pid *pid, struct task_struct *task);extern loff_t mem_lseek (struct file *file,loff_t offset,int orig); +extern const struct file_operations proc_tid_children_operations;extern const struct file_operations proc_maps_operations;extern const struct file_operations proc_numa_maps_operations;extern const struct file_operations proc_smaps_operations;
RFC PATCH 2/4 syscalls, x86: Add __NR_kcmp syscall v7
In reply to Cyrill GorcunovWhile doing the checkpoint-restore in the userspace 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 'comparision' syscall might be the best candidate. So here is it -- __NR_kcmp.
It takes up to 5 agruments - the pids of the two tasks (which characteristics should be compared), the comparision type and (in case of comparision 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.
--- 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 | 1 kernel/kcmp.c | 170 +++++++++++++++++++++++++++++++ tools/testing/selftests/kcmp/Makefile | 35 ++++++ tools/testing/selftests/kcmp/kcmp_test.c | 89 ++++++++++++++++ tools/testing/selftests/run_tests | 2 9 files changed, 317 insertions(+), 1 deletion(-)
arch/x86/syscalls/syscall_32.tbl +1 -0
@@ -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
arch/x86/syscalls/syscall_64.tbl +1 -0
@@ -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
include/linux/kcmp.h new file +17 -0
@@ -0,0 +1,17 @@ +#ifndef _LINUX_KCMP_H +#define _LINUX_KCMP_H + +/* Comparision type */ +enum kcmp_type { +KCMP_FILE , +KCMP_VM , +KCMP_FILES , +KCMP_FS , +KCMP_SIGHAND , +KCMP_IO , +KCMP_SYSVSEM , + +KCMP_TYPES , +}; + +#endif /* _LINUX_KCMP_H */
include/linux/syscalls.h +2 -0
@@ -857,4 +857,6 @@ asmlinkage long sys_process_vm_writev(piunsigned long riovcnt, unsigned long flags); +asmlinkage long sys_kcmp (pid_t pid1,pid_t pid2,int type, +unsigned long idx1,unsigned long idx2);#endif
kernel/Makefile +1 -0
@@ -25,6 +25,7 @@ endif obj-y += sched/ obj-y += power/ +obj-$(CONFIG_X86) += kcmp.o obj-$(CONFIG_FREEZER) += freezer.o obj-$(CONFIG_PROFILING) += profile.o obj-$(CONFIG_SYSCTL_SYSCALL_CHECK) += sysctl_check.o
kernel/kcmp.c new file +170 -0
170 lines not on this page · raw message
tools/testing/selftests/kcmp/Makefile new file +35 -0
@@ -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
tools/testing/selftests/kcmp/kcmp_test.c new file +89 -0
89 lines not on this page · raw message
tools/testing/selftests/run_tests +1 -1
@@ -1,6 +1,6 @@ #!/bin/bash -TARGETS=breakpoints +TARGETS="breakpoints kcmp" for TARGET in $TARGETS do
Jonathan Corbet
maintainerRe: [patch cr 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v7
Just a couple of silly little things that came to mind while I was looking at the code...
kernel/kcmp.c line 27
Quoted 13 lines
kernel/kcmp.c new file +13
-0+/* +* 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. +*/ +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 ]; +}
I don't understand the purpose of this at all. Obfuscation will cause a random shuffling in the ordering of the pointers - it's intended to - so how is the result "suitable for sorting"? More to the point, is there ever a time when a user of this will care about some contrived ordering value? It seems like equality is all that really matters.
Quoted 5 lines
kernel/kcmp.c new file +5
-0+ +/* +* 0 - equal +* 1 - less than +* 2 - greater than
kernel/kcmp.c line 42
Quoted 10 lines
kernel/kcmp.c new file +10
-0+* 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 ); +}
That's a cute trick, but do we know that every compiler that will ever see this code will use 1 for a true integer comparison? Simply spelling it out with an if statement might be more robust, just as efficient, and, at the same time, easier for others to understand.
Thanks,
jon
Re: [patch cr 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v7
Quoted 14 lines · Jonathan Corbet
Just a couple of silly little things that came to mind while I was looking at the code...
Quoted 11 lines
kernel/kcmp.c new file +11
-0+/* +* 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. +*/ +static unsigned long cookies[KCMP_TYPES ][2 ]__read_mostly ; + +static long kptr_obfuscate (long v,int type) +{
Quoted 9 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ return (v ^ cookies[type][0]) * cookies[type][1]; +}I don't understand the purpose of this at all. Obfuscation will cause a random shuffling in the ordering of the pointers - it's intended to - so how is the result "suitable for sorting"? More to the point, is there ever a time when a user of this will care about some contrived ordering value? It seems like equality is all that really matters.
It won't be completely random shuffling but rather re-ordering in some new order, which means the results might be passed to qsort or anything. And yes, in c/r we need at least this "re-ordered" order which will help to figure out shared file descriptors in case of huge number of files opened.
Quoted 12 lines
Quoted 12 lines
kernel/kcmp.c new file +12
-0+ +/* +* 0 - equal +* 1 - less than +* 2 - greater than +* 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);
Quoted 8 lines
kernel/kcmp.c line 42
Quoted 3 lines
kernel/kcmp.c new file +3
-0+ +return (ret <0 ) | ((ret >0 ) <<1 ); +}That's a cute trick, but do we know that every compiler that will ever see this code will use 1 for a true integer comparison? Simply spelling it out with an if statement might be more robust, just as efficient, and, at the same time, easier for others to understand.
Well, I believe if this become true, and (ret < 0) wont emit 1 -- the number of places in kernel will be broken as well (for example see math_div() function). But of course I don't insist and can rewrite this code in straight fashion if needed.
Cyrill
H. Peter Anvin
reviewerRe: [patch cr 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v7
In reply to Jonathan CorbetQuoted 6 lines · Jonathan Corbet
That's a cute trick, but do we know that every compiler that will ever see this code will use 1 for a true integer comparison? Simply spelling it out with an if statement might be more robust, just as efficient, and, at the same time, easier for others to understand.
This has been the case in C from the very beginning and is guaranteed by the C standard. The number of places which will break if this isn't true is enormous.
-hpa
Re: [patch cr 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v7
In reply to Cyrill GorcunovQuoted 28 lines · On Mon, 30 Jan 2012 18:09:07 +0400 Cyrill Gorcunov <[email protected]> wrote:
While doing the checkpoint-restore in the userspace 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_structis to provide somemm_structID 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 'comparision' syscall might be the best candidate. So here is it --
__NR_kcmp.It takes up to 5 agruments - the pids of the two tasks (which characteristics should be compared), the comparision type and (in case of comparision 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.
...
kernel/Makefile endif · line 28
Quoted 4 lines
kernel/Makefile +1
-0@@ -25,6 +25,7 @@ endif obj-y += sched/ obj-y += power/ +obj-$(CONFIG_X86) += kcmp.o
Can we turn this into obj-$(CONFIG_CHECKPOINT_RESTART) and add the cond_syscall to sys_ni.c?
I keep saying this, because "hey, we can delete it all again" figures largely in my rationale for merging your code ;)
Quoted 3 lines
...
H. Peter Anvin
reviewerRe: [patch cr 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v7
Quoted 7 lines · Andrew Morton
Quoted 7 lines
kernel/Makefile +0
-0@@ -25,6 +25,7 @@ endif obj-y += sched/ obj-y += power/
Quoted 8 lines
kernel/Makefile endif · line 28
Quoted 1 line
kernel/Makefile +1
-0+obj-$(CONFIG_X86) += kcmp.oCan we turn this into obj-$(CONFIG_CHECKPOINT_RESTART) and add the
cond_syscalltosys_ni.c?I keep saying this, because "hey, we can delete it all again" figures largely in my rationale for merging your code ;)
Ah, yes please.
-hpa
Re: [patch cr 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v7
Quoted 13 lines · H. Peter Anvin
Quoted 12 lines · Andrew Morton
kernel/Makefile endif · line 28
Quoted 8 lines
kernel/Makefile +1
-0@@ -25,6 +25,7 @@ endif obj-y += sched/ obj-y += power/ +obj-$(CONFIG_X86) += kcmp.oCan we turn this into obj-$(CONFIG_CHECKPOINT_RESTART) and add the
cond_syscalltosys_ni.c?
Quoted 6 lines
Quoted 3 lines
I keep saying this, because "hey, we can delete it all again" figures largely in my rationale for merging your code ;)
Ah, yes please.
Sure, will update, thanks!
Cyrill
Re: [patch cr 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v7
In reply to Cyrill GorcunovQuoted 8 lines · Cyrill Gorcunov
+/* 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.
Quoted 4 lines
+ * 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?
kernel/kcmp.c line 73
Quoted 4 lines
kernel/kcmp.c new file +4
-0+/* +* Tasks are looked up in caller's +* PID namespace only. +*/
Could be a single line.
kernel/kcmp.c line 86
Quoted 13 lines
kernel/kcmp.c new file +13
-0+ + 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 ...
kernel/kcmp.c line 105
Quoted 4 lines
kernel/kcmp.c new file +4
-0+/* +* Note for all cases but the KCMP_FILE we +* don't take any locks in a sake of speed. +*/
Spelling.
kernel/kcmp.c line 163
Quoted 2 lines
kernel/kcmp.c new file +2
-0+get_random_bytes (&cookies[i][j], +sizeof (cookies[i][j]));
ugly line break.
Quoted 1 line
+late_initcall(kcmp_cookie_init);
any particular reason why this needs to be a late initcall?
Quoted 28 lines
tools/testing/selftests/kcmp/Makefile new file +25
-0@@ -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)
tools/testing/selftests/kcmp/Makefile line 35
Quoted 10 lines
tools/testing/selftests/kcmp/Makefile new file +10
-0+ $(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.
tools/testing/selftests/kcmp/Makefile
Quoted 5 lines
tools/testing/selftests/kcmp/Makefile new file +5
-0+#ifdef CONFIG_X86_64 +#include <asm/unistd_64.h> +#else +#include <asm/unistd_32.h> +#endif
Why is asm/unistd.h not good?
tools/testing/selftests/kcmp/Makefile
Quoted 5 lines
tools/testing/selftests/kcmp/Makefile new file +5
-0+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?
tools/testing/selftests/kcmp/Makefile
Quoted 12 lines
tools/testing/selftests/kcmp/Makefile new file +12
-0+ 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.
tools/testing/selftests/kcmp/Makefile
Quoted 8 lines
tools/testing/selftests/kcmp/Makefile new file +8
-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);
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
Re: [patch cr 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v7
Quoted 11 lines · Ingo Molnar
Quoted 7 lines · Cyrill Gorcunov
+/* 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.
Quoted 7 lines
Quoted 4 lines
+ * 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.
Quoted 8 lines
kernel/kcmp.c line 73
Quoted 5 lines
kernel/kcmp.c new file +4
-0+/* +* Tasks are looked up in caller's +* PID namespace only. +*/ Could be a single line.
Ok, will do so.
Quoted 7 lines
Quoted 7 lines
kernel/kcmp.c new file +7
-0+ + task1 =find_task_by_vpid (pid1); +if (!task1) { +rcu_read_unlock (); +return -ESRCH ; + } +
Quoted 8 lines
kernel/kcmp.c line 86
Quoted 6 lines
kernel/kcmp.c new file +6
-0+ 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.
Quoted 7 lines
kernel/kcmp.c line 105
Quoted 5 lines
kernel/kcmp.c new file +4
-0+/* +* 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.
Quoted 6 lines
kernel/kcmp.c line 163
Quoted 3 lines
kernel/kcmp.c new file +2
-0+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.
Quoted 4 lines
Quoted 1 line
+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!
Quoted 7 lines
tools/testing/selftests/kcmp/Makefile line 35
Quoted 5 lines
tools/testing/selftests/kcmp/Makefile new file +5
-0+ +clean: + $(E) " CLEAN" + $(Q) rm -fr ./run_test + $(Q) rm -fr ./test-fileNeeds buy-in from the kbuild guys.
I took breakpoint test as example. Maybe I should
send this test case code as a separate patch?
Quoted 9 lines
tools/testing/selftests/kcmp/kcmp_test.c line 16
Quoted 6 lines
tools/testing/selftests/kcmp/kcmp_test.c new file +5
-0+#ifdef CONFIG_X86_64 +#include <asm/unistd_64.h> +#else +#include <asm/unistd_32.h> +#endif Why is
asm/unistd.hnot 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?
Quoted 9 lines
tools/testing/selftests/kcmp/kcmp_test.c line 29
Quoted 5 lines
tools/testing/selftests/kcmp/kcmp_test.c new file +5
-0+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.
Quoted 15 lines
tools/testing/selftests/kcmp/kcmp_test.c line 63
Quoted 12 lines
tools/testing/selftests/kcmp/kcmp_test.c new file +12
-0+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.
Quoted 12 lines
tools/testing/selftests/kcmp/kcmp_test.c line 83
Quoted 8 lines
tools/testing/selftests/kcmp/kcmp_test.c new file +8
-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);this is
main(), what's wrong with the standard pattern of return ret?
It's fork'ed children.
Quoted 3 lines
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
Re: [patch cr 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v7
Quoted 7 lines · Cyrill Gorcunov
Quoted 5 lines
kernel/kcmp.c line 163
Quoted 2 lines
kernel/kcmp.c new file +2
-0+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
Re: [patch cr 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v7
Quoted 9 lines · On Fri, 3 Feb 2012 10:09:29 +0100 Ingo Molnar <[email protected]> wrote:
Quoted 6 lines · Cyrill Gorcunov
Quoted 5 lines
kernel/kcmp.c line 163
Quoted 2 lines
kernel/kcmp.c new file +2
-0+get_random_bytes (&cookies[i][j], +sizeof (cookies[i][j]));ugly line break.
Quoted 4 lines
Quoted 1 line
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.
Quoted 2 lines
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.
Re: [patch cr 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v7
Quoted 19 lines · Andrew Morton
Quoted 9 lines · On Fri, 3 Feb 2012 10:09:29 +0100 Ingo Molnar <[email protected]> wrote:
Quoted 6 lines · Cyrill Gorcunov
Quoted 5 lines
kernel/kcmp.c line 163
Quoted 2 lines
kernel/kcmp.c new file +2
-0+get_random_bytes (&cookies[i][j], +sizeof (cookies[i][j]));ugly line break.
Quoted 4 lines
Quoted 1 line
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.
Quoted 9 lines
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.
Quoted 2 lines
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
H. Peter Anvin
reviewerRe: [patch cr 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v7
Quoted 21 lines · Cyrill Gorcunov
static
__initintkcmp_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
__initintkcmp_cookie_init(void)
Quoted 10 lines
{
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
H. Peter Anvin
reviewerRe: [patch cr 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v7
Quoted 33 lines · H. Peter Anvin
Quoted 25 lines · Cyrill Gorcunov
static
__initintkcmp_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
__initintkcmp_cookie_init(void)
{
const int size =sizeof(cookies[0][0]);
int i, j;Quoted 6 lines
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);
}
Quoted 8 lines
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;
}Re: [patch cr 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v7
On Fri, Feb 03, 2012 at 09:35:05AM -0800, H. Peter Anvin wrote:
...
Quoted 12 lines
Quoted 8 lines
How about:
const int size =
sizeof(cookies[0]);
get_random_bytes(&cookies[i], size);... and skip the completely unnecessary for loop?
Even better:
static
__initintkcmp_cookie_init(void)
Quoted 9 lines
{
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.
--- 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(-)
arch/x86/syscalls/syscall_32.tbl +1 -0
@@ -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
arch/x86/syscalls/syscall_64.tbl +1 -0
@@ -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
include/linux/kcmp.h new file +17 -0
@@ -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 */
include/linux/syscalls.h +2 -0
@@ -857,4 +857,6 @@ asmlinkage long sys_process_vm_writev(piunsigned long riovcnt, unsigned long flags); +asmlinkage long sys_kcmp (pid_t pid1,pid_t pid2,int type, +unsigned long idx1,unsigned long idx2);#endif
kernel/Makefile +3 -0
@@ -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
kernel/kcmp.c new file +155 -0
155 lines not on this page · raw message
kernel/sys_ni.c +3 -0
@@ -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);
tools/testing/selftests/kcmp/Makefile new file +36 -0
@@ -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
tools/testing/selftests/kcmp/kcmp_test.c new file +84 -0
84 lines not on this page · raw message
tools/testing/selftests/run_tests +1 -1
@@ -1,6 +1,6 @@ #!/bin/bash -TARGETS=breakpoints +TARGETS="breakpoints kcmp" for TARGET in $TARGETS do
Re: [patch cr 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v7
In reply to Andrew MortonQuoted 11 lines · Andrew Morton
Quoted 9 lines · On Fri, 3 Feb 2012 10:09:29 +0100 Ingo Molnar <[email protected]> wrote:
Quoted 6 lines · Cyrill Gorcunov
Quoted 5 lines
kernel/kcmp.c line 163
Quoted 2 lines
kernel/kcmp.c new file +2
-0+get_random_bytes (&cookies[i][j], +sizeof (cookies[i][j]));ugly line break.
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
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.
Quoted 3 lines
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.
Quoted 4 lines
Quoted 2 lines
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
PATCH 1/1 SubmittingPatches: Increase the line length limit from 80 to 100 colums
Quoted 10 lines · Ingo Molnar
Quoted 4 lines
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.)
---
Documentation/CodingStyle +5 -6
@@ -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
scripts/checkpatch.pl +3 -4
@@ -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
Re: [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums
Quoted 35 lines · Ingo Molnar
Quoted 5 lines
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 columsThe 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.)
Quoted 1 line
Signed-off-by Ingo Molnar <[email protected]>
The rationale in the changelog is somewhat over the top but I agree with the change :-)
Cyrill Gorcunov
Acked-byRe: [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums
Quoted 8 lines · Pekka Enberg
Quoted 2 lines
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!
Cyrill
Re: [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums
In reply to Ingo MolnarQuoted 1 line · Ingo Molnar
[PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums
Better don't mention line length at all.
Re: [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums
In reply to Ingo MolnarHello,
Quoted 8 lines · Ingo Molnar
Quoted 5 lines · Ingo Molnar
Quoted 4 lines
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.
Quoted 5 lines
Quoted 5 lines
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.
Quoted 10 lines
[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.
Quoted 6 lines
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.
Quoted 8 lines
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.
Thanks.
Signature · 1 line
tejun
[email protected]
reviewerRe: [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums
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.
Quoted 69 lines · Tejun Heo
Hello,
Quoted 8 lines · Ingo Molnar
Quoted 5 lines · Ingo Molnar
Quoted 4 lines
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.
Quoted 5 lines
Quoted 5 lines
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.
Quoted 1 line
[PATCH] SubmittingPatches: Increase the line length limit from 80 to
100 colums
Quoted 9 lines
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.
Quoted 6 lines
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.
Quoted 8 lines
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.
Signature · 1 line
tejun
Signature · 1 line
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
Re: [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums
In reply to Ingo MolnarQuoted 8 lines
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 @)
Quoted 2 lines
Signed-off-by Ingo Molnar <[email protected]>
-Andi
Re: [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums
In reply to Ingo MolnarQuoted 10 lines · 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.
Quoted 1 line
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.
H. Peter Anvin
reviewerRe: [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums
Quoted 4 lines · Andrew Morton
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
Signature · 2 lines
H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf.
H. Peter Anvin
reviewerRe: [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums
In reply to Andrew MortonQuoted 5 lines · Andrew Morton
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
Signature · 2 lines
H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf.
Re: [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums
In reply to Andrew MortonQuoted 14 lines · Andrew Morton
Quoted 10 lines · 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.
Quoted 8 lines
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 ;-)
Quoted 1 line
IOW, the changelog is quite the exaggeration.
You are right about that.
Quoted 26 lines
Quoted 1 line
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) {
Quoted 4 lines
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
Re: [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums
In reply to Ingo MolnarQuoted 3 lines · Ingo Molnar
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
Joe Perches
maintainerPATCH 1/1 checkpatch: Warn on code with 6+ tab indentation
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.
---
Quoted 1 line · Linus Torvalds
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(-)
scripts/checkpatch.pl +6 -0
@@ -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);
Re: [PATCH] checkpatch: Warn on code with 6+ tab indentation
Quoted 4 lines · Joe Perches
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
Joe Perches
maintainerRe: [PATCH] checkpatch: Warn on code with 6+ tab indentation
Quoted 5 lines · Linus Torvalds
Quoted 3 lines · Joe Perches
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
Quoted 2 lines
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.
Quoted 2 lines
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.
Re: [PATCH] checkpatch: Warn on code with 6+ tab indentation
Quoted 4 lines · Joe Perches
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.
Quoted 6 lines
Quoted 2 lines
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
Joe Perches
maintainerRe: [PATCH] checkpatch: Warn on code with 6+ tab indentation
Quoted 9 lines · Linus Torvalds
Quoted 3 lines · Joe Perches
You liked the same premise, but a worse
implementation, a couple years ago.
https://lkml.org/lkml/2009/12/18/289I 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.Quoted 2 lines
Quoted 2 lines
Why? Because mindless checks like this would just lead to people making things worse and intermixing spaces there instead.
Quoted 7 lines
Quoted 3 lines
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
1509Re: [PATCH] checkpatch: Warn on code with 6+ tab indentation
Quoted 6 lines · Joe Perches
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
Quoted 6 lines
$ grep -rP --include=*.[ch] "^\t{6,}(if|for|while|do|else|switch)" * | \
wc -l
1509Joe Perches
maintainerRe: [PATCH] checkpatch: Warn on code with 6+ tab indentation
Quoted 2 lines · Linus Torvalds
I think that grep convinced me that you are right about this particular pattern.
Oh good.
Quoted 2 lines
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...
Re: [PATCH] checkpatch: Warn on code with 6+ tab indentation
In reply to Linus TorvaldsQuoted 11 lines · On Fri, 3 Feb 2012 17:27:36 -0800 Linus Torvalds…
Quoted 4 lines · Joe Perches
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.
Quoted 8 lines
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.
Re: [PATCH] checkpatch: Warn on code with 6+ tab indentation
In reply to Joe Perchesscripts/checkpatch.pl sub process { · line 1931
Quoted 13 lines · Joe Perches
scripts/checkpatch.pl +5
-0@@ -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.
scripts/checkpatch.pl sub process { · line 1935
Quoted 3 lines
scripts/checkpatch.pl +0
-0my $ctx_cnt = $realcnt - $#ctx - 1; my $ctx = join("\n", @ctx);
Eric
Joe Perches
maintainerRe: [PATCH] checkpatch: Warn on code with 6+ tab indentation
Quoted 2 lines · Eric W. Biederman
scripts/checkpatch.pl
Quoted 1 line · Joe Perches
scripts/checkpatch.pl +0
-0
[]
Quoted 1 line
scripts/checkpatch.pl sub process {
Quoted 1 line
scripts/checkpatch.pl +0
-0@@ -1924,6 +1924,12 @@ sub process {
[]
Quoted 6 lines
scripts/checkpatch.pl sub process { · line 1931
Quoted 5 lines
scripts/checkpatch.pl +5
-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?
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.
Quoted 3 lines
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
Re: [PATCH] checkpatch: Warn on code with 6+ tab indentation
In reply to Joe PerchesQuoted 2 lines · Joe Perches
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
Joe Perches
maintainerRe: [PATCH] checkpatch: Warn on code with 6+ tab indentation
Quoted 8 lines · Tony Luck
Quoted 2 lines · Joe Perches
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.
PATCH v2 1/1 checkpatch: Warn on code with 6+ tab indentation, remove 80col warning
In reply to Joe PerchesQuoted 1 line · Joe Perches
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.
scripts/checkpatch.pl +6 -11
@@ -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);
Joe Perches
maintainerRe: [PATCH, v2] checkpatch: Warn on code with 6+ tab indentation, remove 80col warning
Quoted 5 lines · Ingo Molnar
Quoted 1 line · Joe Perches
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.
Re: [PATCH, v2] checkpatch: Warn on code with 6+ tab indentation, remove 80col warning
Quoted 9 lines · Joe Perches
Quoted 5 lines · Ingo Molnar
Quoted 1 line · Joe Perches
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
Joe Perches
maintainerRe: [PATCH, v2] checkpatch: Warn on code with 6+ tab indentation, remove 80col warning
Quoted 9 lines · Ingo Molnar
Quoted 7 lines · Joe Perches
Quoted 5 lines · Ingo Molnar
Quoted 1 line · Joe Perches
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.
Quoted 5 lines
Quoted 2 lines
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.
Re: [PATCH, v2] checkpatch: Warn on code with 6+ tab indentation, remove 80col warning
Quoted 3 lines · Joe Perches
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.
Joe Perches
maintainerRe: [PATCH, v2] checkpatch: Warn on code with 6+ tab indentation, remove 80col warning
Quoted 8 lines · Pekka Enberg
Quoted 3 lines · Joe Perches
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:
scripts/checkpatch.pl +13 -6
@@ -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
Re: [PATCH, v2] checkpatch: Warn on code with 6+ tab indentation, remove 80col warning
Quoted 7 lines · Joe Perches
Quoted 3 lines
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.
Quoted 45 lines · Joe Perches
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:
scripts/checkpatch.pl +4
-1@@ -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 limitif ($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);
+ }
}
Quoted 1 line
# check for spaces before a quoted newline
I'm fine with something like this too.
Re: [PATCH, v2] checkpatch: Warn on code with 6+ tab indentation, remove 80col warning
In reply to Joe Perchesscripts/checkpatch.pl sub process { · line 1744
Quoted 9 lines · Joe Perches
scripts/checkpatch.pl +9
-0+ $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
Joe Perches
maintainerRe: [PATCH, v2] checkpatch: Warn on code with 6+ tab indentation, remove 80col warning
Quoted 1 line · Ingo Molnar
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.
Quoted 2 lines
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 -50gotta love the first few...
A few lines in arch/x86 might benefit from some wrapping too.
Re: [PATCH, v2] checkpatch: Warn on code with 6+ tab indentation, remove 80col warning
Quoted 6 lines · Joe Perches
Quoted 3 lines · Ingo Molnar
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
Joe Perches
maintainerPATCH 1/1 checkpatch: Add line-length options, set default to 100
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.
--- scripts/checkpatch.pl | 21 +++++++++++++++------ 1 files changed, 15 insertions(+), 6 deletions(-)
scripts/checkpatch.pl +15 -6
@@ -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
Re: [PATCH] checkpatch: Add line-length options, set default to 100
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
Re: [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums
In reply to Linus TorvaldsQuoted 4 lines · Linus Torvalds
Quoted 3 lines · Ingo Molnar
The overwhelming majority of kernel developers have stopped using 80 col terminals years ago.
There was more interesting text there. ;)
Quoted 2 lines
Quite frankly, I think we should still keep it at 80 columns.
...
Quoted 2 lines
So no, 100-char columns are not ok.
Thank you.
Signature · 2 lines
~Randy *** Remember to use Documentation/SubmitChecklist when testing your code ***
Re: [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums
In reply to Ingo MolnarQuoted 6 lines · Ingo Molnar
[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;}
Joe Perches
maintainerRe: [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums
Quoted 17 lines · Jan Engelhardt
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;
Quoted 1 line
}
I'd be OK with something like this while making the current line length check a --strict only option.
Re: [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums
In reply to Jan EngelhardtQuoted 10 lines · Jan Engelhardt
Quoted 6 lines · Ingo Molnar
[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.
Quoted 7 lines
(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...
PATCH 3/4 c/r: procfs: add arg_start/end, env_start/end and exit_code members to /proc/$pid/stat
In reply to Cyrill GorcunovWe would like to have an ability to restore command line arguments and envirion pointers so the task being restored would print appropriate values in /proc/pid/cmdline and /proc/pid/envirion. The exit_code is needed to restore zombie tasks.
--- Documentation/filesystems/proc.txt | 5 +++++ fs/proc/array.c | 25 ++++++++++++++++++++----- 2 files changed, 25 insertions(+), 5 deletions(-)
Documentation/filesystems/proc.txt +5 -0
@@ -311,6 +311,11 @@ Table 1-4: Contents of the stat files (a start_data address above which program data+bss is placed end_data address below which program data+bss is placed start_brk address above which program heap can be expanded with brk() + arg_start address above which program command line is placed + arg_end address below which program command line is placed + env_start address above which program environment is placed + env_end address below which program environment is placed + exit_code the thread's exit_code in the form reported by the waitpid system call .............................................................................. The /proc/PID/maps file containing the currently mapped memory regions and
fs/proc/array.c +20 -5
@@ -464,7 +464,7 @@ static int do_task_stat(struct seq_file seq_printf(m, "%d (%s) %c %d %d %d %d %d %u %lu \ %lu %lu %lu %lu %lu %ld %ld %ld %ld %d 0 %llu %lu %ld %lu %lu %lu %lu %lu \ -%lu %lu %lu %lu %lu %lu %lu %lu %d %d %u %u %llu %lu %ld %lu %lu %lu\n", +%lu %lu %lu %lu %lu %lu %lu %lu %d %d %u %u %llu %lu %ld ", pid_nr_ns(pid, ns), tcomm, state,
@@ -511,10 +511,25 @@ static int do_task_stat(struct seq_file task->policy, (unsigned long long )delayacct_blkio_ticks (task),cputime_to_clock_t (gtime), -cputime_to_clock_t (cgtime), - (mm && permitted) ? mm->start_data :0 , - (mm && permitted) ? mm->end_data :0 , - (mm && permitted) ? mm->start_brk : 0); + cputime_to_clock_t(cgtime)); + +if (mm && permitted) { +seq_printf (m,"%lu %lu %lu %lu %lu %lu %lu " , + mm->start_data, + mm->end_data, + mm->start_brk, + mm->arg_start, + mm->arg_end, + mm->env_start, + mm->env_end); + }else +seq_printf (m,"0 0 0 0 0 0 0 " ); + +if (permitted) +seq_printf (m,"%d\n" , task->exit_code); +else +seq_printf (m,"0\n" ); +if (mm)mmput (mm);return 0 ;
Re: [patch cr 3/4] c/r: procfs: add arg_start/end, env_start/end and exit_code members to /proc/$pid/stat
Quoted 30 lines · On Mon, 30 Jan 2012 18:09:08 +0400 Cyrill Gorcunov <[email protected]> wrote:
We would like to have an ability to restore command line arguments and envirion pointers so the task being restored would print appropriate values in
/proc/pid/cmdlineand/proc/pid/envirion. Theexit_codeis needed to restore zombie tasks....
fs/proc/array.c +4
-5@@ -464,7 +464,7 @@ static int do_task_stat(struct seq_file seq_printf(m, "%d (%s) %c %d %d %d %d %d %u %lu \ %lu %lu %lu %lu %lu %ld %ld %ld %ld %d 0 %llu %lu %ld %lu %lu %lu %lu %lu \ -%lu %lu %lu %lu %lu %lu %lu %lu %d %d %u %u %llu %lu %ld %lu %lu %lu\n", +%lu %lu %lu %lu %lu %lu %lu %lu %d %d %u %u %llu %lu %ld ", pid_nr_ns(pid, ns), tcomm, state,@@ -511,10 +511,25 @@ static int do_task_stat(struct seq_file task->policy, (unsigned long long )delayacct_blkio_ticks (task),cputime_to_clock_t (gtime), -cputime_to_clock_t (cgtime), - (mm && permitted) ? mm->start_data :0 , - (mm && permitted) ? mm->end_data :0 , - (mm && permitted) ? mm->start_brk : 0); + cputime_to_clock_t(cgtime)); + +if (mm && permitted) {
fs/proc/array.c static int do_task_stat(struct seq_file · line 526
Quoted 10 lines
fs/proc/array.c +10
-0+seq_printf (m,"%lu %lu %lu %lu %lu %lu %lu " , + mm->start_data, + mm->end_data, + mm->start_brk, + mm->arg_start, + mm->arg_end, + mm->env_start, + mm->env_end); + }else +seq_printf (m,"0 0 0 0 0 0 0 " );
heh.
We have a whizzy new num_to_str() in -mm which can later be used to speed this up.
This actually means that your patch will conflict horridly with procfs-add-num_to_str-to-speed-up-proc-stat.patch, because that patch redoes /proc/stat handling. I guess I can take care of that issue.
Quoted 9 lines
fs/proc/array.c +6
-0+ +if (permitted) +seq_printf (m,"%d\n" , task->exit_code); +else +seq_printf (m,"0\n" ); +if (mm)mmput (mm);return 0 ;
Re: [patch cr 3/4] c/r: procfs: add arg_start/end, env_start/end and exit_code members to /proc/$pid/stat
On Thu, Feb 02, 2012 at 03:26:48PM -0800, Andrew Morton wrote:
...
Quoted 5 lines
fs/proc/array.c static int do_task_stat(struct seq_file · line 526
Quoted 2 lines
fs/proc/array.c +2
-0+ }else +seq_printf (m,"0 0 0 0 0 0 0 " );heh.
Quoted 7 lines
We have a whizzy new
num_to_str()in -mm which can later be used to speed this up.This actually means that your patch will conflict horridly with procfs-add-num_to_str-to-speed-up-proc-stat.patch, because that patch redoes
/proc/stathandling. I guess I can take care of that issue.
I'll check linux-next version as well.
Cyrill
PATCH 4/4 c/r: prctl: Extend PR_SET_MM to set up more mm_struct entries
In reply to Cyrill GorcunovAfter restore we would like the 'ps' command show the command line and evironment exactly the same it was at checkpoint time.
So this additional PR_SET_MM_ allow us to do so. Note that these members of mm_struct is rather used for output in procfs, except auxv vector which is used by ld.so mostly.
--- include/linux/prctl.h | 5 ++ kernel/sys.c | 86 ++++++++++++++++++++++++++++++++------------------ 2 files changed, 61 insertions(+), 30 deletions(-)
include/linux/prctl.h +5 -0
@@ -113,5 +113,10 @@# define PR_SET_MM_START_STACK 5# define PR_SET_MM_START_BRK 6# define PR_SET_MM_BRK 7 +# define PR_SET_MM_ARG_START 8 +# define PR_SET_MM_ARG_END 9 +# define PR_SET_MM_ENV_START 10 +# define PR_SET_MM_ENV_END 11 +# define PR_SET_MM_AUXV 12#endif /* _LINUX_PRCTL_H */
kernel/sys.c +56 -30
137 lines not on this page · raw message
Re: [patch cr 4/4] c/r: prctl: Extend PR_SET_MM to set up more mm_struct entries
Quoted 6 lines · On Mon, 30 Jan 2012 18:09:09 +0400 Cyrill Gorcunov <[email protected]> wrote:
After restore we would like the 'ps' command show the command line and evironment exactly the same it was at checkpoint time.
So this additional
PR_SET_MM_allow us to do so. Note that these members ofmm_structis rather used for output in procfs, except auxv vector which is used by ld.so mostly.
This changelog is pretty darned hard to understand. Can we have a version 2 please?
Quoted 24 lines
...
kernel/sys.c +0
-13@@ -1753,19 +1755,6 @@ static int prctl_set_mm(int opt, unsigne mm->end_data = addr;break ; -case PR_SET_MM_START_STACK : - -#ifdef CONFIG_STACK_GROWSUP - vm_req_flags =VM_READ |VM_WRITE |VM_GROWSUP ; -#else - vm_req_flags =VM_READ |VM_WRITE |VM_GROWSDOWN ; -#endif -if ((vma->vm_flags & vm_req_flags) != vm_req_flags) - goto out; - - mm->start_stack = addr; -break ; -case PR_SET_MM_START_BRK :if (addr <= mm->end_data) goto out;
kernel/sys.c static int prctl_set_mm(int opt, unsigne · line 1780
Quoted 2 lines
kernel/sys.c +0
-0@@ -1790,16 +1779,53 @@ static int prctl_set_mm(int opt, unsigne mm->brk = addr;break ;
Here would be a good place to add some nice comments explaining what these do. Although I guess that isn't needed if one can get that info by typing "man prctl".
Quoted 29 lines
kernel/sys.c +29
-0+case PR_SET_MM_START_STACK : +case PR_SET_MM_ARG_START : +case PR_SET_MM_ARG_END : +case PR_SET_MM_ENV_START : +case PR_SET_MM_ENV_END : +#ifdef CONFIG_STACK_GROWSUP +if (vma_flags_mismatch (vma,VM_READ |VM_WRITE |VM_GROWSUP ,0 )) +#else + if (vma_flags_mismatch(vma, VM_READ | VM_WRITE | VM_GROWSDOWN, 0)) +#endif + goto out; +if (opt ==PR_SET_MM_START_STACK ) + mm->start_stack = addr; +else if (opt ==PR_SET_MM_ARG_START ) + mm->arg_start = addr; +else if (opt ==PR_SET_MM_ARG_END ) + mm->arg_end = addr; +else if (opt ==PR_SET_MM_ENV_START ) + mm->env_start = addr; +else if (opt ==PR_SET_MM_ENV_END ) + mm->env_end = addr; +break ; + +case PR_SET_MM_AUXV : { +unsigned long user_auxv[AT_VECTOR_SIZE ]; + +if (arg4 >sizeof (mm->saved_auxv)) + goto out; +up_read (&mm->mmap_sem);
kernel/sys.c static int prctl_set_mm(int opt, unsigne · line 1820
Quoted 10 lines
kernel/sys.c +10
-0+ +if (copy_from_user (user_auxv, (const void __user *)addr, arg4)) +return EFAULT ; + +task_lock (current); +memcpy (mm->saved_auxv, user_auxv, arg4); +task_unlock (current); + +return 0 ; + }
I worry a bit about this. We're giving userspace the ability to modify various mm_struct fields. Userspace can already do this via exec(elf-file), but perhaps this opens up a way in which userspace can newly trigger kernel bugs.
Re: [patch cr 4/4] c/r: prctl: Extend PR_SET_MM to set up more mm_struct entries
Quoted 6 lines · Andrew Morton
Quoted 3 lines · On Mon, 30 Jan 2012 18:09:09 +0400 Cyrill Gorcunov <[email protected]> wrote:
After restore we would like the 'ps' command show the command line and evironment exactly the same it was at checkpoint time.
Quoted 7 lines
Quoted 3 lines
So this additional
PR_SET_MM_allow us to do so. Note that these members ofmm_structis rather used for output in procfs, except auxv vector which is used by ld.so mostly.This changelog is pretty darned hard to understand. Can we have a version 2 please?
yeah, will update.
...
Quoted 8 lines
kernel/sys.c static int prctl_set_mm(int opt, unsigne · line 1780
Quoted 3 lines
kernel/sys.c +0
-0@@ -1790,16 +1779,53 @@ static int prctl_set_mm(int opt, unsigne mm->brk = addr;break ;Here would be a good place to add some nice comments explaining what these do. Although I guess that isn't needed if one can get that info by typing "man prctl".
I started cooking prctl man pages but found hardness to explain some regular user who has no ideas about kernel internals why do we modify mm_struct data, still I'm trying.
And I'll add comment here (since having it here in-place allows reader to not read man page ;)
...
Quoted 6 lines
I worry a bit about this. We're giving userspace the ability to modify various
mm_structfields. Userspace can already do this via exec(elf-file), but perhaps this opens up a way in which userspace can newly trigger kernel bugs.
At moment there is no more way to modify these fields other than elf handler, but in future... hard to predict what else there will be done and where also these fields appear in kernel code. but as i said at moment this modification is pretty safe and even if one write some buggy values -- he simply get weird output in /proc/ statistics and such.
Cyrill
Quoted 14 lines · On Mon, 30 Jan 2012 18:09:05 +0400 Cyrill Gorcunov <[email protected]> wrote:
Hi,
this is a series where I tried to resolve all concerns and nits I've got from previous attempt:
- fs, proc: Introduce /proc/<pid>/task/<tid>/children entry v9 Now everything is under
CONFIG_CHECKPOINT_RESTORE- syscalls, x86: Add
__NR_kcmpsyscall v7
* EBADF returned if there is no file
* EOPNOTSUP returned if there is no sysv support compiled in
* test usessyscall()from libc instead of own syscall implementation
* man page for this syscall is under progress still
Quoted 8 lines
- c/r: procfs: add arg_start/end, env_start/end and
exit_codemembers to/proc/$pid/stattask->exit_code is shown IIF a caller has enough permissions- c/r: prctl: Extend
PR_SET_MMto set up moremm_structentrieswhen restoring mm->saved_auxv the task_lock is used to serialize accessPlease review, if there is something I'm still missing, ping me back.
Looks pretty close to me. I had a few issues and niggles. Please cc me on v10 and unless someone squawks convincingly I will grab it.