Re: [x86] kernel/audit.c cleanup according to checkpatch.pl
[David Woodhouse - Thu, Jan 03, 2008 at 02:57:08PM +]
|
| On Thu, 2008-01-03 at 17:50 +0300, Cyrill Gorcunov wrote:
| >
| >
| > so what i would do now? i could post updated patch *without* that
| > splitted line, should I?
|
| And with the if (rc == 0) thing fixed too. Yes please.
|
| --
| dwmw2
|
here is an updated patch
---
From: Cyrill Gorcunov <[EMAIL PROTECTED]>
Subject: [x86] kernel/audit.c cleanup according to checkpatch.pl
This patch eliminates most of errors pointed by checkpatch.pl
over kernel/audit.c file.
Signed-off-by: Cyrill Gorcunov <[EMAIL PROTECTED]>
---
David, to use 'if (rc)' form instead of 'if (rc == 0)' i had
to move 'kfree(ctx)' out of if-else condition but that is OK
'case a lot of further audit.c code was doing the same ;)
kernel/audit.c | 126 +--
1 files changed, 66 insertions(+), 60 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index f93c271..2961ee4 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -89,7 +89,7 @@ static intaudit_rate_limit;
/* Number of outstanding audit_buffers allowed. */
static int audit_backlog_limit = 64;
static int audit_backlog_wait_time = 60 * HZ;
-static int audit_backlog_wait_overflow = 0;
+static int audit_backlog_wait_overflow;
/* The identity of the user shutting down the audit system. */
uid_t audit_sig_uid = -1;
@@ -136,7 +136,7 @@ static DEFINE_MUTEX(audit_cmd_mutex);
/* AUDIT_MAXFREE is the number of empty audit_buffers we keep on the
* audit_freelist. Doing so eliminates many kmalloc/kfree calls. */
-#define AUDIT_MAXFREE (2*NR_CPUS)
+#define AUDIT_MAXFREE (2 * NR_CPUS)
/* The audit_buffer is used when formatting an audit record. The caller
* locks briefly to get the record off the freelist or to allocate the
@@ -158,8 +158,7 @@ static void audit_set_pid(struct audit_buffer *ab, pid_t
pid)
void audit_panic(const char *message)
{
- switch (audit_failure)
- {
+ switch (audit_failure) {
case AUDIT_FAIL_SILENT:
break;
case AUDIT_FAIL_PRINTK:
@@ -173,15 +172,16 @@ void audit_panic(const char *message)
static inline int audit_rate_check(void)
{
- static unsigned longlast_check = 0;
- static int messages = 0;
+ static unsigned longlast_check;
+ static int messages;
static DEFINE_SPINLOCK(lock);
unsigned long flags;
unsigned long now;
unsigned long elapsed;
int retval = 0;
- if (!audit_rate_limit) return 1;
+ if (!audit_rate_limit)
+ return 1;
spin_lock_irqsave(, flags);
if (++messages < audit_rate_limit) {
@@ -210,7 +210,7 @@ static inline int audit_rate_check(void)
*/
void audit_log_lost(const char *message)
{
- static unsigned longlast_msg = 0;
+ static unsigned longlast_msg;
static DEFINE_SPINLOCK(lock);
unsigned long flags;
unsigned long now;
@@ -253,14 +253,15 @@ static int audit_set_rate_limit(int limit, uid_t
loginuid, u32 sid)
if (sid) {
char *ctx = NULL;
u32 len;
- if ((rc = selinux_sid_to_string(sid, , )) == 0) {
- audit_log(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE,
- "audit_rate_limit=%d old=%d by auid=%u"
- " subj=%s res=%d",
- limit, old, loginuid, ctx, res);
- kfree(ctx);
- } else
+ rc = selinux_sid_to_string(sid, , );
+ if (rc)
res = 0; /* Something weird, deny request */
+ else
+ audit_log(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE,
+ "audit_rate_limit=%d old=%d by auid=%u"
+ " subj=%s res=%d",
+ limit, old, loginuid, ctx, res);
+ kfree(ctx);
}
audit_log(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE,
"audit_rate_limit=%d old=%d by auid=%u res=%d",
@@ -288,14 +289,15 @@ static int audit_set_backlog_limit(int limit, uid_t
loginuid, u32 sid)
if (sid) {
char *ctx = NULL;
u32 len;
- if ((rc = selinux_sid_to_string(sid, , )) == 0) {
- audit_log(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE,
- "audit_backlog_limit=%d old=%d by auid=%u"
- " subj=%s res=%d",
- limit, old, loginuid, ctx, res);
- kfree(ctx);
- } else
+ rc = selinux_sid_to_string(sid, , );
+
Re: [x86] kernel/audit.c cleanup according to checkpatch.pl
On Thu, 2008-01-03 at 17:50 +0300, Cyrill Gorcunov wrote: > > > so what i would do now? i could post updated patch *without* that > splitted line, should I? And with the if (rc == 0) thing fixed too. Yes please. -- dwmw2 -- 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/
Re: [x86] kernel/audit.c cleanup according to checkpatch.pl
[David Woodhouse - Thu, Jan 03, 2008 at 02:37:24PM +] | | On Thu, 2008-01-03 at 15:37 +0100, Ingo Molnar wrote: | > * David Woodhouse <[EMAIL PROTECTED]> wrote: | > | > > On Thu, 2008-01-03 at 15:05 +0100, Ingo Molnar wrote: | > > > not to make a big issue out of this, but when was the last time you | > > > tried to grep this way: | > > > | > > > grep -E "audit_rate_limit=[0-9]+ audit_backlog" */*.c | > > | > > Not precisely that, but I've certainly had greps fail because people | > > have split up strings to meet the stupid 80-character "limit". | > | > yes - but if you read my whole reply you'll see that i qualified it: | > | > >> That's pretty much the only grep pattern that would break. People | > >> usually grep on the constant portion of the string, so breaking up a | > >> ^ | > >> line along a variable boundary is perfectly okay. | | Yes, you did. But you failed to provide any good reason for actually | changing it, either. Leave it as it was. | | -- | dwmw2 | so what i would do now? i could post updated patch *without* that splitted line, should I? - Cyrill - -- 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/
Re: [x86] kernel/audit.c cleanup according to checkpatch.pl
On Thu, 2008-01-03 at 15:37 +0100, Ingo Molnar wrote: > * David Woodhouse <[EMAIL PROTECTED]> wrote: > > > On Thu, 2008-01-03 at 15:05 +0100, Ingo Molnar wrote: > > > not to make a big issue out of this, but when was the last time you > > > tried to grep this way: > > > > > > grep -E "audit_rate_limit=[0-9]+ audit_backlog" */*.c > > > > Not precisely that, but I've certainly had greps fail because people > > have split up strings to meet the stupid 80-character "limit". > > yes - but if you read my whole reply you'll see that i qualified it: > > >> That's pretty much the only grep pattern that would break. People > >> usually grep on the constant portion of the string, so breaking up a > >> ^ > >> line along a variable boundary is perfectly okay. Yes, you did. But you failed to provide any good reason for actually changing it, either. Leave it as it was. -- dwmw2 -- 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/
Re: [x86] kernel/audit.c cleanup according to checkpatch.pl
* David Woodhouse <[EMAIL PROTECTED]> wrote: > On Thu, 2008-01-03 at 15:05 +0100, Ingo Molnar wrote: > > not to make a big issue out of this, but when was the last time you > > tried to grep this way: > > > > grep -E "audit_rate_limit=[0-9]+ audit_backlog" */*.c > > Not precisely that, but I've certainly had greps fail because people > have split up strings to meet the stupid 80-character "limit". yes - but if you read my whole reply you'll see that i qualified it: >> That's pretty much the only grep pattern that would break. People >> usually grep on the constant portion of the string, so breaking up a >> ^ >> line along a variable boundary is perfectly okay. >> Ingo -- 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/
Re: [x86] kernel/audit.c cleanup according to checkpatch.pl
On Thu, 2008-01-03 at 15:05 +0100, Ingo Molnar wrote: > not to make a big issue out of this, but when was the last time you > tried to grep this way: > > grep -E "audit_rate_limit=[0-9]+ audit_backlog" */*.c Not precisely that, but I've certainly had greps fail because people have split up strings to meet the stupid 80-character "limit". Leave it as it was. Also, please don't add 'if (rc == 0)'. Use 'if (rc)' instead. -- dwmw2 -- 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/
Re: [x86] kernel/audit.c cleanup according to checkpatch.pl
* Jörn Engel <[EMAIL PROTECTED]> wrote: > > - "audit: audit_lost=%d audit_rate_limit=%d > > audit_backlog_limit=%d\n", > > + "audit: audit_lost=%d audit_rate_limit=%d " > > + "audit_backlog_limit=%d\n", > >atomic_read(_lost), > >audit_rate_limit, > >audit_backlog_limit); > > This hunk is a bit questionable. It can easily deceive a reader to > assume two seperate lines printed out and sometimes defeats grepping > for printk output to find the code generating the message. not to make a big issue out of this, but when was the last time you tried to grep this way: grep -E "audit_rate_limit=[0-9]+ audit_backlog" */*.c ? That's pretty much the only grep pattern that would break. People usually grep on the constant portion of the string, so breaking up a line along a variable boundary is perfectly okay. Ingo -- 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/
Re: [x86] kernel/audit.c cleanup according to checkpatch.pl
[Tomas Carnecky - Thu, Jan 03, 2008 at 01:10:28PM +0100]
> Cyrill Gorcunov wrote:
>> [=?ISO-8859-1?Q?J=F6rn_Engel_ - Thu, Jan 03, 2008 at 12:29:57PM +0100]
>> | On Thu, 3 January 2008 14:19:25 +0300, Cyrill Gorcunov wrote:
>> | > @@ -232,7 +232,8 @@ void audit_log_lost(const char *message)
>> | > | > if (print) {
>> | > printk(KERN_WARNING
>> | > - "audit: audit_lost=%d audit_rate_limit=%d
>> audit_backlog_limit=%d\n",
>> | > + "audit: audit_lost=%d audit_rate_limit=%d "
>> | > + "audit_backlog_limit=%d\n",
>> | > atomic_read(_lost),
>> | > audit_rate_limit,
>> | > audit_backlog_limit);
>> | | This hunk is a bit questionable. It can easily deceive a reader to
>> | assume two seperate lines printed out and sometimes defeats grepping
>> | for printk output to find the code generating the message.
>> | | Rest looks good to me.
>> | | Jörn
>> | | -- | He that composes himself is wiser than he that composes a book.
>> | -- B. Franklin
>> | indeed.
>> here is updated one (with these part removed)
>
> Instead of removing that part completely, why not print this:
> "audit: lost=%d rate_limit=%d backlog_limit=%d\n"
>
> In that line there were too many 'audit's IMHO, and if someone wants to
> grep 'audit_lost=' he still can, 'audit:.*lost=' or something like that..
>
> tom
>
Well, it seems David is a mainteiner of this code,
so if he would not argue against this the we could.
- Cyrill -
--
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/
Re: [x86] kernel/audit.c cleanup according to checkpatch.pl
Cyrill Gorcunov wrote:
[=?ISO-8859-1?Q?J=F6rn_Engel_ - Thu, Jan 03, 2008 at 12:29:57PM +0100]
| On Thu, 3 January 2008 14:19:25 +0300, Cyrill Gorcunov wrote:
| > @@ -232,7 +232,8 @@ void audit_log_lost(const char *message)
| >
| > if (print) {
| > printk(KERN_WARNING
| > - "audit: audit_lost=%d audit_rate_limit=%d
audit_backlog_limit=%d\n",
| > + "audit: audit_lost=%d audit_rate_limit=%d "
| > + "audit_backlog_limit=%d\n",
| > atomic_read(_lost),
| > audit_rate_limit,
| > audit_backlog_limit);
|
| This hunk is a bit questionable. It can easily deceive a reader to
| assume two seperate lines printed out and sometimes defeats grepping
| for printk output to find the code generating the message.
|
| Rest looks good to me.
|
| Jörn
|
| --
| He that composes himself is wiser than he that composes a book.
| -- B. Franklin
|
indeed.
here is updated one (with these part removed)
Instead of removing that part completely, why not print this:
"audit: lost=%d rate_limit=%d backlog_limit=%d\n"
In that line there were too many 'audit's IMHO, and if someone wants to
grep 'audit_lost=' he still can, 'audit:.*lost=' or something like that..
tom
--
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/
Re: [x86] kernel/audit.c cleanup according to checkpatch.pl
[=?ISO-8859-1?Q?J=F6rn_Engel_ - Thu, Jan 03, 2008 at 12:29:57PM +0100]
| On Thu, 3 January 2008 14:19:25 +0300, Cyrill Gorcunov wrote:
| > @@ -232,7 +232,8 @@ void audit_log_lost(const char *message)
| >
| > if (print) {
| > printk(KERN_WARNING
| > - "audit: audit_lost=%d audit_rate_limit=%d
audit_backlog_limit=%d\n",
| > + "audit: audit_lost=%d audit_rate_limit=%d "
| > + "audit_backlog_limit=%d\n",
| >atomic_read(_lost),
| >audit_rate_limit,
| >audit_backlog_limit);
|
| This hunk is a bit questionable. It can easily deceive a reader to
| assume two seperate lines printed out and sometimes defeats grepping
| for printk output to find the code generating the message.
|
| Rest looks good to me.
|
| Jörn
|
| --
| He that composes himself is wiser than he that composes a book.
| -- B. Franklin
|
indeed.
here is updated one (with these part removed)
---
kernel/audit.c | 66 ++-
1 files changed, 36 insertions(+), 30 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index f93c271..22b951e 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -89,7 +89,7 @@ static intaudit_rate_limit;
/* Number of outstanding audit_buffers allowed. */
static int audit_backlog_limit = 64;
static int audit_backlog_wait_time = 60 * HZ;
-static int audit_backlog_wait_overflow = 0;
+static int audit_backlog_wait_overflow;
/* The identity of the user shutting down the audit system. */
uid_t audit_sig_uid = -1;
@@ -158,8 +158,7 @@ static void audit_set_pid(struct audit_buffer *ab, pid_t
pid)
void audit_panic(const char *message)
{
- switch (audit_failure)
- {
+ switch (audit_failure) {
case AUDIT_FAIL_SILENT:
break;
case AUDIT_FAIL_PRINTK:
@@ -173,15 +172,16 @@ void audit_panic(const char *message)
static inline int audit_rate_check(void)
{
- static unsigned longlast_check = 0;
- static int messages = 0;
+ static unsigned longlast_check;
+ static int messages;
static DEFINE_SPINLOCK(lock);
unsigned long flags;
unsigned long now;
unsigned long elapsed;
int retval = 0;
- if (!audit_rate_limit) return 1;
+ if (!audit_rate_limit)
+ return 1;
spin_lock_irqsave(, flags);
if (++messages < audit_rate_limit) {
@@ -210,7 +210,7 @@ static inline int audit_rate_check(void)
*/
void audit_log_lost(const char *message)
{
- static unsigned longlast_msg = 0;
+ static unsigned longlast_msg;
static DEFINE_SPINLOCK(lock);
unsigned long flags;
unsigned long now;
@@ -253,7 +253,8 @@ static int audit_set_rate_limit(int limit, uid_t loginuid,
u32 sid)
if (sid) {
char *ctx = NULL;
u32 len;
- if ((rc = selinux_sid_to_string(sid, , )) == 0) {
+ rc = selinux_sid_to_string(sid, , );
+ if (rc == 0) {
audit_log(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE,
"audit_rate_limit=%d old=%d by auid=%u"
" subj=%s res=%d",
@@ -288,7 +289,8 @@ static int audit_set_backlog_limit(int limit, uid_t
loginuid, u32 sid)
if (sid) {
char *ctx = NULL;
u32 len;
- if ((rc = selinux_sid_to_string(sid, , )) == 0) {
+ rc = selinux_sid_to_string(sid, , );
+ if (rc == 0) {
audit_log(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE,
"audit_backlog_limit=%d old=%d by auid=%u"
" subj=%s res=%d",
@@ -326,7 +328,8 @@ static int audit_set_enabled(int state, uid_t loginuid, u32
sid)
if (sid) {
char *ctx = NULL;
u32 len;
- if ((rc = selinux_sid_to_string(sid, , )) == 0) {
+ rc = selinux_sid_to_string(sid, , );
+ if (rc == 0) {
audit_log(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE,
"audit_enabled=%d old=%d by auid=%u"
" subj=%s res=%d",
@@ -366,7 +369,8 @@ static int audit_set_failure(int state, uid_t loginuid, u32
sid)
if (sid) {
char *ctx = NULL;
u32 len;
- if ((rc = selinux_sid_to_string(sid, , )) == 0) {
+ rc = selinux_sid_to_string(sid, , );
+ if (rc == 0) {
audit_log(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE,
"audit_failure=%d old=%d by auid=%u"
" subj=%s res=%d",
@@ -626,18 +630,20 @@ static int
Re: [x86] kernel/audit.c cleanup according to checkpatch.pl
On Thu, 3 January 2008 14:19:25 +0300, Cyrill Gorcunov wrote:
> @@ -232,7 +232,8 @@ void audit_log_lost(const char *message)
>
> if (print) {
> printk(KERN_WARNING
> -"audit: audit_lost=%d audit_rate_limit=%d
> audit_backlog_limit=%d\n",
> +"audit: audit_lost=%d audit_rate_limit=%d "
> +"audit_backlog_limit=%d\n",
> atomic_read(_lost),
> audit_rate_limit,
> audit_backlog_limit);
This hunk is a bit questionable. It can easily deceive a reader to
assume two seperate lines printed out and sometimes defeats grepping
for printk output to find the code generating the message.
Rest looks good to me.
Jörn
--
He that composes himself is wiser than he that composes a book.
-- B. Franklin
--
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/
[x86] kernel/audit.c cleanup according to checkpatch.pl
This patch eliminates code-style errors according
to checkpatch.pl
Signed-off-by: Cyrill Gorcunov <[EMAIL PROTECTED]>
---
Ingo, David, md5sums will be different for old and patched
asm listings due to:
- zero initialized static vars take off
- assignments moved out from 'if' conditions
- EXPORT_SYMBOL's set at appropriate places
anyway the changes are trivial, so even a glance review
would be enough to ensure that it doesn't bring errors
into the kernel
before:
total: 20 errors, 17 warnings, 1473 lines checked
after:
total: 2 errors, 12 warnings, 1480 lines checked
Any comments are welcome.
kernel/audit.c | 69 ++-
1 files changed, 38 insertions(+), 31 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index f93c271..4e71602 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -89,7 +89,7 @@ static intaudit_rate_limit;
/* Number of outstanding audit_buffers allowed. */
static int audit_backlog_limit = 64;
static int audit_backlog_wait_time = 60 * HZ;
-static int audit_backlog_wait_overflow = 0;
+static int audit_backlog_wait_overflow;
/* The identity of the user shutting down the audit system. */
uid_t audit_sig_uid = -1;
@@ -158,8 +158,7 @@ static void audit_set_pid(struct audit_buffer *ab, pid_t
pid)
void audit_panic(const char *message)
{
- switch (audit_failure)
- {
+ switch (audit_failure) {
case AUDIT_FAIL_SILENT:
break;
case AUDIT_FAIL_PRINTK:
@@ -173,15 +172,16 @@ void audit_panic(const char *message)
static inline int audit_rate_check(void)
{
- static unsigned longlast_check = 0;
- static int messages = 0;
+ static unsigned longlast_check;
+ static int messages;
static DEFINE_SPINLOCK(lock);
unsigned long flags;
unsigned long now;
unsigned long elapsed;
int retval = 0;
- if (!audit_rate_limit) return 1;
+ if (!audit_rate_limit)
+ return 1;
spin_lock_irqsave(, flags);
if (++messages < audit_rate_limit) {
@@ -210,7 +210,7 @@ static inline int audit_rate_check(void)
*/
void audit_log_lost(const char *message)
{
- static unsigned longlast_msg = 0;
+ static unsigned longlast_msg;
static DEFINE_SPINLOCK(lock);
unsigned long flags;
unsigned long now;
@@ -232,7 +232,8 @@ void audit_log_lost(const char *message)
if (print) {
printk(KERN_WARNING
- "audit: audit_lost=%d audit_rate_limit=%d
audit_backlog_limit=%d\n",
+ "audit: audit_lost=%d audit_rate_limit=%d "
+ "audit_backlog_limit=%d\n",
atomic_read(_lost),
audit_rate_limit,
audit_backlog_limit);
@@ -253,7 +254,8 @@ static int audit_set_rate_limit(int limit, uid_t loginuid,
u32 sid)
if (sid) {
char *ctx = NULL;
u32 len;
- if ((rc = selinux_sid_to_string(sid, , )) == 0) {
+ rc = selinux_sid_to_string(sid, , );
+ if (rc == 0) {
audit_log(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE,
"audit_rate_limit=%d old=%d by auid=%u"
" subj=%s res=%d",
@@ -288,7 +290,8 @@ static int audit_set_backlog_limit(int limit, uid_t
loginuid, u32 sid)
if (sid) {
char *ctx = NULL;
u32 len;
- if ((rc = selinux_sid_to_string(sid, , )) == 0) {
+ rc = selinux_sid_to_string(sid, , );
+ if (rc == 0) {
audit_log(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE,
"audit_backlog_limit=%d old=%d by auid=%u"
" subj=%s res=%d",
@@ -326,7 +329,8 @@ static int audit_set_enabled(int state, uid_t loginuid, u32
sid)
if (sid) {
char *ctx = NULL;
u32 len;
- if ((rc = selinux_sid_to_string(sid, , )) == 0) {
+ rc = selinux_sid_to_string(sid, , );
+ if (rc == 0) {
audit_log(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE,
"audit_enabled=%d old=%d by auid=%u"
" subj=%s res=%d",
@@ -366,7 +370,8 @@ static int audit_set_failure(int state, uid_t loginuid, u32
sid)
if (sid) {
char *ctx = NULL;
u32 len;
- if ((rc = selinux_sid_to_string(sid, , )) == 0) {
+ rc = selinux_sid_to_string(sid, , );
+ if (rc == 0) {
audit_log(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE,
"audit_failure=%d
[x86] kernel/audit.c cleanup according to checkpatch.pl
This patch eliminates code-style errors according
to checkpatch.pl
Signed-off-by: Cyrill Gorcunov [EMAIL PROTECTED]
---
Ingo, David, md5sums will be different for old and patched
asm listings due to:
- zero initialized static vars take off
- assignments moved out from 'if' conditions
- EXPORT_SYMBOL's set at appropriate places
anyway the changes are trivial, so even a glance review
would be enough to ensure that it doesn't bring errors
into the kernel
before:
total: 20 errors, 17 warnings, 1473 lines checked
after:
total: 2 errors, 12 warnings, 1480 lines checked
Any comments are welcome.
kernel/audit.c | 69 ++-
1 files changed, 38 insertions(+), 31 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index f93c271..4e71602 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -89,7 +89,7 @@ static intaudit_rate_limit;
/* Number of outstanding audit_buffers allowed. */
static int audit_backlog_limit = 64;
static int audit_backlog_wait_time = 60 * HZ;
-static int audit_backlog_wait_overflow = 0;
+static int audit_backlog_wait_overflow;
/* The identity of the user shutting down the audit system. */
uid_t audit_sig_uid = -1;
@@ -158,8 +158,7 @@ static void audit_set_pid(struct audit_buffer *ab, pid_t
pid)
void audit_panic(const char *message)
{
- switch (audit_failure)
- {
+ switch (audit_failure) {
case AUDIT_FAIL_SILENT:
break;
case AUDIT_FAIL_PRINTK:
@@ -173,15 +172,16 @@ void audit_panic(const char *message)
static inline int audit_rate_check(void)
{
- static unsigned longlast_check = 0;
- static int messages = 0;
+ static unsigned longlast_check;
+ static int messages;
static DEFINE_SPINLOCK(lock);
unsigned long flags;
unsigned long now;
unsigned long elapsed;
int retval = 0;
- if (!audit_rate_limit) return 1;
+ if (!audit_rate_limit)
+ return 1;
spin_lock_irqsave(lock, flags);
if (++messages audit_rate_limit) {
@@ -210,7 +210,7 @@ static inline int audit_rate_check(void)
*/
void audit_log_lost(const char *message)
{
- static unsigned longlast_msg = 0;
+ static unsigned longlast_msg;
static DEFINE_SPINLOCK(lock);
unsigned long flags;
unsigned long now;
@@ -232,7 +232,8 @@ void audit_log_lost(const char *message)
if (print) {
printk(KERN_WARNING
- audit: audit_lost=%d audit_rate_limit=%d
audit_backlog_limit=%d\n,
+ audit: audit_lost=%d audit_rate_limit=%d
+ audit_backlog_limit=%d\n,
atomic_read(audit_lost),
audit_rate_limit,
audit_backlog_limit);
@@ -253,7 +254,8 @@ static int audit_set_rate_limit(int limit, uid_t loginuid,
u32 sid)
if (sid) {
char *ctx = NULL;
u32 len;
- if ((rc = selinux_sid_to_string(sid, ctx, len)) == 0) {
+ rc = selinux_sid_to_string(sid, ctx, len);
+ if (rc == 0) {
audit_log(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE,
audit_rate_limit=%d old=%d by auid=%u
subj=%s res=%d,
@@ -288,7 +290,8 @@ static int audit_set_backlog_limit(int limit, uid_t
loginuid, u32 sid)
if (sid) {
char *ctx = NULL;
u32 len;
- if ((rc = selinux_sid_to_string(sid, ctx, len)) == 0) {
+ rc = selinux_sid_to_string(sid, ctx, len);
+ if (rc == 0) {
audit_log(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE,
audit_backlog_limit=%d old=%d by auid=%u
subj=%s res=%d,
@@ -326,7 +329,8 @@ static int audit_set_enabled(int state, uid_t loginuid, u32
sid)
if (sid) {
char *ctx = NULL;
u32 len;
- if ((rc = selinux_sid_to_string(sid, ctx, len)) == 0) {
+ rc = selinux_sid_to_string(sid, ctx, len);
+ if (rc == 0) {
audit_log(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE,
audit_enabled=%d old=%d by auid=%u
subj=%s res=%d,
@@ -366,7 +370,8 @@ static int audit_set_failure(int state, uid_t loginuid, u32
sid)
if (sid) {
char *ctx = NULL;
u32 len;
- if ((rc = selinux_sid_to_string(sid, ctx, len)) == 0) {
+ rc = selinux_sid_to_string(sid, ctx, len);
+ if (rc == 0) {
audit_log(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE,
Re: [x86] kernel/audit.c cleanup according to checkpatch.pl
On Thu, 3 January 2008 14:19:25 +0300, Cyrill Gorcunov wrote:
@@ -232,7 +232,8 @@ void audit_log_lost(const char *message)
if (print) {
printk(KERN_WARNING
-audit: audit_lost=%d audit_rate_limit=%d
audit_backlog_limit=%d\n,
+audit: audit_lost=%d audit_rate_limit=%d
+audit_backlog_limit=%d\n,
atomic_read(audit_lost),
audit_rate_limit,
audit_backlog_limit);
This hunk is a bit questionable. It can easily deceive a reader to
assume two seperate lines printed out and sometimes defeats grepping
for printk output to find the code generating the message.
Rest looks good to me.
Jörn
--
He that composes himself is wiser than he that composes a book.
-- B. Franklin
--
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/
Re: [x86] kernel/audit.c cleanup according to checkpatch.pl
[=?ISO-8859-1?Q?J=F6rn_Engel_ - Thu, Jan 03, 2008 at 12:29:57PM +0100]
| On Thu, 3 January 2008 14:19:25 +0300, Cyrill Gorcunov wrote:
| @@ -232,7 +232,8 @@ void audit_log_lost(const char *message)
|
| if (print) {
| printk(KERN_WARNING
| - audit: audit_lost=%d audit_rate_limit=%d
audit_backlog_limit=%d\n,
| + audit: audit_lost=%d audit_rate_limit=%d
| + audit_backlog_limit=%d\n,
| atomic_read(audit_lost),
| audit_rate_limit,
| audit_backlog_limit);
|
| This hunk is a bit questionable. It can easily deceive a reader to
| assume two seperate lines printed out and sometimes defeats grepping
| for printk output to find the code generating the message.
|
| Rest looks good to me.
|
| Jörn
|
| --
| He that composes himself is wiser than he that composes a book.
| -- B. Franklin
|
indeed.
here is updated one (with these part removed)
---
kernel/audit.c | 66 ++-
1 files changed, 36 insertions(+), 30 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index f93c271..22b951e 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -89,7 +89,7 @@ static intaudit_rate_limit;
/* Number of outstanding audit_buffers allowed. */
static int audit_backlog_limit = 64;
static int audit_backlog_wait_time = 60 * HZ;
-static int audit_backlog_wait_overflow = 0;
+static int audit_backlog_wait_overflow;
/* The identity of the user shutting down the audit system. */
uid_t audit_sig_uid = -1;
@@ -158,8 +158,7 @@ static void audit_set_pid(struct audit_buffer *ab, pid_t
pid)
void audit_panic(const char *message)
{
- switch (audit_failure)
- {
+ switch (audit_failure) {
case AUDIT_FAIL_SILENT:
break;
case AUDIT_FAIL_PRINTK:
@@ -173,15 +172,16 @@ void audit_panic(const char *message)
static inline int audit_rate_check(void)
{
- static unsigned longlast_check = 0;
- static int messages = 0;
+ static unsigned longlast_check;
+ static int messages;
static DEFINE_SPINLOCK(lock);
unsigned long flags;
unsigned long now;
unsigned long elapsed;
int retval = 0;
- if (!audit_rate_limit) return 1;
+ if (!audit_rate_limit)
+ return 1;
spin_lock_irqsave(lock, flags);
if (++messages audit_rate_limit) {
@@ -210,7 +210,7 @@ static inline int audit_rate_check(void)
*/
void audit_log_lost(const char *message)
{
- static unsigned longlast_msg = 0;
+ static unsigned longlast_msg;
static DEFINE_SPINLOCK(lock);
unsigned long flags;
unsigned long now;
@@ -253,7 +253,8 @@ static int audit_set_rate_limit(int limit, uid_t loginuid,
u32 sid)
if (sid) {
char *ctx = NULL;
u32 len;
- if ((rc = selinux_sid_to_string(sid, ctx, len)) == 0) {
+ rc = selinux_sid_to_string(sid, ctx, len);
+ if (rc == 0) {
audit_log(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE,
audit_rate_limit=%d old=%d by auid=%u
subj=%s res=%d,
@@ -288,7 +289,8 @@ static int audit_set_backlog_limit(int limit, uid_t
loginuid, u32 sid)
if (sid) {
char *ctx = NULL;
u32 len;
- if ((rc = selinux_sid_to_string(sid, ctx, len)) == 0) {
+ rc = selinux_sid_to_string(sid, ctx, len);
+ if (rc == 0) {
audit_log(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE,
audit_backlog_limit=%d old=%d by auid=%u
subj=%s res=%d,
@@ -326,7 +328,8 @@ static int audit_set_enabled(int state, uid_t loginuid, u32
sid)
if (sid) {
char *ctx = NULL;
u32 len;
- if ((rc = selinux_sid_to_string(sid, ctx, len)) == 0) {
+ rc = selinux_sid_to_string(sid, ctx, len);
+ if (rc == 0) {
audit_log(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE,
audit_enabled=%d old=%d by auid=%u
subj=%s res=%d,
@@ -366,7 +369,8 @@ static int audit_set_failure(int state, uid_t loginuid, u32
sid)
if (sid) {
char *ctx = NULL;
u32 len;
- if ((rc = selinux_sid_to_string(sid, ctx, len)) == 0) {
+ rc = selinux_sid_to_string(sid, ctx, len);
+ if (rc == 0) {
audit_log(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE,
audit_failure=%d old=%d by auid=%u
subj=%s res=%d,
@@ -626,18
Re: [x86] kernel/audit.c cleanup according to checkpatch.pl
[Tomas Carnecky - Thu, Jan 03, 2008 at 01:10:28PM +0100]
Cyrill Gorcunov wrote:
[=?ISO-8859-1?Q?J=F6rn_Engel_ - Thu, Jan 03, 2008 at 12:29:57PM +0100]
| On Thu, 3 January 2008 14:19:25 +0300, Cyrill Gorcunov wrote:
| @@ -232,7 +232,8 @@ void audit_log_lost(const char *message)
| | if (print) {
| printk(KERN_WARNING
| - audit: audit_lost=%d audit_rate_limit=%d
audit_backlog_limit=%d\n,
| + audit: audit_lost=%d audit_rate_limit=%d
| + audit_backlog_limit=%d\n,
| atomic_read(audit_lost),
| audit_rate_limit,
| audit_backlog_limit);
| | This hunk is a bit questionable. It can easily deceive a reader to
| assume two seperate lines printed out and sometimes defeats grepping
| for printk output to find the code generating the message.
| | Rest looks good to me.
| | Jörn
| | -- | He that composes himself is wiser than he that composes a book.
| -- B. Franklin
| indeed.
here is updated one (with these part removed)
Instead of removing that part completely, why not print this:
audit: lost=%d rate_limit=%d backlog_limit=%d\n
In that line there were too many 'audit's IMHO, and if someone wants to
grep 'audit_lost=' he still can, 'audit:.*lost=' or something like that..
tom
Well, it seems David is a mainteiner of this code,
so if he would not argue against this the we could.
- Cyrill -
--
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/
Re: [x86] kernel/audit.c cleanup according to checkpatch.pl
Cyrill Gorcunov wrote:
[=?ISO-8859-1?Q?J=F6rn_Engel_ - Thu, Jan 03, 2008 at 12:29:57PM +0100]
| On Thu, 3 January 2008 14:19:25 +0300, Cyrill Gorcunov wrote:
| @@ -232,7 +232,8 @@ void audit_log_lost(const char *message)
|
| if (print) {
| printk(KERN_WARNING
| - audit: audit_lost=%d audit_rate_limit=%d
audit_backlog_limit=%d\n,
| + audit: audit_lost=%d audit_rate_limit=%d
| + audit_backlog_limit=%d\n,
| atomic_read(audit_lost),
| audit_rate_limit,
| audit_backlog_limit);
|
| This hunk is a bit questionable. It can easily deceive a reader to
| assume two seperate lines printed out and sometimes defeats grepping
| for printk output to find the code generating the message.
|
| Rest looks good to me.
|
| Jörn
|
| --
| He that composes himself is wiser than he that composes a book.
| -- B. Franklin
|
indeed.
here is updated one (with these part removed)
Instead of removing that part completely, why not print this:
audit: lost=%d rate_limit=%d backlog_limit=%d\n
In that line there were too many 'audit's IMHO, and if someone wants to
grep 'audit_lost=' he still can, 'audit:.*lost=' or something like that..
tom
--
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/
Re: [x86] kernel/audit.c cleanup according to checkpatch.pl
* Jörn Engel [EMAIL PROTECTED] wrote: - audit: audit_lost=%d audit_rate_limit=%d audit_backlog_limit=%d\n, + audit: audit_lost=%d audit_rate_limit=%d + audit_backlog_limit=%d\n, atomic_read(audit_lost), audit_rate_limit, audit_backlog_limit); This hunk is a bit questionable. It can easily deceive a reader to assume two seperate lines printed out and sometimes defeats grepping for printk output to find the code generating the message. not to make a big issue out of this, but when was the last time you tried to grep this way: grep -E audit_rate_limit=[0-9]+ audit_backlog */*.c ? That's pretty much the only grep pattern that would break. People usually grep on the constant portion of the string, so breaking up a line along a variable boundary is perfectly okay. Ingo -- 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/
Re: [x86] kernel/audit.c cleanup according to checkpatch.pl
* David Woodhouse [EMAIL PROTECTED] wrote: On Thu, 2008-01-03 at 15:05 +0100, Ingo Molnar wrote: not to make a big issue out of this, but when was the last time you tried to grep this way: grep -E audit_rate_limit=[0-9]+ audit_backlog */*.c Not precisely that, but I've certainly had greps fail because people have split up strings to meet the stupid 80-character limit. yes - but if you read my whole reply you'll see that i qualified it: That's pretty much the only grep pattern that would break. People usually grep on the constant portion of the string, so breaking up a ^ line along a variable boundary is perfectly okay. Ingo -- 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/
Re: [x86] kernel/audit.c cleanup according to checkpatch.pl
[David Woodhouse - Thu, Jan 03, 2008 at 02:37:24PM +] | | On Thu, 2008-01-03 at 15:37 +0100, Ingo Molnar wrote: | * David Woodhouse [EMAIL PROTECTED] wrote: | | On Thu, 2008-01-03 at 15:05 +0100, Ingo Molnar wrote: |not to make a big issue out of this, but when was the last time you |tried to grep this way: | | grep -E audit_rate_limit=[0-9]+ audit_backlog */*.c | | Not precisely that, but I've certainly had greps fail because people | have split up strings to meet the stupid 80-character limit. | | yes - but if you read my whole reply you'll see that i qualified it: | | That's pretty much the only grep pattern that would break. People | usually grep on the constant portion of the string, so breaking up a | ^ | line along a variable boundary is perfectly okay. | | Yes, you did. But you failed to provide any good reason for actually | changing it, either. Leave it as it was. | | -- | dwmw2 | so what i would do now? i could post updated patch *without* that splitted line, should I? - Cyrill - -- 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/
Re: [x86] kernel/audit.c cleanup according to checkpatch.pl
On Thu, 2008-01-03 at 15:37 +0100, Ingo Molnar wrote: * David Woodhouse [EMAIL PROTECTED] wrote: On Thu, 2008-01-03 at 15:05 +0100, Ingo Molnar wrote: not to make a big issue out of this, but when was the last time you tried to grep this way: grep -E audit_rate_limit=[0-9]+ audit_backlog */*.c Not precisely that, but I've certainly had greps fail because people have split up strings to meet the stupid 80-character limit. yes - but if you read my whole reply you'll see that i qualified it: That's pretty much the only grep pattern that would break. People usually grep on the constant portion of the string, so breaking up a ^ line along a variable boundary is perfectly okay. Yes, you did. But you failed to provide any good reason for actually changing it, either. Leave it as it was. -- dwmw2 -- 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/
Re: [x86] kernel/audit.c cleanup according to checkpatch.pl
On Thu, 2008-01-03 at 17:50 +0300, Cyrill Gorcunov wrote: so what i would do now? i could post updated patch *without* that splitted line, should I? And with the if (rc == 0) thing fixed too. Yes please. -- dwmw2 -- 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/
Re: [x86] kernel/audit.c cleanup according to checkpatch.pl
[David Woodhouse - Thu, Jan 03, 2008 at 02:57:08PM +]
|
| On Thu, 2008-01-03 at 17:50 +0300, Cyrill Gorcunov wrote:
|
|
| so what i would do now? i could post updated patch *without* that
| splitted line, should I?
|
| And with the if (rc == 0) thing fixed too. Yes please.
|
| --
| dwmw2
|
here is an updated patch
---
From: Cyrill Gorcunov [EMAIL PROTECTED]
Subject: [x86] kernel/audit.c cleanup according to checkpatch.pl
This patch eliminates most of errors pointed by checkpatch.pl
over kernel/audit.c file.
Signed-off-by: Cyrill Gorcunov [EMAIL PROTECTED]
---
David, to use 'if (rc)' form instead of 'if (rc == 0)' i had
to move 'kfree(ctx)' out of if-else condition but that is OK
'case a lot of further audit.c code was doing the same ;)
kernel/audit.c | 126 +--
1 files changed, 66 insertions(+), 60 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index f93c271..2961ee4 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -89,7 +89,7 @@ static intaudit_rate_limit;
/* Number of outstanding audit_buffers allowed. */
static int audit_backlog_limit = 64;
static int audit_backlog_wait_time = 60 * HZ;
-static int audit_backlog_wait_overflow = 0;
+static int audit_backlog_wait_overflow;
/* The identity of the user shutting down the audit system. */
uid_t audit_sig_uid = -1;
@@ -136,7 +136,7 @@ static DEFINE_MUTEX(audit_cmd_mutex);
/* AUDIT_MAXFREE is the number of empty audit_buffers we keep on the
* audit_freelist. Doing so eliminates many kmalloc/kfree calls. */
-#define AUDIT_MAXFREE (2*NR_CPUS)
+#define AUDIT_MAXFREE (2 * NR_CPUS)
/* The audit_buffer is used when formatting an audit record. The caller
* locks briefly to get the record off the freelist or to allocate the
@@ -158,8 +158,7 @@ static void audit_set_pid(struct audit_buffer *ab, pid_t
pid)
void audit_panic(const char *message)
{
- switch (audit_failure)
- {
+ switch (audit_failure) {
case AUDIT_FAIL_SILENT:
break;
case AUDIT_FAIL_PRINTK:
@@ -173,15 +172,16 @@ void audit_panic(const char *message)
static inline int audit_rate_check(void)
{
- static unsigned longlast_check = 0;
- static int messages = 0;
+ static unsigned longlast_check;
+ static int messages;
static DEFINE_SPINLOCK(lock);
unsigned long flags;
unsigned long now;
unsigned long elapsed;
int retval = 0;
- if (!audit_rate_limit) return 1;
+ if (!audit_rate_limit)
+ return 1;
spin_lock_irqsave(lock, flags);
if (++messages audit_rate_limit) {
@@ -210,7 +210,7 @@ static inline int audit_rate_check(void)
*/
void audit_log_lost(const char *message)
{
- static unsigned longlast_msg = 0;
+ static unsigned longlast_msg;
static DEFINE_SPINLOCK(lock);
unsigned long flags;
unsigned long now;
@@ -253,14 +253,15 @@ static int audit_set_rate_limit(int limit, uid_t
loginuid, u32 sid)
if (sid) {
char *ctx = NULL;
u32 len;
- if ((rc = selinux_sid_to_string(sid, ctx, len)) == 0) {
- audit_log(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE,
- audit_rate_limit=%d old=%d by auid=%u
-subj=%s res=%d,
- limit, old, loginuid, ctx, res);
- kfree(ctx);
- } else
+ rc = selinux_sid_to_string(sid, ctx, len);
+ if (rc)
res = 0; /* Something weird, deny request */
+ else
+ audit_log(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE,
+ audit_rate_limit=%d old=%d by auid=%u
+ subj=%s res=%d,
+ limit, old, loginuid, ctx, res);
+ kfree(ctx);
}
audit_log(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE,
audit_rate_limit=%d old=%d by auid=%u res=%d,
@@ -288,14 +289,15 @@ static int audit_set_backlog_limit(int limit, uid_t
loginuid, u32 sid)
if (sid) {
char *ctx = NULL;
u32 len;
- if ((rc = selinux_sid_to_string(sid, ctx, len)) == 0) {
- audit_log(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE,
- audit_backlog_limit=%d old=%d by auid=%u
-subj=%s res=%d,
- limit, old, loginuid, ctx, res);
- kfree(ctx);
- } else
+ rc = selinux_sid_to_string(sid, ctx, len);
+ if (rc)
res = 0; /* Something weird, deny request */
+ else
