Re: [PATCH] reformat_with_checkpatch: Add automation to checkpatch

[Joe Perches]

On Sat, 2014-07-12 at 12:28 -0700, Greg KH wrote:
> On Sat, Jul 12, 2014 at 11:29:37AM -0700, Joe Perches wrote:
> > On Sat, 2014-07-12 at 10:55 -0700, Greg KH wrote:
> > > Yes, the warnings are all due to line-length, but Joe, you shouldn't add
> > > a patch that causes more checkpatch warnings than before :)
> > 
> > Yeah, that was intentional though.
> > 
> > This script does a series of relatively discrete changes.
> > 
> > Lindent would more or less work, but it's _horrible_ at
> > wrapping overlong lines and merges all types of changes
> > together.
> 
> Oh I agree, I don't want to see Lindent, but maybe, if the patch adds
> checkpatch warnings, it should be at least "flagged" as maybe a problem?

The script I submitted does run checkpatch
before any commit is done.

> Would you want to be on the receiving end of this patch script?

You might have noticed I don't have a kernel.org
account.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] reformat_with_checkpatch: Add automation to checkpatch

[Greg KH]

On Sat, Jul 12, 2014 at 11:29:37AM -0700, Joe Perches wrote:
> On Sat, 2014-07-12 at 10:55 -0700, Greg KH wrote:
> > Yes, the warnings are all due to line-length, but Joe, you shouldn't add
> > a patch that causes more checkpatch warnings than before :)
> 
> Yeah, that was intentional though.
> 
> This script does a series of relatively discrete changes.
> 
> Lindent would more or less work, but it's _horrible_ at
> wrapping overlong lines and merges all types of changes
> together.

Oh I agree, I don't want to see Lindent, but maybe, if the patch adds
checkpatch warnings, it should be at least "flagged" as maybe a problem?

> > While I always want to see more developers get involved with kernel
> > development, there should be a minimum barrier to entry.  And that
> > barrier is the knowledge of the C language, and knowledge of how to edit
> > a text file, and use git.  This script takes that barrier away, for
> > whitespace cleanups, with not much real use overall.
> > 
> > So, I'll keep my local copy of this script now, just to have fun with at
> > times when I'm bored.  But I don't think it should be merged, as-is.
> 
> Dunno, I still think it's useful.

For you, and me, but the world?

Would you want to be on the receiving end of this patch script?  I
don't, and I'm willing to take almost any patch cleanup for staging
code.  I think that says something :)

> Maybe when you get new code, you might run it through a
> script like this before committing it.

I will keep it for me, like I said.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] reformat_with_checkpatch: Add automation to checkpatch

[Joe Perches]

On Sat, 2014-07-12 at 10:55 -0700, Greg KH wrote:
> Yes, the warnings are all due to line-length, but Joe, you shouldn't add
> a patch that causes more checkpatch warnings than before :)

Yeah, that was intentional though.

This script does a series of relatively discrete changes.

Lindent would more or less work, but it's _horrible_ at
wrapping overlong lines and merges all types of changes
together.

For now, people are probably better at line wrapping in
an acceptable way.

> While I always want to see more developers get involved with kernel
> development, there should be a minimum barrier to entry.  And that
> barrier is the knowledge of the C language, and knowledge of how to edit
> a text file, and use git.  This script takes that barrier away, for
> whitespace cleanups, with not much real use overall.
> 
> So, I'll keep my local copy of this script now, just to have fun with at
> times when I'm bored.  But I don't think it should be merged, as-is.

Dunno, I still think it's useful.

Maybe when you get new code, you might run it through a
script like this before committing it.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] reformat_with_checkpatch: Add automation to checkpatch

[Greg KH]

On Sat, Jul 12, 2014 at 12:30:43PM +0300, Dan Carpenter wrote:
> On Fri, Jul 11, 2014 at 06:40:16PM -0700, Joe Perches wrote:
> > On Fri, 2014-07-11 at 18:34 -0700, Greg KH wrote:
> > > On Fri, Jul 11, 2014 at 06:21:27PM -0700, Joe Perches wrote:
> > > > A simple script to run checkpatch --fix for various types of
> > > > of cleanups.
> > > > 
> > > > This script is useful primarily for staging.
> > > > 
> > > > This reformats code to a more CodingStyle conforming style,
> > > > compiles it, verifies that the object code hasn't changed,
> > > > and git commits it too.
> > > 
> > > And 'git commits' it?
> > 
> > The thought I had was to made it easier to
> > submit "my first kernel patch" correctly.
> > like that tuxradar article you wrote a few
> > years ago.
> > 
> > http://www.tuxradar.com/content/newbies-guide-hacking-linux-kernel
> > 
> > > Heh, I should just run this myself to clean up
> > > staging and beat everyone else to it...
> > 
> > At least for the whitespace noise, you could
> > but I hope it'll encourage a few more people
> > to try kernel patching instead.
> 
> I have always hate the idea of automated patches from random people
> because I don't trust them to not be malicious so I have to review them
> manually.  There is a story that back in the day the US government paid
> someone to send tons of checkpatch style patches to BSD.  The guy
> thought he was "cleaning up the code" but actually he was part of a
> larger scheme to flood the maintainers with patches so another person
> could slip in some malicious code.

Based on some of the patches that I get sent, I wouldn't be surprised if
that's already happening :(

> It's better if someone just ran this on all new staging code before
> adding it to the tree.

I spent some time messing with this script today, and while it is "fun"
to run, I don't think it's all that useful.

One example, before running the script:

~/linux/work/staging $ ./scripts/checkpatch.pl --terse --file 
drivers/staging/android/binder.c | tail -n 1
total: 0 errors, 103 warnings, 3670 lines checked

After running it:

~/linux/work/staging $ ./scripts/checkpatch.pl --terse --file 
drivers/staging/android/binder.c | tail -n 1
total: 0 errors, 125 warnings, 3670 lines checked

And that was after the script created two patches, with the resulting
diffstat of:

 drivers/staging/android/binder.c |  124 +++
 1 file changed, 62 insertions(+), 62 deletions(-)

So 2 patches, 60+ lines to review, and 22 more warnings from checkpatch
as the end result?

Yes, the warnings are all due to line-length, but Joe, you shouldn't add
a patch that causes more checkpatch warnings than before :)


I know people have scripts like this of their own, and while it might be
nice to "standardize" them, I am really reluctant to put this script in
the kernel tree itself.  There's a barrier of entry to write your own
type of script here that honestly, I like.

We already have the example of someone who obviously does not know the C
language at all, running through the kernel tree at the moment, creating
horrible patches that are flat-out wrong and annoying maintainers with
their result.  I've had to kill-file them for now, as it was just too
annoying and maintainer time is what we have the least of.

While I always want to see more developers get involved with kernel
development, there should be a minimum barrier to entry.  And that
barrier is the knowledge of the C language, and knowledge of how to edit
a text file, and use git.  This script takes that barrier away, for
whitespace cleanups, with not much real use overall.

So, I'll keep my local copy of this script now, just to have fun with at
times when I'm bored.  But I don't think it should be merged, as-is.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] reformat_with_checkpatch: Add automation to checkpatch

[Joe Perches]

On Sat, 2014-07-12 at 01:18 -0700, Greg KH wrote:
> On Fri, Jul 11, 2014 at 06:21:27PM -0700, Joe Perches wrote:
> > A simple script to run checkpatch --fix for various types of
> > of cleanups.
[]
> I did the following:
> 
> $ scripts/reformat_with_checkpatch.sh drivers/base/bus.c
> 
> Ignore the first set of things it tries to commit by answering N to the
> "Would you like to commit these changes".
> 
> Then the second thing it tries to change in the file says that there is
> a .o file difference.
> 
> Yet the diff is below, I don't see how this happens.  Is this due to
> there being some old temp file around because I did not accept the first
> set of changes?

[]

> What the script complained about:
> 
> Comparing objects...
> --- drivers/base/bus.o.new2014-07-12 01:16:32.984755945 -0700
> +++ drivers/base/bus.o.old2014-07-12 01:16:31.924755967 -0700
> @@ -2449,13 +2449,13 @@
>  
>   :
>   ...
> -:bf 03 00 00 00  mov$0x3,%edi
> +:be 03 00 00 00  mov$0x3,%esi
>  :00 00   add%al,(%rax)
>   ...
>  
>  0028 :
>   ...
> -:a2 03 00 00 00 00 00movabs %al,0x3
> +:a1 03 00 00 00 00 00movabs 0x3,%eax
>  :00 00 
>  
>  0050 :
> @@ -2468,7 +2468,7 @@
>  
>  0078 :
>   ...
> -:56  push   %rsi
> +:57  push   %rdi
>  :00 00   add%al,(%rax)
>  :00 00   add%al,(%rax)
>  :00 00   add%al,(%rax)
> Object differences exist! - Verify changes before commit!
> 
> 

I've no real idea, but this looks more like
non-repeatable gcc compilation output to me.

Here's my script output from that sequence.

$ gcc --version
gcc (Ubuntu 4.8.2-19ubuntu1) 4.8.2
Copyright (C) 2013 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ scripts/reformat_with_checkpatch.sh drivers/base/bus.c
file:  description:  
types:
total: 0 errors, 0 warnings, 0 checks, 1272 lines checked

NOTE: Used message types: BRACKET_SPACE POINTER_LOCATION SPACE_BEFORE_TAB 
SPACING TRAILING_WHITESPACE

drivers/base/bus.c has no obvious style problems and is ready for submission.
file:  description:  
types:
total: 0 errors, 0 warnings, 0 checks, 1272 lines checked

NOTE: Used message types: SPACE_BEFORE_TAB

drivers/base/bus.c has no obvious style problems and is ready for submission.
file:  description:  
types:
total: 0 errors, 0 warnings, 0 checks, 1272 lines checked

NOTE: Used message types: INDENTED_LABEL

drivers/base/bus.c has no obvious style problems and is ready for submission.
file:  description:  
types:
CHECK: Alignment should match open parenthesis
#37: FILE: drivers/base/bus.c:37:
+static int __must_check bus_rescan_devices_helper(struct device *dev,
+   void *data);

CHECK: Alignment should match open parenthesis
#518: FILE: drivers/base/bus.c:518:
+   error = sysfs_create_link(&bus->p->devices_kset->kobj,
+   &dev->kobj, dev_name(dev));

CHECK: Alignment should match open parenthesis
#522: FILE: drivers/base/bus.c:522:
+   error = sysfs_create_link(&dev->kobj,
+   &dev->bus->p->subsys.kobj, "subsystem");

CHECK: Alignment should match open parenthesis
#701: FILE: drivers/base/bus.c:701:
+   printk(KERN_ERR "%s: uevent attr (%s) failed\n",
+   __func__, drv->name);

CHECK: Alignment should match open parenthesis
#707: FILE: drivers/base/bus.c:707:
+   printk(KERN_ERR "%s: driver_create_groups(%s) failed\n",
+   __func__, drv->name);

CHECK: Alignment should match open parenthesis
#715: FILE: drivers/base/bus.c:715:
+   printk(KERN_ERR "%s: add_bind_files(%s) failed\n",
+   __func__, drv->name);

CHECK: Alignment should match open parenthesis
#1003: FILE: drivers/base/bus.c:1003:
+   int (*compare)(const struct device *a,
+   const struct device *b))

total: 0 errors, 0 warnings, 7 checks, 1272 lines checked

NOTE: Used message types: PARENTHESIS_ALIGNMENT

Wrote EXPERIMENTAL --fix correction(s) to 'drivers/base/bus.c'

Do _NOT_ trust the results written to this file.
Do _NOT_ submit these changes without inspecting them for correctness.

This EXPERIMENTAL file is simply a convenience to help rewrite patches.
No warranties, expressed or implied...

drivers/base/bus.c has style problems, please review.

If any of these errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
Compiling original version...
  HOSTCC  scripts/kconfig/conf.o
  HOSTCC  scripts/kconfig/zconf.tab.o
  HOSTLD  

Re: [PATCH] reformat_with_checkpatch: Add automation to checkpatch

[Dan Carpenter]

On Fri, Jul 11, 2014 at 06:40:16PM -0700, Joe Perches wrote:
> On Fri, 2014-07-11 at 18:34 -0700, Greg KH wrote:
> > On Fri, Jul 11, 2014 at 06:21:27PM -0700, Joe Perches wrote:
> > > A simple script to run checkpatch --fix for various types of
> > > of cleanups.
> > > 
> > > This script is useful primarily for staging.
> > > 
> > > This reformats code to a more CodingStyle conforming style,
> > > compiles it, verifies that the object code hasn't changed,
> > > and git commits it too.
> > 
> > And 'git commits' it?
> 
> The thought I had was to made it easier to
> submit "my first kernel patch" correctly.
> like that tuxradar article you wrote a few
> years ago.
> 
> http://www.tuxradar.com/content/newbies-guide-hacking-linux-kernel
> 
> > Heh, I should just run this myself to clean up
> > staging and beat everyone else to it...
> 
> At least for the whitespace noise, you could
> but I hope it'll encourage a few more people
> to try kernel patching instead.

I have always hate the idea of automated patches from random people
because I don't trust them to not be malicious so I have to review them
manually.  There is a story that back in the day the US government paid
someone to send tons of checkpatch style patches to BSD.  The guy
thought he was "cleaning up the code" but actually he was part of a
larger scheme to flood the maintainers with patches so another person
could slip in some malicious code.

It's better if someone just ran this on all new staging code before
adding it to the tree.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] reformat_with_checkpatch: Add automation to checkpatch

[Greg KH]

On Fri, Jul 11, 2014 at 06:21:27PM -0700, Joe Perches wrote:
> A simple script to run checkpatch --fix for various types of
> of cleanups.
> 
> This script is useful primarily for staging.
> 
> This reformats code to a more CodingStyle conforming style,
> compiles it, verifies that the object code hasn't changed,
> and git commits it too.
> 
> You must have the necessary development tools, git, and a
> recent git tree.  Ideally use Greg KH's staging-next, which
> can be retrieved via these commands:
> 
> git clone git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
> git checkout staging-next
> 
> To use this script try a sequence of commands like:
> 
>   cd 
>   git checkout -b 
>   make allyesconfig
>   mkdir patches
>   ./scripts/reformat_with_checkpatch.sh drivers/staging//*.[ch]
>   git format-patch --cover-letter -o patches/ staging-next
>   git send-email patches/

When running this on drivers/base/bus.c, it says that the .o files are
different, when the diffstat for what makes them different is only
whitespace.

I did the following:

$ scripts/reformat_with_checkpatch.sh drivers/base/bus.c

Ignore the first set of things it tries to commit by answering N to the
"Would you like to commit these changes".

Then the second thing it tries to change in the file says that there is
a .o file difference.

Yet the diff is below, I don't see how this happens.  Is this due to
there being some old temp file around because I did not accept the first
set of changes?

thanks,

greg k-h

-
diff:

 drivers/base/bus.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 83e910a57563..3546d02b46f0 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -32,7 +32,6 @@ static struct kset *system_kset;
 
 #define to_drv_attr(_attr) container_of(_attr, struct driver_attribute, attr)
 
-
 static int __must_check bus_rescan_devices_helper(struct device *dev,
void *data);
 
@@ -128,6 +127,7 @@ static const struct sysfs_ops bus_sysfs_ops = {
 int bus_create_file(struct bus_type *bus, struct bus_attribute *attr)
 {
int error;
+
if (bus_get(bus)) {
error = sysfs_create_file(&bus->p->subsys.kobj, &attr->attr);
bus_put(bus);
@@ -817,6 +817,7 @@ EXPORT_SYMBOL_GPL(device_reprobe);
 struct bus_type *find_bus(char *name)
 {
struct kobject *k = kset_find_obj(bus_kset, name);
+
return k ? to_bus(k) : NULL;
 }
 #endif  /*  0  */

--


What the script complained about:

Comparing objects...
--- drivers/base/bus.o.new  2014-07-12 01:16:32.984755945 -0700
+++ drivers/base/bus.o.old  2014-07-12 01:16:31.924755967 -0700
@@ -2449,13 +2449,13 @@
 
  :
...
-:  bf 03 00 00 00  mov$0x3,%edi
+:  be 03 00 00 00  mov$0x3,%esi
 :  00 00   add%al,(%rax)
...
 
 0028 :
...
-:  a2 03 00 00 00 00 00movabs %al,0x3
+:  a1 03 00 00 00 00 00movabs 0x3,%eax
 :  00 00 
 
 0050 :
@@ -2468,7 +2468,7 @@
 
 0078 :
...
-:  56  push   %rsi
+:  57  push   %rdi
 :  00 00   add%al,(%rax)
 :  00 00   add%al,(%rax)
 :  00 00   add%al,(%rax)
Object differences exist! - Verify changes before commit!


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] reformat_with_checkpatch: Add automation to checkpatch

[Greg KH]

On Fri, Jul 11, 2014 at 06:57:24PM -0700, Joe Perches wrote:
> On Fri, 2014-07-11 at 18:53 -0700, Greg KH wrote:
> > On Fri, Jul 11, 2014 at 06:21:27PM -0700, Joe Perches wrote:
> > > A simple script to run checkpatch --fix for various types of
> > > of cleanups.
> []
> > Anyway, try running this script on
> > drivers/staging/lustre/lnet/lnet/acceptor.c to see how this build fails.
> 
> lustre doesn't use "normal" kernel makefiles.

Ok, I've fixed all of the lustre Makefile crud up and checked it into my
staging-next branch.  In doing so, it exposed just what a horrid mess
the lustre include files are, which is good, hopefully people will
notice and start fixing it up.

And with the fixes, this script now properly can build and test lustre
files.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] reformat_with_checkpatch: Add automation to checkpatch

[Greg KH]

On Fri, Jul 11, 2014 at 06:40:16PM -0700, Joe Perches wrote:
> On Fri, 2014-07-11 at 18:34 -0700, Greg KH wrote:
> > On Fri, Jul 11, 2014 at 06:21:27PM -0700, Joe Perches wrote:
> > > A simple script to run checkpatch --fix for various types of
> > > of cleanups.
> > > 
> > > This script is useful primarily for staging.
> > > 
> > > This reformats code to a more CodingStyle conforming style,
> > > compiles it, verifies that the object code hasn't changed,
> > > and git commits it too.
> > 
> > And 'git commits' it?
> 
> The thought I had was to made it easier to
> submit "my first kernel patch" correctly.
> like that tuxradar article you wrote a few
> years ago.
> 
> http://www.tuxradar.com/content/newbies-guide-hacking-linux-kernel

Yeah, but it's good for people to actually understand the change they
are making, and edit it themselves.  Or at least I think so, but I know
others don't, so I don't mind this script.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] reformat_with_checkpatch: Add automation to checkpatch

[Greg KH]

On Fri, Jul 11, 2014 at 06:57:24PM -0700, Joe Perches wrote:
> On Fri, 2014-07-11 at 18:53 -0700, Greg KH wrote:
> > On Fri, Jul 11, 2014 at 06:21:27PM -0700, Joe Perches wrote:
> > > A simple script to run checkpatch --fix for various types of
> > > of cleanups.
> []
> > Anyway, try running this script on
> > drivers/staging/lustre/lnet/lnet/acceptor.c to see how this build fails.
> 
> lustre doesn't use "normal" kernel makefiles.

Well, it does, it just has a:
subdir-ccflags-y := -I$(src)/include/
line in drivers/staging/lustre/Makefile that messes up making .o files
in subdirectories.

I'll go fix that include file mess up now to be able to remove that line
so the the .o file building will work.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] reformat_with_checkpatch: Add automation to checkpatch

[Greg KH]

On Fri, Jul 11, 2014 at 07:01:14PM -0700, Greg KH wrote:
> On Fri, Jul 11, 2014 at 06:46:52PM -0700, Joe Perches wrote:
> > On Fri, 2014-07-11 at 18:39 -0700, Greg KH wrote:
> > > On Fri, Jul 11, 2014 at 06:21:27PM -0700, Joe Perches wrote:
> > > > A simple script to run checkpatch --fix for various types of
> > > > of cleanups.
> > ]
> > > drivers/staging/lustre/include/linux/lnet/api.h has no obvious style 
> > > problems and is ready for submission.
> > > file:  description: 
> > >  types:
> > > Useless use of greediness modifier '+' in regex; marked by <-- HERE in 
> > > m/(^\+.*) {8,8}+ <-- HERE \t/ at ./scripts/checkpatch.pl line 2217.
> > []
> > > drivers/staging/lustre/include/linux/lnet/api.h has no obvious style 
> > > problems and is ready for submission.
> > []
> > > Is that expected?
> > 
> > No, I haven't seen that.
> > 
> > Can you tell me what git tree you're working on?
> 
> My staging-next branch of staging.git on git.kernel.org
> 
> > Also, can you use the scripts/checkpatch from -next
> > tag next-20140711
> 
> that will take a bit to checkout, I'll do that afterward.
> 
> > My system has:
> > 
> > $ perl --version 
> > 
> > This is perl 5, version 18, subversion 2 (v5.18.2) built for 
> > i686-linux-gnu-thread-multi-64int
> > (with 41 registered patches, see perl -V for more detail)
> 
> I think this started showing up for me for perl 5.20.  Let me go
> checkout linux-next and see if that fixes anything or not...

Ok, with linux-next I get the same thing:

~/linux/tmp/linux-next $ ./scripts/checkpatch.pl -f --strict 
drivers/staging/lustre/include/linux/lnet/api.h --types=c99_comments
Useless use of greediness modifier '+' in regex; marked by <-- HERE in 
m/(^\+.*) {8,8}+ <-- HERE \t/ at ./scripts/checkpatch.pl line 2358.
total: 0 errors, 0 warnings, 0 checks, 220 lines checked

NOTE: Used message types: C99_COMMENTS

drivers/staging/lustre/include/linux/lnet/api.h has no obvious style problems 
and is ready for submission.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] reformat_with_checkpatch: Add automation to checkpatch

[Greg KH]

On Fri, Jul 11, 2014 at 06:46:52PM -0700, Joe Perches wrote:
> On Fri, 2014-07-11 at 18:39 -0700, Greg KH wrote:
> > On Fri, Jul 11, 2014 at 06:21:27PM -0700, Joe Perches wrote:
> > > A simple script to run checkpatch --fix for various types of
> > > of cleanups.
> ]
> > drivers/staging/lustre/include/linux/lnet/api.h has no obvious style 
> > problems and is ready for submission.
> > file:  description: 
> >  types:
> > Useless use of greediness modifier '+' in regex; marked by <-- HERE in 
> > m/(^\+.*) {8,8}+ <-- HERE \t/ at ./scripts/checkpatch.pl line 2217.
> []
> > drivers/staging/lustre/include/linux/lnet/api.h has no obvious style 
> > problems and is ready for submission.
> []
> > Is that expected?
> 
> No, I haven't seen that.
> 
> Can you tell me what git tree you're working on?

My staging-next branch of staging.git on git.kernel.org

> Also, can you use the scripts/checkpatch from -next
> tag next-20140711

that will take a bit to checkout, I'll do that afterward.

> My system has:
> 
> $ perl --version 
> 
> This is perl 5, version 18, subversion 2 (v5.18.2) built for 
> i686-linux-gnu-thread-multi-64int
> (with 41 registered patches, see perl -V for more detail)

I think this started showing up for me for perl 5.20.  Let me go
checkout linux-next and see if that fixes anything or not...

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] reformat_with_checkpatch: Add automation to checkpatch

[Joe Perches]

On Fri, 2014-07-11 at 18:53 -0700, Greg KH wrote:
> On Fri, Jul 11, 2014 at 06:21:27PM -0700, Joe Perches wrote:
> > A simple script to run checkpatch --fix for various types of
> > of cleanups.
[]
> Anyway, try running this script on
> drivers/staging/lustre/lnet/lnet/acceptor.c to see how this build fails.

lustre doesn't use "normal" kernel makefiles.

I supposed that the entire staging/lustre/
path could be built for every change, but for
most of staging, and nearly all of the rest
of the kernel tree, using:

make file.o

works.

I know there can be many enhancements to the
reformat script, but I think a minimally
acceptable version is a decent start.

So, thanks for trying it out.

And keep the complaints coming.

cheers, Joe

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] reformat_with_checkpatch: Add automation to checkpatch

[Joe Perches]

On Fri, 2014-07-11 at 18:43 -0700, Greg KH wrote:
> On Fri, Jul 11, 2014 at 06:21:27PM -0700, Joe Perches wrote:
> > A simple script to run checkpatch --fix for various types of
> > of cleanups.
[]
> If I pick drivers/staging/lustre/include/linux/lnet/types.h, then I get:
> 
>   staging: lnet: checkpatch cleanup: whitespace neatening
> 
> and no 'types.h' here, is that intentional?  If so, why?

Yes, it's how it's written.
It uses just the directory name, not any basename($file)

It can be changed if that's what's desired.

> And this is fun, I'm going to let this rip on the lustre code...

It doesn't autocommit, it does show the various changes
it makes and asks you to accept them.

I suppose that could be automated with something better
than "yes", but I didn't want it to be completely automatic.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] reformat_with_checkpatch: Add automation to checkpatch

[Greg KH]

On Fri, Jul 11, 2014 at 06:21:27PM -0700, Joe Perches wrote:
> A simple script to run checkpatch --fix for various types of
> of cleanups.
> 
> This script is useful primarily for staging.
> 
> This reformats code to a more CodingStyle conforming style,
> compiles it, verifies that the object code hasn't changed,
> and git commits it too.
> 
> You must have the necessary development tools, git, and a
> recent git tree.  Ideally use Greg KH's staging-next, which
> can be retrieved via these commands:
> 
> git clone git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
> git checkout staging-next
> 
> To use this script try a sequence of commands like:
> 
>   cd 
>   git checkout -b 
>   make allyesconfig
>   mkdir patches
>   ./scripts/reformat_with_checkpatch.sh drivers/staging//*.[ch]
>   git format-patch --cover-letter -o patches/ staging-next
>   git send-email patches/
> 
> Signed-off-by: Joe Perches 
> ---
>  scripts/reformat_with_checkpatch.sh | 141 
> 
>  1 file changed, 141 insertions(+)
>  create mode 100755 scripts/reformat_with_checkpatch.sh
> 
> diff --git a/scripts/reformat_with_checkpatch.sh 
> b/scripts/reformat_with_checkpatch.sh
> new file mode 100755
> index 000..5415a8e
> --- /dev/null
> +++ b/scripts/reformat_with_checkpatch.sh
> @@ -0,0 +1,141 @@
> +#!/bin/bash
> +# (c) 2014, Joe Perches 
> +#
> +# Automate checkpatch modifications and git commits to
> +# neaten code that doesn't conform to CodingStyle.
> +# Useful primarily for files in drivers/staging
> +#
> +# Licensed under the terms of the GNU GPL License version 2
> +
> +declare -ar whitespace_types=(   \
> +
> 'whitespace_neatening:spacing,space_before_tab,pointer_location,trailing_whitespace,bracket_space'
>\
> +'remove_spaces_before_tabs:space_before_tab' \
> +'fix_label_positions:indented_label' \
> +'align_arguments_to_parenthesis:parenthesis_alignment'   \
> +)
> +
> +declare -ar changecode_types=(   \
> +
> 'fix_brace_positions:open_brace,braces,else_after_brace,while_after_brace'
>\
> +'fix_blank_lines:line_spacing'   \
> +'use_standard_attributes:prefer_packed,prefer_aligned'   \
> +'remove_unnecessary_externs:avoid_externs'   \
> +'update_c90_comment_style:c99_comments'  \
> +)
> +
> +checkpatch_update ()
> +{
> +file=$1
> +
> +desc=$(echo $2 | cut -f1 -d: | sed 's/_/ /g')
> +types=$(echo $2 | cut -f2- -d:)
> +
> +echo "file: <$file> description: <$desc> types:<$types>"
> +
> +./scripts/checkpatch.pl --file --fix-inplace --strict --types="$types" 
> $file
> +
> +checkpatch_fixes=$file.diff
> +git diff --stat -p --exit-code $file > $checkpatch_fixes
> +if [ $? == 0 ] ; then
> + rm -f $checkpatch_fixes
> + return
> +fi
> +
> +basename=$(basename $file)
> +if [ "${basename##*.}" == "c" ] ; then
> +
> + git checkout $file
> + obj="$(echo $file | sed 's/\.c$/\.o/')"
> + if [ -e $obj ] ; then
> + rm -f $obj
> + fi
> +
> + echo "Compiling original version..."
> +
> + ${CROSS_COMPILE}make $obj

This fails for when a cflags option is set (like an include path).  Now,
I could argue that having an include path override in a kernel Makefile
is a bug, but well, it's staging...

Anyway, try running this script on
drivers/staging/lustre/lnet/lnet/acceptor.c to see how this build fails.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] reformat_with_checkpatch: Add automation to checkpatch

[Joe Perches]

On Fri, 2014-07-11 at 18:39 -0700, Greg KH wrote:
> On Fri, Jul 11, 2014 at 06:21:27PM -0700, Joe Perches wrote:
> > A simple script to run checkpatch --fix for various types of
> > of cleanups.
]
> drivers/staging/lustre/include/linux/lnet/api.h has no obvious style problems 
> and is ready for submission.
> file:  description:  c90 comment style> types:
> Useless use of greediness modifier '+' in regex; marked by <-- HERE in 
> m/(^\+.*) {8,8}+ <-- HERE \t/ at ./scripts/checkpatch.pl line 2217.
[]
> drivers/staging/lustre/include/linux/lnet/api.h has no obvious style problems 
> and is ready for submission.
[]
> Is that expected?

No, I haven't seen that.

Can you tell me what git tree you're working on?
Also, can you use the scripts/checkpatch from -next
tag next-20140711

My system has:

$ perl --version 

This is perl 5, version 18, subversion 2 (v5.18.2) built for 
i686-linux-gnu-thread-multi-64int
(with 41 registered patches, see perl -V for more detail)

Copyright 1987-2013, Larry Wall

Perl may be copied only under the terms of either the Artistic License or the
GNU General Public License, which may be found in the Perl 5 source kit.

Complete documentation for Perl, including FAQ lists, should be found on
this system using "man perl" or "perldoc perl".  If you have access to the
Internet, point your browser at http://www.perl.org/, the Perl Home Page.

and using that:

$ ./scripts/checkpatch.pl -f --strict 
drivers/staging/lustre/include/linux/lnet/api.h --types=c99_comments
total: 0 errors, 0 warnings, 0 checks, 220 lines checked

NOTE: Used message types: C99_COMMENTS

drivers/staging/lustre/include/linux/lnet/api.h has no obvious style problems 
and is ready for submission.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] reformat_with_checkpatch: Add automation to checkpatch

[Joe Perches]

On Fri, 2014-07-11 at 18:34 -0700, Greg KH wrote:
> On Fri, Jul 11, 2014 at 06:21:27PM -0700, Joe Perches wrote:
> > A simple script to run checkpatch --fix for various types of
> > of cleanups.
> > 
> > This script is useful primarily for staging.
> > 
> > This reformats code to a more CodingStyle conforming style,
> > compiles it, verifies that the object code hasn't changed,
> > and git commits it too.
> 
> And 'git commits' it?

The thought I had was to made it easier to
submit "my first kernel patch" correctly.
like that tuxradar article you wrote a few
years ago.

http://www.tuxradar.com/content/newbies-guide-hacking-linux-kernel

> Heh, I should just run this myself to clean up
> staging and beat everyone else to it...

At least for the whitespace noise, you could
but I hope it'll encourage a few more people
to try kernel patching instead.

> I know some people already have private versions of these things, might
> as well make it public for all to abuse :)

And I hope they can improve it too as it's
just a brainless little script.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] reformat_with_checkpatch: Add automation to checkpatch

[Greg KH]

On Fri, Jul 11, 2014 at 06:21:27PM -0700, Joe Perches wrote:
> A simple script to run checkpatch --fix for various types of
> of cleanups.
> 
> This script is useful primarily for staging.
> 
> This reformats code to a more CodingStyle conforming style,
> compiles it, verifies that the object code hasn't changed,
> and git commits it too.
> 
> You must have the necessary development tools, git, and a
> recent git tree.  Ideally use Greg KH's staging-next, which
> can be retrieved via these commands:
> 
> git clone git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
> git checkout staging-next
> 
> To use this script try a sequence of commands like:
> 
>   cd 
>   git checkout -b 
>   make allyesconfig
>   mkdir patches
>   ./scripts/reformat_with_checkpatch.sh drivers/staging//*.[ch]
>   git format-patch --cover-letter -o patches/ staging-next
>   git send-email patches/
> 
> Signed-off-by: Joe Perches 
> ---
>  scripts/reformat_with_checkpatch.sh | 141 
> 
>  1 file changed, 141 insertions(+)
>  create mode 100755 scripts/reformat_with_checkpatch.sh
> 
> diff --git a/scripts/reformat_with_checkpatch.sh 
> b/scripts/reformat_with_checkpatch.sh
> new file mode 100755
> index 000..5415a8e
> --- /dev/null
> +++ b/scripts/reformat_with_checkpatch.sh
> @@ -0,0 +1,141 @@
> +#!/bin/bash
> +# (c) 2014, Joe Perches 
> +#
> +# Automate checkpatch modifications and git commits to
> +# neaten code that doesn't conform to CodingStyle.
> +# Useful primarily for files in drivers/staging
> +#
> +# Licensed under the terms of the GNU GPL License version 2
> +
> +declare -ar whitespace_types=(   \
> +
> 'whitespace_neatening:spacing,space_before_tab,pointer_location,trailing_whitespace,bracket_space'
>\
> +'remove_spaces_before_tabs:space_before_tab' \
> +'fix_label_positions:indented_label' \
> +'align_arguments_to_parenthesis:parenthesis_alignment'   \
> +)
> +
> +declare -ar changecode_types=(   \
> +
> 'fix_brace_positions:open_brace,braces,else_after_brace,while_after_brace'
>\
> +'fix_blank_lines:line_spacing'   \
> +'use_standard_attributes:prefer_packed,prefer_aligned'   \
> +'remove_unnecessary_externs:avoid_externs'   \
> +'update_c90_comment_style:c99_comments'  \
> +)
> +
> +checkpatch_update ()
> +{
> +file=$1
> +
> +desc=$(echo $2 | cut -f1 -d: | sed 's/_/ /g')
> +types=$(echo $2 | cut -f2- -d:)
> +
> +echo "file: <$file> description: <$desc> types:<$types>"
> +
> +./scripts/checkpatch.pl --file --fix-inplace --strict --types="$types" 
> $file
> +
> +checkpatch_fixes=$file.diff
> +git diff --stat -p --exit-code $file > $checkpatch_fixes
> +if [ $? == 0 ] ; then
> + rm -f $checkpatch_fixes
> + return
> +fi
> +
> +basename=$(basename $file)
> +if [ "${basename##*.}" == "c" ] ; then
> +
> + git checkout $file
> + obj="$(echo $file | sed 's/\.c$/\.o/')"
> + if [ -e $obj ] ; then
> + rm -f $obj
> + fi
> +
> + echo "Compiling original version..."
> +
> + ${CROSS_COMPILE}make $obj
> +
> + ${CROSS_COMPILE}objdump -D $obj | \
> + sed "s/^[[:space:]]\+[0-9a-f]\+//" > $obj.old
> +
> + patch -p1 < $checkpatch_fixes
> +
> + echo "Compiling modified version..."
> +
> + ${CROSS_COMPILE}make $obj
> +
> + ${CROSS_COMPILE}objdump -D $obj | \
> + sed "s/^[[:space:]]\+[0-9a-f]\+//" > $obj.new
> +
> + echo "Comparing objects..."
> + diff -Nurd $obj.new $obj.old
> + if [ $? -ne 0 ] ; then
> + echo "Object differences exist! - Verify changes before commit!"
> + read -s -p "Press the 'enter' key to continue: "
> + else
> + echo "No object differences found"
> + fi
> + rm -f $obj.old
> + rm -f $obj.new
> +fi
> +
> +echo "running checkpatch on possible checkpatch fixes..."
> +
> +./scripts/checkpatch.pl --no-summary --no-signoff $checkpatch_fixes
> +rm -f $checkpatch_fixes
> +
> +echo "Verify checkpatch output and make any necessary changes"
> +echo "Edit '$file' if appropriate"
> +read -s -p "Press the 'enter' key to continue: "
> +
> +commit_log_file=$(mktemp git_commit.XX)
> +
> +if [ -e $commit_log_file ] ; then
> + rm -f $commit_log_file
> +fi
> +
> +if [[ $file =~ ^drivers/staging/ ]] ; then
> + echo -n "staging: " >> $commit_log_file
> +fi
> +echo "$(basename $(dirname $file)): checkpatch cleanup: $desc" >> 
> $commit_log_file

If I pick drivers/staging/lustre/include/linux/lnet/types.h, then I get:

staging: lnet: checkpatch cleanup: whitespace neatening

and no 'types.h' here, is that intentional?  If so, why?

And this is fun, I'm going to let this rip on the lustre code...

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of 

Re: [PATCH] reformat_with_checkpatch: Add automation to checkpatch

[Greg KH]

On Fri, Jul 11, 2014 at 06:21:27PM -0700, Joe Perches wrote:
> A simple script to run checkpatch --fix for various types of
> of cleanups.
> 
> This script is useful primarily for staging.
> 
> This reformats code to a more CodingStyle conforming style,
> compiles it, verifies that the object code hasn't changed,
> and git commits it too.
> 
> You must have the necessary development tools, git, and a
> recent git tree.  Ideally use Greg KH's staging-next, which
> can be retrieved via these commands:
> 
> git clone git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
> git checkout staging-next
> 
> To use this script try a sequence of commands like:
> 
>   cd 
>   git checkout -b 
>   make allyesconfig
>   mkdir patches
>   ./scripts/reformat_with_checkpatch.sh drivers/staging//*.[ch]
>   git format-patch --cover-letter -o patches/ staging-next
>   git send-email patches/
> 
> Signed-off-by: Joe Perches 
> ---
>  scripts/reformat_with_checkpatch.sh | 141 
> 
>  1 file changed, 141 insertions(+)
>  create mode 100755 scripts/reformat_with_checkpatch.sh


No --help option?

How do I run this thing?

I ran it on a file that had no problems and got a mess, all I think due
to something odd in checkpatch itself right now:

$ ./scripts/checkpatch.pl --file  
drivers/staging/lustre/include/linux/lnet/api.h 
Useless use of greediness modifier '+' in regex; marked by <-- HERE in 
m/(^\+.*) {8,8}+ <-- HERE \t/ at ./scripts/checkpatch.pl line 2217.
total: 0 errors, 0 warnings, 220 lines checked

What's with that 'Useless use..." error?  perl 5.20 in use, is that the issue?

Anyway, running this script on the file gave me this:

$ ./reformat_with_checkpatch  drivers/staging/lustre/include/linux/lnet/api.h 
file:  description: 
 
types:
Useless use of greediness modifier '+' in regex; marked by <-- HERE in 
m/(^\+.*) {8,8}+ <-- HERE \t/ at ./scripts/checkpatch.pl line 2217.
total: 0 errors, 0 warnings, 0 checks, 220 lines checked

NOTE: Used message types: BRACKET_SPACE POINTER_LOCATION SPACE_BEFORE_TAB 
SPACING TRAILING_WHITESPACE

drivers/staging/lustre/include/linux/lnet/api.h has no obvious style problems 
and is ready for submission.
file:  description:  types:
Useless use of greediness modifier '+' in regex; marked by <-- HERE in 
m/(^\+.*) {8,8}+ <-- HERE \t/ at ./scripts/checkpatch.pl line 2217.
total: 0 errors, 0 warnings, 0 checks, 220 lines checked

NOTE: Used message types: SPACE_BEFORE_TAB

drivers/staging/lustre/include/linux/lnet/api.h has no obvious style problems 
and is ready for submission.
file:  description:  types:
Useless use of greediness modifier '+' in regex; marked by <-- HERE in 
m/(^\+.*) {8,8}+ <-- HERE \t/ at ./scripts/checkpatch.pl line 2217.
total: 0 errors, 0 warnings, 0 checks, 220 lines checked

NOTE: Used message types: INDENTED_LABEL

drivers/staging/lustre/include/linux/lnet/api.h has no obvious style problems 
and is ready for submission.
file:  description:  types:
Useless use of greediness modifier '+' in regex; marked by <-- HERE in 
m/(^\+.*) {8,8}+ <-- HERE \t/ at ./scripts/checkpatch.pl line 2217.
total: 0 errors, 0 warnings, 0 checks, 220 lines checked

NOTE: Used message types: PARENTHESIS_ALIGNMENT

drivers/staging/lustre/include/linux/lnet/api.h has no obvious style problems 
and is ready for submission.
file:  description:  types:
Useless use of greediness modifier '+' in regex; marked by <-- HERE in 
m/(^\+.*) {8,8}+ <-- HERE \t/ at ./scripts/checkpatch.pl line 2217.
total: 0 errors, 0 warnings, 0 checks, 220 lines checked

NOTE: Used message types: BRACES ELSE_AFTER_BRACE OPEN_BRACE WHILE_AFTER_BRACE

drivers/staging/lustre/include/linux/lnet/api.h has no obvious style problems 
and is ready for submission.
file:  description:  types:
Useless use of greediness modifier '+' in regex; marked by <-- HERE in 
m/(^\+.*) {8,8}+ <-- HERE \t/ at ./scripts/checkpatch.pl line 2217.
total: 0 errors, 0 warnings, 0 checks, 220 lines checked

NOTE: Used message types: LINE_SPACING

drivers/staging/lustre/include/linux/lnet/api.h has no obvious style problems 
and is ready for submission.
file:  description:  types:
Useless use of greediness modifier '+' in regex; marked by <-- HERE in 
m/(^\+.*) {8,8}+ <-- HERE \t/ at ./scripts/checkpatch.pl line 2217.
total: 0 errors, 0 warnings, 0 checks, 220 lines checked

NOTE: Used message types: PREFER_ALIGNED PREFER_PACKED

drivers/staging/lustre/include/linux/lnet/api.h has no obvious style problems 
and is ready for submission.
file:  description:  types:
Useless use of greediness modifier '+' in regex; marked by <-- HERE in 
m/(^\+.*) {8,8}+ <-- HERE \t/ at ./scripts/checkpatch.pl line 2217.
total: 0 errors, 0 warnings, 0 checks, 220 lines checked

NOTE: Used message types: AVOID_EXTERNS

drivers/staging/lustre/include/linux/lnet/api.h has no obvious style problems 
and is ready for submission.
file:  description:  types:
Useless use of greediness modifier '+' in 

Re: [PATCH] reformat_with_checkpatch: Add automation to checkpatch

[Greg KH]

On Fri, Jul 11, 2014 at 06:21:27PM -0700, Joe Perches wrote:
> A simple script to run checkpatch --fix for various types of
> of cleanups.
> 
> This script is useful primarily for staging.
> 
> This reformats code to a more CodingStyle conforming style,
> compiles it, verifies that the object code hasn't changed,
> and git commits it too.

And 'git commits' it?  Heh, I should just run this myself to clean up
staging and beat everyone else to it...

I know some people already have private versions of these things, might
as well make it public for all to abuse :)

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/