This fixes all the checkpatch --file complaints about kernel/ptrace.c
and also removes an unused #include. I've verified that there are no
changes to the compiled code on x86_64.
Signed-off-by: Roland McGrath <[email protected]>
---
kernel/ptrace.c | 25 +++++++++++++------------
1 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index aaad0ec..dbe79ef 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -21,9 +21,7 @@
#include <linux/audit.h>
#include <linux/pid_namespace.h>
#include <linux/syscalls.h>
-
-#include <asm/pgtable.h>
-#include <asm/uaccess.h>
+#include <linux/uaccess.h>
/*
@@ -48,7 +46,7 @@ void __ptrace_link(struct task_struct *child, struct task_struct *new_parent)
list_add(&child->ptrace_entry, &new_parent->ptraced);
child->parent = new_parent;
}
-
+
/*
* Turn a tracing stop into a normal stop now, since with no tracer there
* would be no way to wake it up with SIGCONT or SIGKILL. If there was a
@@ -173,7 +171,7 @@ bool ptrace_may_access(struct task_struct *task, unsigned int mode)
task_lock(task);
err = __ptrace_may_access(task, mode);
task_unlock(task);
- return (!err ? true : false);
+ return !err;
}
int ptrace_attach(struct task_struct *task)
@@ -338,7 +336,8 @@ void exit_ptrace(struct task_struct *tracer)
}
}
-int ptrace_readdata(struct task_struct *tsk, unsigned long src, char __user *dst, int len)
+int ptrace_readdata(struct task_struct *tsk, unsigned long src,
+ char __user *dst, int len)
{
int copied = 0;
@@ -358,12 +357,13 @@ int ptrace_readdata(struct task_struct *tsk, unsigned long src, char __user *dst
copied += retval;
src += retval;
dst += retval;
- len -= retval;
+ len -= retval;
}
return copied;
}
-int ptrace_writedata(struct task_struct *tsk, char __user *src, unsigned long dst, int len)
+int ptrace_writedata(struct task_struct *tsk, char __user *src,
+ unsigned long dst, int len)
{
int copied = 0;
@@ -383,7 +383,7 @@ int ptrace_writedata(struct task_struct *tsk, char __user *src, unsigned long ds
copied += retval;
src += retval;
dst += retval;
- len -= retval;
+ len -= retval;
}
return copied;
}
@@ -496,9 +496,9 @@ static int ptrace_resume(struct task_struct *child, long request, long data)
if (unlikely(!arch_has_single_step()))
return -EIO;
user_enable_single_step(child);
- }
- else
+ } else {
user_disable_single_step(child);
+ }
child->exit_code = data;
wake_up_process(child);
@@ -527,7 +527,8 @@ int ptrace_request(struct task_struct *child, long request,
ret = ptrace_setoptions(child, data);
break;
case PTRACE_GETEVENTMSG:
- ret = put_user(child->ptrace_message, (unsigned long __user *) data);
+ ret = put_user(child->ptrace_message,
+ (unsigned long __user *) data);
break;
case PTRACE_GETSIGINFO:
On Wed, Apr 8, 2009 at 3:21 AM, Roland McGrath <[email protected]> wrote:
> This fixes all the checkpatch --file complaints about kernel/ptrace.c
> and also removes an unused #include. ?I've verified that there are no
> changes to the compiled code on x86_64.
silly question, but... how do you verify that? is there any tool under
scripts/ that do that comparison?
thanks,
sergio
>
> Signed-off-by: Roland McGrath <[email protected]>
> ---
> ?kernel/ptrace.c | ? 25 +++++++++++++------------
> ?1 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index aaad0ec..dbe79ef 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -21,9 +21,7 @@
> ?#include <linux/audit.h>
> ?#include <linux/pid_namespace.h>
> ?#include <linux/syscalls.h>
> -
> -#include <asm/pgtable.h>
> -#include <asm/uaccess.h>
> +#include <linux/uaccess.h>
>
>
> ?/*
> @@ -48,7 +46,7 @@ void __ptrace_link(struct task_struct *child, struct task_struct *new_parent)
> ? ? ? ?list_add(&child->ptrace_entry, &new_parent->ptraced);
> ? ? ? ?child->parent = new_parent;
> ?}
> -
> +
> ?/*
> ?* Turn a tracing stop into a normal stop now, since with no tracer there
> ?* would be no way to wake it up with SIGCONT or SIGKILL. ?If there was a
> @@ -173,7 +171,7 @@ bool ptrace_may_access(struct task_struct *task, unsigned int mode)
> ? ? ? ?task_lock(task);
> ? ? ? ?err = __ptrace_may_access(task, mode);
> ? ? ? ?task_unlock(task);
> - ? ? ? return (!err ? true : false);
> + ? ? ? return !err;
> ?}
>
> ?int ptrace_attach(struct task_struct *task)
> @@ -338,7 +336,8 @@ void exit_ptrace(struct task_struct *tracer)
> ? ? ? ?}
> ?}
>
> -int ptrace_readdata(struct task_struct *tsk, unsigned long src, char __user *dst, int len)
> +int ptrace_readdata(struct task_struct *tsk, unsigned long src,
> + ? ? ? ? ? ? ? ? ? char __user *dst, int len)
> ?{
> ? ? ? ?int copied = 0;
>
> @@ -358,12 +357,13 @@ int ptrace_readdata(struct task_struct *tsk, unsigned long src, char __user *dst
> ? ? ? ? ? ? ? ?copied += retval;
> ? ? ? ? ? ? ? ?src += retval;
> ? ? ? ? ? ? ? ?dst += retval;
> - ? ? ? ? ? ? ? len -= retval;
> + ? ? ? ? ? ? ? len -= retval;
> ? ? ? ?}
> ? ? ? ?return copied;
> ?}
>
> -int ptrace_writedata(struct task_struct *tsk, char __user *src, unsigned long dst, int len)
> +int ptrace_writedata(struct task_struct *tsk, char __user *src,
> + ? ? ? ? ? ? ? ? ? ?unsigned long dst, int len)
> ?{
> ? ? ? ?int copied = 0;
>
> @@ -383,7 +383,7 @@ int ptrace_writedata(struct task_struct *tsk, char __user *src, unsigned long ds
> ? ? ? ? ? ? ? ?copied += retval;
> ? ? ? ? ? ? ? ?src += retval;
> ? ? ? ? ? ? ? ?dst += retval;
> - ? ? ? ? ? ? ? len -= retval;
> + ? ? ? ? ? ? ? len -= retval;
> ? ? ? ?}
> ? ? ? ?return copied;
> ?}
> @@ -496,9 +496,9 @@ static int ptrace_resume(struct task_struct *child, long request, long data)
> ? ? ? ? ? ? ? ?if (unlikely(!arch_has_single_step()))
> ? ? ? ? ? ? ? ? ? ? ? ?return -EIO;
> ? ? ? ? ? ? ? ?user_enable_single_step(child);
> - ? ? ? }
> - ? ? ? else
> + ? ? ? } else {
> ? ? ? ? ? ? ? ?user_disable_single_step(child);
> + ? ? ? }
>
> ? ? ? ?child->exit_code = data;
> ? ? ? ?wake_up_process(child);
> @@ -527,7 +527,8 @@ int ptrace_request(struct task_struct *child, long request,
> ? ? ? ? ? ? ? ?ret = ptrace_setoptions(child, data);
> ? ? ? ? ? ? ? ?break;
> ? ? ? ?case PTRACE_GETEVENTMSG:
> - ? ? ? ? ? ? ? ret = put_user(child->ptrace_message, (unsigned long __user *) data);
> + ? ? ? ? ? ? ? ret = put_user(child->ptrace_message,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?(unsigned long __user *) data);
> ? ? ? ? ? ? ? ?break;
>
> ? ? ? ?case PTRACE_GETSIGINFO:
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/
>
On Tue, 7 Apr 2009, Roland McGrath wrote:
>
> This fixes all the checkpatch --file complaints about kernel/ptrace.c
> and also removes an unused #include. I've verified that there are no
> changes to the compiled code on x86_64.
Please don't bother with that insane "line length" option when using
"--file". At least not if the "fix" is to just mindlessly split the line.
That is _never_ a fix.
Changes like these:
> -int ptrace_readdata(struct task_struct *tsk, unsigned long src, char __user *dst, int len)
> +int ptrace_readdata(struct task_struct *tsk, unsigned long src,
> + char __user *dst, int len)
> -int ptrace_writedata(struct task_struct *tsk, char __user *src, unsigned long dst, int len)
> +int ptrace_writedata(struct task_struct *tsk, char __user *src,
> + unsigned long dst, int len)
> case PTRACE_GETEVENTMSG:
> - ret = put_user(child->ptrace_message, (unsigned long __user *) data);
> + ret = put_user(child->ptrace_message,
> + (unsigned long __user *) data);
just make the code harder to 'grep'.
Yes, at some point you have to split lines, but that point is not 80
columns any more. The advantage of getting the whole line when grepping
for function names much outweighs the downside of somebody using those
old 80x24 green phosphorous vt52's.
[ The same thing very much goes for complex if-statements etc. If
people can't stand the long lines, the primary solution would be to
turn a complex conditional into a helper inline functions, or to fix
excessive indentation by splitting up functions.
In the above case, the last one could perhaps have been handled
creating a new variable for and moving the cast to the initialiser,
for example. Is it worth it to avoid a 85-column line? Probably not.
And some lines just end up long. I think 100 characters may be a
more reasonable limit for "too long", but quite frankly, it depends on
the line.
So I think 'checkpatch' is pure crap in this area, and I've told
people so before, and they keep telling me that it has relaxed it's
idiotic warnings, but that is apparently just a lie. ]
Oh well. If I actually read perl, I could parse what the hell those
80-character rules are in checkpath. It already has random "it's ok if
X" stuff. But it never seems to really have any "oh, but splitting is
worse" logic.
Linus
Am Wednesday 08 April 2009 19:19:36 schrieb Linus Torvalds:
> And some lines just end up long. I think 100 characters may be a
> more reasonable limit for "too long", but quite frankly, it depends on
> the line.
>
> So I think 'checkpatch' is pure crap in this area, and I've told
> people so before, and they keep telling me that it has relaxed it's
> idiotic warnings, but that is apparently just a lie. ]
>
> Oh well. If I actually read perl, I could parse what the hell those
> 80-character rules are in checkpath. It already has random "it's ok if
> X" stuff. But it never seems to really have any "oh, but splitting is
> worse" logic.
Isnt checkpatch just following what is written down in the Documentation folder? Maybe adopting the following part of CodingStyle and add more examples for good and bad would give the checkpatch authors a better idea about your intent.
--------- snip---------
Chapter 2: Breaking long lines and strings
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
preferred limit.
Statements longer than 80 columns will be broken into sensible chunks.
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. Long strings are as well broken into shorter strings. The
only exception to this is where exceeding 80 columns significantly increases
readability and does not hide information.
void fun(int a, int b, int c)
{
if (condition)
printk(KERN_WARNING "Warning this is a long printk with "
"3 parameters a: %u b: %u "
"c: %u \n", a, b, c);
else
next_statement;
}
----------snip---------
> silly question, but... how do you verify that? is there any tool under
> scripts/ that do that comparison?
I didn't use any script, there might be done, I don't know.
I compared "objdump -rd" output on the .o file before and after, using diff.
Thanks,
Roland
On Wed, 8 Apr 2009, Christian Borntraeger wrote:
>
> Isnt checkpatch just following what is written down in the Documentation
> folder? Maybe adopting the following part of CodingStyle and add more
> examples for good and bad would give the checkpatch authors a better
> idea about your intent.
The thing is, it's true that it's good if things fit in 80 columns.
But _splitting_ lines isn't the answer. Making code simpler is, but
somehow the 80-column warning never causes that to happen - instead people
just split.
And yes, I guess we should remove the language saying so. It's not from
my original coding stule, it was added later by others, and came through
Andrew (commit 560362dafe4de60db70f2c298a53f4613453a78b: "[PATCH]
Codingstyle update" in the historical Linux archive).
Linus
On Wed, Apr 8, 2009 at 9:50 AM, Roland McGrath <[email protected]> wrote:
>> silly question, but... how do you verify that? is there any tool under
>> scripts/ that do that comparison?
>
> I didn't use any script, there might be done, I don't know.
> I compared "objdump -rd" output on the .o file before and after, using diff.
>
>
> Thanks,
> Roland
>
Ah, I see. I had written a silly objdiff scritp that was using objdump
-D output for comparison, and there were a few differences, that's why
I was wondering what would be the right way to verify that.
Thanks,
Sergio.
* Linus Torvalds <[email protected]> wrote:
> Oh well. If I actually read perl, I could parse what the hell
> those 80-character rules are in checkpath. It already has random
> "it's ok if X" stuff. But it never seems to really have any "oh,
> but splitting is worse" logic.
We should perhaps introduce an too-deep-indentation warning: any
function with "[;{}]$" lines of 4 tabs in a row is already suspect
IMHO. At 5 it's definitely crazy and ugly.
This would be a very efficient function-length reductor: it cannot
be worked around via line wraps.
It would also be wonderful to warn about bad 80 columns 'fixes' -
i've seen way too many perfectly fine cleanups damaged by ugly
line-wrapping solutions.
We could also up the limit to 90 or 100 columns. My terminals are at
90 columns and that's still pretty ergonomic. 100 is too wide to me
personally. (i'd argue that lines longer than 100 fall outside the
brain's 'field of view' cache and are beyond a general complexity
threshold as well, so they are not efficient to read, regardless of
monitor size and quality.)
Ingo
Ingo Molnar <[email protected]> writes:
> We could also up the limit to 90 or 100 columns. My terminals are at
> 90 columns and that's still pretty ergonomic. 100 is too wide to me
> personally. (i'd argue that lines longer than 100 fall outside the
> brain's 'field of view' cache and are beyond a general complexity
> threshold as well, so they are not efficient to read, regardless of
> monitor size and quality.)
I agree that _really_ wide lines start to make the code unreadable.
I find 80-column terminals/editor-windows still quite useful because
you can fit two of them side-by-side on almost every display, and I
think it's more useful to have multiple display contexts active than
it is to have huge amounts of whitespace at the ends of all my lines
(after all, comments aside, most code doesn't seem to be all _that_
wide -- the issue here is what happens with the minority that is).
Maybe given a blank slate, 90 columns or so might fit current coding
practices slightly better, but 80-columns seems to still be the
default in many cases (if you just say "xterm" you'll get an 80-column
terminal), and is in practice a very reliable _minimum_, so if you
want to avoid annoying people with hard-wrapping, 80 columns seems
like a pretty reasonable standard width...
-Miles
--
Politics, n. A strife of interests masquerading as a contest of
principles. The conduct of public affairs for private advantage.
On Thu, 9 Apr 2009, Ingo Molnar wrote:
>
> We should perhaps introduce an too-deep-indentation warning: any
> function with "[;{}]$" lines of 4 tabs in a row is already suspect
> IMHO. At 5 it's definitely crazy and ugly.
>
> This would be a very efficient function-length reductor: it cannot
> be worked around via line wraps.
People would start using spaces to try to work around it instead, which is
a worse cure than the problem.
Also, the thing is, a long _individual_ line is not a problem even if you
have a 80-column terminal. Sane editors will have a marker for "this line
continues", and even if you have an insane editors that doesn't do that,
it's pretty obvious - and if you really care about the end of that
_particular_ line (most of the time you don't), you can just move to that
line.
So if you have a couple of long lines occasionally, that's not a huge
problem. In fact, that's why I hate splitting lines so much: the "false
indentation" that a line split causes is generally much more confusing
visually (not so much in something like a function header, but often very
much so inside the code itself).
> It would also be wonderful to warn about bad 80 columns 'fixes' -
> i've seen way too many perfectly fine cleanups damaged by ugly
> line-wrapping solutions.
The thing is, it's very hard to warn about those. You need more
understanding than your average perl-script can ever get.
> We could also up the limit to 90 or 100 columns. My terminals are at
> 90 columns and that's still pretty ergonomic.
I tend to start out with a 80x24 and just resize it, and end up at some
random value. It's usually in the 90x40 range for me. But I do want the
code to be perfectly _readable_ in a 80x24 window, and quite frankly, if
you look at something like kernel/ptrace.c, it really generally is.
So sure, that "int ptrace_readdata()" line is longer than that, and won't
show completely. But you don't miss any huge glaring code issues even in
the truncated mode. In fact, if I try to use 80x24, my biggest issue will
inevitably be not the 80 part, but the 24 part.
IOW, I think there is much more reason to hate long _functions_ than there
is reason to hate long lines. Both cause you to scroll. The long function
where there is action over more than 24 lines happens a lot more.
Linus