[PATCH 2/2] Fixed Trivial Warnings in file: Deleted Spaces prior to tabs, and added lines. modified: kernel/auditfilter.c

[PATCH 2/2] Fixed Trivial Warnings in file: Deleted Spaces prior to tabs, and added lines. modified: kernel/auditfilter.c

[Scott Matheina]

Signed-off-by: Scott Matheina <scott@matheina.com>
---
 kernel/auditfilter.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 7714d93..774f9ad 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -39,13 +39,13 @@
  * Locking model:
  *
  * audit_filter_mutex:
- * 		Synchronizes writes and blocking reads of audit's filterlist
- * 		data.  Rcu is used to traverse the filterlist and access
- * 		contents of structs audit_entry, audit_watch and opaque
- * 		LSM rules during filtering.  If modified, these structures
- * 		must be copied and replace their counterparts in the filterlist.
- * 		An audit_parent struct is not accessed during filtering, so may
- * 		be written directly provided audit_filter_mutex is held.
+ *		Synchronizes writes and blocking reads of audit's filterlist
+ *		data.  Rcu is used to traverse the filterlist and access
+ *		contents of structs audit_entry, audit_watch and opaque
+ *		LSM rules during filtering.  If modified, these structures
+ *		must be copied and replace their counterparts in the filterlist.
+ *		An audit_parent struct is not accessed during filtering, so may
+ *		be written directly provided audit_filter_mutex is held.
  */
 
 /* Audit filter lists, defined in <linux/audit.h> */
@@ -109,6 +109,7 @@ void audit_free_rule_rcu(struct rcu_head *head)
 {
 	struct audit_entry *e = container_of(head, struct audit_entry, rcu);
 	audit_free_rule(e);
+
 }
 
 /* Initialize an audit filterlist entry. */
@@ -176,9 +177,11 @@ static __u32 *classes[AUDIT_SYSCALL_CLASSES];
 int __init audit_register_class(int class, unsigned *list)
 {
 	__u32 *p = kcalloc(AUDIT_BITMASK_SIZE, sizeof(__u32), GFP_KERNEL);
+
 	if (!p)
 		return -ENOMEM;
 	while (*list != ~0U) {
+
 		unsigned n = *list++;
 		if (n >= AUDIT_BITMASK_SIZE * 32 - AUDIT_SYSCALL_CLASSES) {
 			kfree(p);
-- 
1.9.1

Re: [PATCH 2/2] Fixed Trivial Warnings in file: Deleted Spaces prior to tabs, and added lines. modified: kernel/auditfilter.c

[Paul Moore]

On Saturday, October 10, 2015 08:57:55 PM Scott Matheina wrote:
> Signed-off-by: Scott Matheina <scott@matheina.com>
> ---
>  kernel/auditfilter.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)

Sorry for the delay in reviewing this, comments inline ...

> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 7714d93..774f9ad 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -39,13 +39,13 @@
>   * Locking model:
>   *
>   * audit_filter_mutex:
> - * 		Synchronizes writes and blocking reads of audit's filterlist
> - * 		data.  Rcu is used to traverse the filterlist and access
> - * 		contents of structs audit_entry, audit_watch and opaque
> - * 		LSM rules during filtering.  If modified, these structures
> - * 		must be copied and replace their counterparts in the filterlist.
> - * 		An audit_parent struct is not accessed during filtering, so may
> - * 		be written directly provided audit_filter_mutex is held.
> + *		Synchronizes writes and blocking reads of audit's filterlist
> + *		data.  Rcu is used to traverse the filterlist and access
> + *		contents of structs audit_entry, audit_watch and opaque
> + *		LSM rules during filtering.  If modified, these structures
> + *		must be copied and replace their counterparts in the filterlist.
> + *		An audit_parent struct is not accessed during filtering, so may
> + *		be written directly provided audit_filter_mutex is held.
>   */

Okay, that's fine.

>  /* Audit filter lists, defined in <linux/audit.h> */
> @@ -109,6 +109,7 @@ void audit_free_rule_rcu(struct rcu_head *head)
>  {
>  	struct audit_entry *e = container_of(head, struct audit_entry, rcu);
>  	audit_free_rule(e);
> +
>  }

Why?

>  /* Initialize an audit filterlist entry. */
> @@ -176,9 +177,11 @@ static __u32 *classes[AUDIT_SYSCALL_CLASSES];
>  int __init audit_register_class(int class, unsigned *list)
>  {
>  	__u32 *p = kcalloc(AUDIT_BITMASK_SIZE, sizeof(__u32), GFP_KERNEL);
> +
>  	if (!p)
>  		return -ENOMEM;

Okay.

>  	while (*list != ~0U) {
> +
>  		unsigned n = *list++;
>  		if (n >= AUDIT_BITMASK_SIZE * 32 - AUDIT_SYSCALL_CLASSES) {
>  			kfree(p);

Why?

-- 
paul moore
www.paul-moore.com

Re: [PATCH 2/2] Fixed Trivial Warnings in file: Deleted Spaces prior to tabs, and added lines. modified: kernel/auditfilter.c

[Scott Matheina]

On 10/14/2015 04:54 PM, Paul Moore wrote:
> On Saturday, October 10, 2015 08:57:55 PM Scott Matheina wrote:
>> Signed-off-by: Scott Matheina <scott@matheina.com>
>> ---
>>  kernel/auditfilter.c | 17 ++++++++++-------
>>  1 file changed, 10 insertions(+), 7 deletions(-)
> Sorry for the delay in reviewing this, comments inline ...
>
>> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
>> index 7714d93..774f9ad 100644
>> --- a/kernel/auditfilter.c
>> +++ b/kernel/auditfilter.c
>> @@ -39,13 +39,13 @@
>>   * Locking model:
>>   *
>>   * audit_filter_mutex:
>> - * 		Synchronizes writes and blocking reads of audit's filterlist
>> - * 		data.  Rcu is used to traverse the filterlist and access
>> - * 		contents of structs audit_entry, audit_watch and opaque
>> - * 		LSM rules during filtering.  If modified, these structures
>> - * 		must be copied and replace their counterparts in the filterlist.
>> - * 		An audit_parent struct is not accessed during filtering, so may
>> - * 		be written directly provided audit_filter_mutex is held.
>> + *		Synchronizes writes and blocking reads of audit's filterlist
>> + *		data.  Rcu is used to traverse the filterlist and access
>> + *		contents of structs audit_entry, audit_watch and opaque
>> + *		LSM rules during filtering.  If modified, these structures
>> + *		must be copied and replace their counterparts in the filterlist.
>> + *		An audit_parent struct is not accessed during filtering, so may
>> + *		be written directly provided audit_filter_mutex is held.
>>   */
> Okay, that's fine.
>
>>  /* Audit filter lists, defined in <linux/audit.h> */
>> @@ -109,6 +109,7 @@ void audit_free_rule_rcu(struct rcu_head *head)
>>  {
>>  	struct audit_entry *e = container_of(head, struct audit_entry, rcu);
>>  	audit_free_rule(e);
>> +
>>  }
> Why?

I was following the error messages in checkpatch.pl, but the warning went away after adding this line. No problem
with the code. 

>>  /* Initialize an audit filterlist entry. */
>> @@ -176,9 +177,11 @@ static __u32 *classes[AUDIT_SYSCALL_CLASSES];
>>  int __init audit_register_class(int class, unsigned *list)
>>  {
>>  	__u32 *p = kcalloc(AUDIT_BITMASK_SIZE, sizeof(__u32), GFP_KERNEL);
>> +
>>  	if (!p)
>>  		return -ENOMEM;
> Okay.
>
>>  	while (*list != ~0U) {
>> +
>>  		unsigned n = *list++;
>>  		if (n >= AUDIT_BITMASK_SIZE * 32 - AUDIT_SYSCALL_CLASSES) {
>>  			kfree(p);
> Why?

This is the same as above. Just going through the checkpatch.pl script, and looking for warnings to fix. 

As you might have guessed, this is one of my first patches. I wasn't sure if a patch like this would even get
reviewed, and responded to. I'm subscribed to the linux-kernel mail group, and seeing what is acceptable. 

Thanks for the review. I don't plan on making a habit of submitting such incredibly trivial patches, but you 
have to start somewhere, and I thought it'd be hard to screw up by fixing a couple of trivial style errors.  

Re: [PATCH 2/2] Fixed Trivial Warnings in file: Deleted Spaces prior to tabs, and added lines. modified: kernel/auditfilter.c

[Richard Guy Briggs]

On 15/10/18, Scott Matheina wrote:
> On 10/14/2015 04:54 PM, Paul Moore wrote:
> > On Saturday, October 10, 2015 08:57:55 PM Scott Matheina wrote:
> >> Signed-off-by: Scott Matheina <scott@matheina.com>
> >> ---
> >>  kernel/auditfilter.c | 17 ++++++++++-------
> >>  1 file changed, 10 insertions(+), 7 deletions(-)
> > Sorry for the delay in reviewing this, comments inline ...
> >
> >> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> >> index 7714d93..774f9ad 100644
> >> --- a/kernel/auditfilter.c
> >> +++ b/kernel/auditfilter.c
> >> @@ -39,13 +39,13 @@
> >>   * Locking model:
> >>   *
> >>   * audit_filter_mutex:
> >> - * 		Synchronizes writes and blocking reads of audit's filterlist
> >> - * 		data.  Rcu is used to traverse the filterlist and access
> >> - * 		contents of structs audit_entry, audit_watch and opaque
> >> - * 		LSM rules during filtering.  If modified, these structures
> >> - * 		must be copied and replace their counterparts in the filterlist.
> >> - * 		An audit_parent struct is not accessed during filtering, so may
> >> - * 		be written directly provided audit_filter_mutex is held.
> >> + *		Synchronizes writes and blocking reads of audit's filterlist
> >> + *		data.  Rcu is used to traverse the filterlist and access
> >> + *		contents of structs audit_entry, audit_watch and opaque
> >> + *		LSM rules during filtering.  If modified, these structures
> >> + *		must be copied and replace their counterparts in the filterlist.
> >> + *		An audit_parent struct is not accessed during filtering, so may
> >> + *		be written directly provided audit_filter_mutex is held.
> >>   */
> > Okay, that's fine.
> >
> >>  /* Audit filter lists, defined in <linux/audit.h> */
> >> @@ -109,6 +109,7 @@ void audit_free_rule_rcu(struct rcu_head *head)
> >>  {
> >>  	struct audit_entry *e = container_of(head, struct audit_entry, rcu);
> >>  	audit_free_rule(e);
> >> +
> >>  }
> > Why?
> 
> I was following the error messages in checkpatch.pl, but the warning
> went away after adding this line. No problem with the code. 

That sounds like a bug in checkpatch.pl, since that blank line should be
tween the declaration and the function call.

> >>  /* Initialize an audit filterlist entry. */
> >> @@ -176,9 +177,11 @@ static __u32 *classes[AUDIT_SYSCALL_CLASSES];
> >>  int __init audit_register_class(int class, unsigned *list)
> >>  {
> >>  	__u32 *p = kcalloc(AUDIT_BITMASK_SIZE, sizeof(__u32), GFP_KERNEL);
> >> +
> >>  	if (!p)
> >>  		return -ENOMEM;
> > Okay.
> >
> >>  	while (*list != ~0U) {
> >> +
> >>  		unsigned n = *list++;
> >>  		if (n >= AUDIT_BITMASK_SIZE * 32 - AUDIT_SYSCALL_CLASSES) {
> >>  			kfree(p);
> > Why?
> 
> This is the same as above. Just going through the checkpatch.pl
> script, and looking for warnings to fix. 

Again, another manifestation of that bug?  That blank line should be
after the declaration and before the if statement.

> As you might have guessed, this is one of my first patches. I wasn't
> sure if a patch like this would even get reviewed, and responded to.
> I'm subscribed to the linux-kernel mail group, and seeing what is
> acceptable. 
> 
> Thanks for the review. I don't plan on making a habit of submitting
> such incredibly trivial patches, but you have to start somewhere, and
> I thought it'd be hard to screw up by fixing a couple of trivial style
> errors.  

Well, I agree, you have to start somewhere...  Too bad you hit a bug in
checkpatch.pl!

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

Re: [PATCH 2/2] Fixed Trivial Warnings in file: Deleted Spaces prior to tabs, and added lines. modified: kernel/auditfilter.c

[Joe Perches]

On Mon, 2015-10-19 at 12:10 -0400, Richard Guy Briggs wrote:
> On 15/10/18, Scott Matheina wrote:
> > On 10/14/2015 04:54 PM, Paul Moore wrote:
> > > On Saturday, October 10, 2015 08:57:55 PM Scott Matheina wrote:
[]
> > >> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
[]
> > >> @@ -109,6 +109,7 @@ void audit_free_rule_rcu(struct rcu_head *head)
> > >>  {
> > >>  	struct audit_entry *e = container_of(head, struct audit_entry, rcu);
> > >>  	audit_free_rule(e);
> > >> +
> > >>  }
> > > Why?
> > 
> > I was following the error messages in checkpatch.pl, but the warning
> > went away after adding this line. No problem with the code. 
> 
> That sounds like a bug in checkpatch.pl, since that blank line should be
> tween the declaration and the function call.

checkpatch message asks for a blank line after the
"struct audit_entry *e = ..." declaration.

> > >>  	while (*list != ~0U) {
> > >> +
> > >>  		unsigned n = *list++;
> > >>  		if (n >= AUDIT_BITMASK_SIZE * 32 - AUDIT_SYSCALL_CLASSES) {
> > >>  			kfree(p);
> > > Why?
> > 
> > This is the same as above. Just going through the checkpatch.pl
> > script, and looking for warnings to fix. 
> 
> Again, another manifestation of that bug?  That blank line should be
> after the declaration and before the if statement.
[]
> Well, I agree, you have to start somewhere...  Too bad you hit a bug in
> checkpatch.pl!

Here too, not a bug in checkpatch.

checkpatch output asks for a blank line after the
"unsigned n" declaration, not before.

Re: [PATCH 2/2] Fixed Trivial Warnings in file: Deleted Spaces prior to tabs, and added lines. modified: kernel/auditfilter.c

[Richard Guy Briggs]

On 15/10/21, Joe Perches wrote:
> On Mon, 2015-10-19 at 12:10 -0400, Richard Guy Briggs wrote:
> > On 15/10/18, Scott Matheina wrote:
> > > On 10/14/2015 04:54 PM, Paul Moore wrote:
> > > > On Saturday, October 10, 2015 08:57:55 PM Scott Matheina wrote:
> []
> > > >> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> []
> > > >> @@ -109,6 +109,7 @@ void audit_free_rule_rcu(struct rcu_head *head)
> > > >>  {
> > > >>  	struct audit_entry *e = container_of(head, struct audit_entry, rcu);
> > > >>  	audit_free_rule(e);
> > > >> +
> > > >>  }
> > > > Why?
> > > 
> > > I was following the error messages in checkpatch.pl, but the warning
> > > went away after adding this line. No problem with the code. 
> > 
> > That sounds like a bug in checkpatch.pl, since that blank line should be
> > tween the declaration and the function call.
> 
> checkpatch message asks for a blank line after the
> "struct audit_entry *e = ..." declaration.

Well then maybe it is a bug in his interpretation of the output of
checkpatch.pl?  Scott, did you re-run checkpatch.pl after adding those
spaces?  Did it pass?

> > > >>  	while (*list != ~0U) {
> > > >> +
> > > >>  		unsigned n = *list++;
> > > >>  		if (n >= AUDIT_BITMASK_SIZE * 32 - AUDIT_SYSCALL_CLASSES) {
> > > >>  			kfree(p);
> > > > Why?
> > > 
> > > This is the same as above. Just going through the checkpatch.pl
> > > script, and looking for warnings to fix. 
> > 
> > Again, another manifestation of that bug?  That blank line should be
> > after the declaration and before the if statement.
> []
> > Well, I agree, you have to start somewhere...  Too bad you hit a bug in
> > checkpatch.pl!
> 
> Here too, not a bug in checkpatch.
> 
> checkpatch output asks for a blank line after the
> "unsigned n" declaration, not before.

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

Re: [PATCH 2/2] Fixed Trivial Warnings in file: Deleted Spaces prior to tabs, and added lines. modified: kernel/auditfilter.c

[Scott Matheina]

On 10/21/2015 10:33 AM, Richard Guy Briggs wrote:
> On 15/10/21, Joe Perches wrote:
>> On Mon, 2015-10-19 at 12:10 -0400, Richard Guy Briggs wrote:
>>> On 15/10/18, Scott Matheina wrote:
>>>> On 10/14/2015 04:54 PM, Paul Moore wrote:
>>>>> On Saturday, October 10, 2015 08:57:55 PM Scott Matheina wrote:
>> []
>>>>>> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
>> []
>>>>>> @@ -109,6 +109,7 @@ void audit_free_rule_rcu(struct rcu_head *head)
>>>>>>  {
>>>>>>  	struct audit_entry *e = container_of(head, struct audit_entry, rcu);
>>>>>>  	audit_free_rule(e);
>>>>>> +
>>>>>>  }
>>>>> Why?
>>>> I was following the error messages in checkpatch.pl, but the warning
>>>> went away after adding this line. No problem with the code. 
>>> That sounds like a bug in checkpatch.pl, since that blank line should be
>>> tween the declaration and the function call.
>> checkpatch message asks for a blank line after the
>> "struct audit_entry *e = ..." declaration.
> Well then maybe it is a bug in his interpretation of the output of
> checkpatch.pl?  Scott, did you re-run checkpatch.pl after adding those
> spaces?  Did it pass?

The error did go away. 

>>>>>>  	while (*list != ~0U) {
>>>>>> +
>>>>>>  		unsigned n = *list++;
>>>>>>  		if (n >= AUDIT_BITMASK_SIZE * 32 - AUDIT_SYSCALL_CLASSES) {
>>>>>>  			kfree(p);
>>>>> Why?
>>>> This is the same as above. Just going through the checkpatch.pl
>>>> script, and looking for warnings to fix. 
>>> Again, another manifestation of that bug?  That blank line should be
>>> after the declaration and before the if statement.
>> []
>>> Well, I agree, you have to start somewhere...  Too bad you hit a bug in
>>> checkpatch.pl!
>> Here too, not a bug in checkpatch.
>>
>> checkpatch output asks for a blank line after the
>> "unsigned n" declaration, not before.
> - RGB
>
> --
> Richard Guy Briggs <rbriggs@redhat.com>
> Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
> Remote, Ottawa, Canada
> Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

Re: [PATCH 2/2] Fixed Trivial Warnings in file: Deleted Spaces prior to tabs, and added lines. modified: kernel/auditfilter.c

[Richard Guy Briggs]

On 15/10/21, Scott Matheina wrote:
> On 10/21/2015 10:33 AM, Richard Guy Briggs wrote:
> > On 15/10/21, Joe Perches wrote:
> >> On Mon, 2015-10-19 at 12:10 -0400, Richard Guy Briggs wrote:
> >>> On 15/10/18, Scott Matheina wrote:
> >>>> On 10/14/2015 04:54 PM, Paul Moore wrote:
> >>>>> On Saturday, October 10, 2015 08:57:55 PM Scott Matheina wrote:
> >> []
> >>>>>> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> >> []
> >>>>>> @@ -109,6 +109,7 @@ void audit_free_rule_rcu(struct rcu_head *head)
> >>>>>>  {
> >>>>>>  	struct audit_entry *e = container_of(head, struct audit_entry, rcu);
> >>>>>>  	audit_free_rule(e);
> >>>>>> +
> >>>>>>  }
> >>>>> Why?
> >>>> I was following the error messages in checkpatch.pl, but the warning
> >>>> went away after adding this line. No problem with the code. 
> >>> That sounds like a bug in checkpatch.pl, since that blank line should be
> >>> tween the declaration and the function call.
> >> checkpatch message asks for a blank line after the
> >> "struct audit_entry *e = ..." declaration.
> > Well then maybe it is a bug in his interpretation of the output of
> > checkpatch.pl?  Scott, did you re-run checkpatch.pl after adding those
> > spaces?  Did it pass?
> 
> The error did go away. 

Joe, I confirm the error went away.  Looks like a bug in checkpatch.pl
to me.  I tried a number of combinations of things and it didn't
complain about several things it should have.  I did try a few other
things to make sure it was still finding problems like brace placement
and leading spaces, but it looks like the blank line checking code isn't
working.  This is on 4.0, so maybe it has been fixed since then.  Scott,
what kernel version are you using?

> >>>>>>  	while (*list != ~0U) {
> >>>>>> +
> >>>>>>  		unsigned n = *list++;
> >>>>>>  		if (n >= AUDIT_BITMASK_SIZE * 32 - AUDIT_SYSCALL_CLASSES) {
> >>>>>>  			kfree(p);
> >>>>> Why?
> >>>> This is the same as above. Just going through the checkpatch.pl
> >>>> script, and looking for warnings to fix. 
> >>> Again, another manifestation of that bug?  That blank line should be
> >>> after the declaration and before the if statement.
> >> []
> >>> Well, I agree, you have to start somewhere...  Too bad you hit a bug in
> >>> checkpatch.pl!
> >> Here too, not a bug in checkpatch.
> >>
> >> checkpatch output asks for a blank line after the
> >> "unsigned n" declaration, not before.
> > - RGB

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

Re: [PATCH 2/2] Fixed Trivial Warnings in file: Deleted Spaces prior to tabs, and added lines. modified: kernel/auditfilter.c

[Scott Matheina]

On 10/21/2015 09:15 PM, Richard Guy Briggs wrote:
> On 15/10/21, Scott Matheina wrote:
>> On 10/21/2015 10:33 AM, Richard Guy Briggs wrote:
>>> On 15/10/21, Joe Perches wrote:
>>>> On Mon, 2015-10-19 at 12:10 -0400, Richard Guy Briggs wrote:
>>>>> On 15/10/18, Scott Matheina wrote:
>>>>>> On 10/14/2015 04:54 PM, Paul Moore wrote:
>>>>>>> On Saturday, October 10, 2015 08:57:55 PM Scott Matheina wrote:
>>>> []
>>>>>>>> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
>>>> []
>>>>>>>> @@ -109,6 +109,7 @@ void audit_free_rule_rcu(struct rcu_head *head)
>>>>>>>>  {
>>>>>>>>  	struct audit_entry *e = container_of(head, struct audit_entry, rcu);
>>>>>>>>  	audit_free_rule(e);
>>>>>>>> +
>>>>>>>>  }
>>>>>>> Why?
>>>>>> I was following the error messages in checkpatch.pl, but the warning
>>>>>> went away after adding this line. No problem with the code. 
>>>>> That sounds like a bug in checkpatch.pl, since that blank line should be
>>>>> tween the declaration and the function call.
>>>> checkpatch message asks for a blank line after the
>>>> "struct audit_entry *e = ..." declaration.
>>> Well then maybe it is a bug in his interpretation of the output of
>>> checkpatch.pl?  Scott, did you re-run checkpatch.pl after adding those
>>> spaces?  Did it pass?
>> The error did go away. 
> Joe, I confirm the error went away.  Looks like a bug in checkpatch.pl
> to me.  I tried a number of combinations of things and it didn't
> complain about several things it should have.  I did try a few other
> things to make sure it was still finding problems like brace placement
> and leading spaces, but it looks like the blank line checking code isn't
> working.  This is on 4.0, so maybe it has been fixed since then.  Scott,
> what kernel version are you using?
I'm running Ubuntu 14.04 LTS (Kernel 3.19.0-30-generic) on my machine.

I cloned Linus' repo for source code. I'm pretty sure you were talking about the active Kernel on my machine, so if not please let me know.
>
>>>>>>>>  	while (*list != ~0U) {
>>>>>>>> +
>>>>>>>>  		unsigned n = *list++;
>>>>>>>>  		if (n >= AUDIT_BITMASK_SIZE * 32 - AUDIT_SYSCALL_CLASSES) {
>>>>>>>>  			kfree(p);
>>>>>>> Why?
>>>>>> This is the same as above. Just going through the checkpatch.pl
>>>>>> script, and looking for warnings to fix. 
>>>>> Again, another manifestation of that bug?  That blank line should be
>>>>> after the declaration and before the if statement.
>>>> []
>>>>> Well, I agree, you have to start somewhere...  Too bad you hit a bug in
>>>>> checkpatch.pl!
>>>> Here too, not a bug in checkpatch.
>>>>
>>>> checkpatch output asks for a blank line after the
>>>> "unsigned n" declaration, not before.
>>> - RGB
> - RGB
>
> --
> Richard Guy Briggs <rbriggs@redhat.com>
> Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
> Remote, Ottawa, Canada
> Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

Re: [PATCH 2/2] Fixed Trivial Warnings in file: Deleted Spaces prior to tabs, and added lines. modified: kernel/auditfilter.c

[Richard Guy Briggs]

On 15/10/21, Scott Matheina wrote:
> On 10/21/2015 09:15 PM, Richard Guy Briggs wrote:
> > On 15/10/21, Scott Matheina wrote:
> >> On 10/21/2015 10:33 AM, Richard Guy Briggs wrote:
> >>> On 15/10/21, Joe Perches wrote:
> >>>> On Mon, 2015-10-19 at 12:10 -0400, Richard Guy Briggs wrote:
> >>>>> On 15/10/18, Scott Matheina wrote:
> >>>>>> On 10/14/2015 04:54 PM, Paul Moore wrote:
> >>>>>>> On Saturday, October 10, 2015 08:57:55 PM Scott Matheina wrote:
> >>>> []
> >>>>>>>> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> >>>> []
> >>>>>>>> @@ -109,6 +109,7 @@ void audit_free_rule_rcu(struct rcu_head *head)
> >>>>>>>>  {
> >>>>>>>>  	struct audit_entry *e = container_of(head, struct audit_entry, rcu);
> >>>>>>>>  	audit_free_rule(e);
> >>>>>>>> +
> >>>>>>>>  }
> >>>>>>> Why?
> >>>>>> I was following the error messages in checkpatch.pl, but the warning
> >>>>>> went away after adding this line. No problem with the code. 
> >>>>> That sounds like a bug in checkpatch.pl, since that blank line should be
> >>>>> tween the declaration and the function call.
> >>>> checkpatch message asks for a blank line after the
> >>>> "struct audit_entry *e = ..." declaration.
> >>> Well then maybe it is a bug in his interpretation of the output of
> >>> checkpatch.pl?  Scott, did you re-run checkpatch.pl after adding those
> >>> spaces?  Did it pass?
> >> The error did go away. 
> > Joe, I confirm the error went away.  Looks like a bug in checkpatch.pl
> > to me.  I tried a number of combinations of things and it didn't
> > complain about several things it should have.  I did try a few other
> > things to make sure it was still finding problems like brace placement
> > and leading spaces, but it looks like the blank line checking code isn't
> > working.  This is on 4.0, so maybe it has been fixed since then.  Scott,
> > what kernel version are you using?
> I'm running Ubuntu 14.04 LTS (Kernel 3.19.0-30-generic) on my machine.
> 
> I cloned Linus' repo for source code. I'm pretty sure you were talking
> about the active Kernel on my machine, so if not please let me know.

I was talking about the source used to generate this patch in question,
run ./scripts/checkpatch.pl and do a compile test.  The active kernel on
your machine is irrelevant unless you subsequently booted it to test it.
How recent is your clone/pull of Linus' repo?

> >>>>>>>>  	while (*list != ~0U) {
> >>>>>>>> +
> >>>>>>>>  		unsigned n = *list++;
> >>>>>>>>  		if (n >= AUDIT_BITMASK_SIZE * 32 - AUDIT_SYSCALL_CLASSES) {
> >>>>>>>>  			kfree(p);
> >>>>>>> Why?
> >>>>>> This is the same as above. Just going through the checkpatch.pl
> >>>>>> script, and looking for warnings to fix. 
> >>>>> Again, another manifestation of that bug?  That blank line should be
> >>>>> after the declaration and before the if statement.
> >>>> []
> >>>>> Well, I agree, you have to start somewhere...  Too bad you hit a bug in
> >>>>> checkpatch.pl!
> >>>> Here too, not a bug in checkpatch.
> >>>>
> >>>> checkpatch output asks for a blank line after the
> >>>> "unsigned n" declaration, not before.
> >>> - RGB
> > - RGB

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

Re: [PATCH 2/2] Fixed Trivial Warnings in file: Deleted Spaces prior to tabs, and added lines. modified: kernel/auditfilter.c

[Scott Matheina]

On 10/21/2015 09:15 PM, Richard Guy Briggs wrote:
> On 15/10/21, Scott Matheina wrote:
>> On 10/21/2015 10:33 AM, Richard Guy Briggs wrote:
>>> On 15/10/21, Joe Perches wrote:
>>>> On Mon, 2015-10-19 at 12:10 -0400, Richard Guy Briggs wrote:
>>>>> On 15/10/18, Scott Matheina wrote:
>>>>>> On 10/14/2015 04:54 PM, Paul Moore wrote:
>>>>>>> On Saturday, October 10, 2015 08:57:55 PM Scott Matheina wrote:
>>>> []
>>>>>>>> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
>>>> []
>>>>>>>> @@ -109,6 +109,7 @@ void audit_free_rule_rcu(struct rcu_head *head)
>>>>>>>>   {
>>>>>>>>   	struct audit_entry *e = container_of(head, struct audit_entry, rcu);
>>>>>>>>   	audit_free_rule(e);
>>>>>>>> +
>>>>>>>>   }
>>>>>>> Why?
>>>>>> I was following the error messages in checkpatch.pl, but the warning
>>>>>> went away after adding this line. No problem with the code.
>>>>> That sounds like a bug in checkpatch.pl, since that blank line should be
>>>>> tween the declaration and the function call.
>>>> checkpatch message asks for a blank line after the
>>>> "struct audit_entry *e = ..." declaration.
>>> Well then maybe it is a bug in his interpretation of the output of
>>> checkpatch.pl?  Scott, did you re-run checkpatch.pl after adding those
>>> spaces?  Did it pass?
>> The error did go away.
> Joe, I confirm the error went away.  Looks like a bug in checkpatch.pl
> to me.  I tried a number of combinations of things and it didn't
> complain about several things it should have.  I did try a few other
> things to make sure it was still finding problems like brace placement
> and leading spaces, but it looks like the blank line checking code isn't
> working.  This is on 4.0, so maybe it has been fixed since then.  Scott,
> what kernel version are you using?
I had just cloned Linus' repo, so v4.3rc6.
>
>>>>>>>>   	while (*list != ~0U) {
>>>>>>>> +
>>>>>>>>   		unsigned n = *list++;
>>>>>>>>   		if (n >= AUDIT_BITMASK_SIZE * 32 - AUDIT_SYSCALL_CLASSES) {
>>>>>>>>   			kfree(p);
>>>>>>> Why?
>>>>>> This is the same as above. Just going through the checkpatch.pl
>>>>>> script, and looking for warnings to fix.
>>>>> Again, another manifestation of that bug?  That blank line should be
>>>>> after the declaration and before the if statement.
>>>> []
>>>>> Well, I agree, you have to start somewhere...  Too bad you hit a bug in
>>>>> checkpatch.pl!
>>>> Here too, not a bug in checkpatch.
>>>>
>>>> checkpatch output asks for a blank line after the
>>>> "unsigned n" declaration, not before.
>>> - RGB
> - RGB
>
> --
> Richard Guy Briggs <rbriggs@redhat.com>
> Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
> Remote, Ottawa, Canada
> Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

Re: [PATCH 2/2] Fixed Trivial Warnings in file: Deleted Spaces prior to tabs, and added lines. modified: kernel/auditfilter.c

[Joe Perches]

On Wed, 2015-10-21 at 22:15 -0400, Richard Guy Briggs wrote:
> On 15/10/21, Scott Matheina wrote:
> > On 10/21/2015 10:33 AM, Richard Guy Briggs wrote:
> > > On 15/10/21, Joe Perches wrote:
> > >> On Mon, 2015-10-19 at 12:10 -0400, Richard Guy Briggs wrote:
> > >>> On 15/10/18, Scott Matheina wrote:
> > >>>> On 10/14/2015 04:54 PM, Paul Moore wrote:
> > >>>>> On Saturday, October 10, 2015 08:57:55 PM Scott Matheina wrote:
> > >> []
> > >>>>>> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > >> []
> > >>>>>> @@ -109,6 +109,7 @@ void audit_free_rule_rcu(struct rcu_head *head)
> > >>>>>>  {
> > >>>>>>  	struct audit_entry *e = container_of(head, struct audit_entry, rcu);
> > >>>>>>  	audit_free_rule(e);
> > >>>>>> +
> > >>>>>>  }
> > >>>>> Why?
> > >>>> I was following the error messages in checkpatch.pl, but the warning
> > >>>> went away after adding this line. No problem with the code. 
> > >>> That sounds like a bug in checkpatch.pl, since that blank line should be
> > >>> tween the declaration and the function call.
> > >> checkpatch message asks for a blank line after the
> > >> "struct audit_entry *e = ..." declaration.
> > > Well then maybe it is a bug in his interpretation of the output of
> > > checkpatch.pl?  Scott, did you re-run checkpatch.pl after adding those
> > > spaces?  Did it pass?
> > 
> > The error did go away. 
> 
> Joe, I confirm the error went away.  Looks like a bug in checkpatch.pl
> to me.

It's not a bug in checkpatch.

checkpatch doesn't care if there are blank lines between declarations.

Here's the output of checkpatch for this area:

WARNING: Missing a blank line after declarations
#111: FILE: kernel/auditfilter.c:111:
+	struct audit_entry *e = container_of(head, struct audit_entry, rcu);
+	audit_free_rule(e);

That doesn't suggest putting a blank line before line 111.
It suggests putting a blank line after the declaration of e.

Re: [PATCH 2/2] Fixed Trivial Warnings in file: Deleted Spaces prior to tabs, and added lines. modified: kernel/auditfilter.c

[Paul Moore]

On Sunday, October 18, 2015 12:50:45 PM Scott Matheina wrote:
> On 10/14/2015 04:54 PM, Paul Moore wrote:
> > On Saturday, October 10, 2015 08:57:55 PM Scott Matheina wrote:
> >> Signed-off-by: Scott Matheina <scott@matheina.com>
> >> ---
> >> 
> >>  kernel/auditfilter.c | 17 ++++++++++-------
> >>  1 file changed, 10 insertions(+), 7 deletions(-)
> > 
> > Sorry for the delay in reviewing this, comments inline ...
> > 
> >> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> >> index 7714d93..774f9ad 100644
> >> --- a/kernel/auditfilter.c
> >> +++ b/kernel/auditfilter.c
> >> @@ -39,13 +39,13 @@
> >> 
> >>   * Locking model:
> >>   *
> >> 
> >>   * audit_filter_mutex:
> >> - * 		Synchronizes writes and blocking reads of audit's filterlist
> >> - * 		data.  Rcu is used to traverse the filterlist and access
> >> - * 		contents of structs audit_entry, audit_watch and opaque
> >> - * 		LSM rules during filtering.  If modified, these structures
> >> - * 		must be copied and replace their counterparts in the filterlist.
> >> - * 		An audit_parent struct is not accessed during filtering, so may
> >> - * 		be written directly provided audit_filter_mutex is held.
> >> + *		Synchronizes writes and blocking reads of audit's filterlist
> >> + *		data.  Rcu is used to traverse the filterlist and access
> >> + *		contents of structs audit_entry, audit_watch and opaque
> >> + *		LSM rules during filtering.  If modified, these structures
> >> + *		must be copied and replace their counterparts in the filterlist.
> >> + *		An audit_parent struct is not accessed during filtering, so may
> >> + *		be written directly provided audit_filter_mutex is held.
> >> 
> >>   */
> > 
> > Okay, that's fine.

...

> As you might have guessed, this is one of my first patches. I wasn't sure if
> a patch like this would even get reviewed, and responded to. I'm subscribed
> to the linux-kernel mail group, and seeing what is acceptable.
> 
> Thanks for the review. I don't plan on making a habit of submitting such
> incredibly trivial patches, but you have to start somewhere, and I thought
> it'd be hard to screw up by fixing a couple of trivial style errors.

We all start somewhere, and with that in mind, if you want to resubmit this 
patch with only the fix above (fixing the whitespace in the comment block), 
I'll apply it.  While the patch is trivial, it is does fix a minor nit with 
near-zero risk.

I would encourage you to try something a bit more substantial next time, as 
they say, bug fixes are the quickest way to a maintainer's heart.

-- 
paul moore
www.paul-moore.com

Re: [PATCH 2/2] Fixed Trivial Warnings in file: Deleted Spaces prior to tabs, and added lines. modified: kernel/auditfilter.c

[Scott Matheina]

On 10/21/2015 01:23 PM, Paul Moore wrote:
> On Sunday, October 18, 2015 12:50:45 PM Scott Matheina wrote:
>> On 10/14/2015 04:54 PM, Paul Moore wrote:
>>> On Saturday, October 10, 2015 08:57:55 PM Scott Matheina wrote:
>>>> Signed-off-by: Scott Matheina <scott@matheina.com>
>>>> ---
>>>>
>>>>  kernel/auditfilter.c | 17 ++++++++++-------
>>>>  1 file changed, 10 insertions(+), 7 deletions(-)
>>> Sorry for the delay in reviewing this, comments inline ...
>>>
>>>> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
>>>> index 7714d93..774f9ad 100644
>>>> --- a/kernel/auditfilter.c
>>>> +++ b/kernel/auditfilter.c
>>>> @@ -39,13 +39,13 @@
>>>>
>>>>   * Locking model:
>>>>   *
>>>>
>>>>   * audit_filter_mutex:
>>>> - * 		Synchronizes writes and blocking reads of audit's filterlist
>>>> - * 		data.  Rcu is used to traverse the filterlist and access
>>>> - * 		contents of structs audit_entry, audit_watch and opaque
>>>> - * 		LSM rules during filtering.  If modified, these structures
>>>> - * 		must be copied and replace their counterparts in the filterlist.
>>>> - * 		An audit_parent struct is not accessed during filtering, so may
>>>> - * 		be written directly provided audit_filter_mutex is held.
>>>> + *		Synchronizes writes and blocking reads of audit's filterlist
>>>> + *		data.  Rcu is used to traverse the filterlist and access
>>>> + *		contents of structs audit_entry, audit_watch and opaque
>>>> + *		LSM rules during filtering.  If modified, these structures
>>>> + *		must be copied and replace their counterparts in the filterlist.
>>>> + *		An audit_parent struct is not accessed during filtering, so may
>>>> + *		be written directly provided audit_filter_mutex is held.
>>>>
>>>>   */
>>> Okay, that's fine.
> ...
>
>> As you might have guessed, this is one of my first patches. I wasn't sure if
>> a patch like this would even get reviewed, and responded to. I'm subscribed
>> to the linux-kernel mail group, and seeing what is acceptable.
>>
>> Thanks for the review. I don't plan on making a habit of submitting such
>> incredibly trivial patches, but you have to start somewhere, and I thought
>> it'd be hard to screw up by fixing a couple of trivial style errors.
> We all start somewhere, and with that in mind, if you want to resubmit this 
> patch with only the fix above (fixing the whitespace in the comment block), 
> I'll apply it.  While the patch is trivial, it is does fix a minor nit with 
> near-zero risk.
>
> I would encourage you to try something a bit more substantial next time, as 
> they say, bug fixes are the quickest way to a maintainer's heart.
>
Thanks for the feedback. I'll resubmit. Now I get to figure out how to resubmit a patch with changes, so a good 
learning experience for me. Pure Hobbyist at this time, but I love to learn. 

Scott 

Re: [PATCH 2/2] Fixed Trivial Warnings in file: Deleted Spaces prior to tabs, and added lines. modified: kernel/auditfilter.c

[Paul Moore]

On Wed, Oct 21, 2015 at 8:35 PM, Scott Matheina <scott@matheina.com> wrote:
> Thanks for the feedback. I'll resubmit. Now I get to figure out how to resubmit a patch with changes, so a good
> learning experience for me. Pure Hobbyist at this time, but I love to learn.

If you haven't already, go read Documentation/SubmittingPatches in the
kernel source, it will cover a lot of the basics.  Also, as far as I'm
concerned, the #1 absolutely most important thing you must always do
when submitting patches is to make sure that I can simply save your
email and apply it with patch, or import it directly via
stgit/git/whatever; if I have to transform your patch in some way to
get it to apply, I get grumpy, and I don't like to get grumpy.

Beyond that, good luck and have fun :)

-- 
paul moore
www.paul-moore.com