Hi
I am proposing these 4 patches.
I am currently learning the core kernel the hard way.
I will appreciate any feedback negative or positive.
Thanks
Jules Irenge (4):
acct: Add required space between variable and operator
audit: uninitialize global variable audit_sig_sid
audit: uninitialize static variables
context_tracking: uninitialize static variables
kernel/acct.c | 2 +-
kernel/audit.c | 10 +++++-----
kernel/context_tracking.c | 2 +-
3 files changed, 7 insertions(+), 7 deletions(-)
--
2.26.2
Checkpatch tool reports an error at variable declaration
"ERROR: do not initialise statics to 0"
This is due to the fact that these variables are stored in the buffer
In the .bss section, one can not set an initial value
Here we can trust the compiler to automatically set them to zero.
The variable has since been uninitialized.
Signed-off-by: Jules Irenge <[email protected]>
---
kernel/audit.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 7b1a38a211a9..7d79ecb58b01 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -311,8 +311,8 @@ void audit_panic(const char *message)
static inline int audit_rate_check(void)
{
- static unsigned long last_check = 0;
- static int messages = 0;
+ static unsigned long last_check;
+ static int messages;
static DEFINE_SPINLOCK(lock);
unsigned long flags;
unsigned long now;
@@ -348,7 +348,7 @@ static inline int audit_rate_check(void)
*/
void audit_log_lost(const char *message)
{
- static unsigned long last_msg = 0;
+ static unsigned long last_msg;
static DEFINE_SPINLOCK(lock);
unsigned long flags;
unsigned long now;
@@ -713,7 +713,7 @@ static int kauditd_send_queue(struct sock *sk, u32 portid,
{
int rc = 0;
struct sk_buff *skb;
- static unsigned int failed = 0;
+ static unsigned int failed;
/* NOTE: kauditd_thread takes care of all our locking, we just use
* the netlink info passed to us (e.g. sk and portid) */
--
2.26.2
Checkpatch tool reports an error at a staic variable declaration
"ERROR: do not initialise statics to false"
This is due to the fact that this variable is stored in the buffer
In the .bss section, one can not set an initial value
Here we can trust the compiler to automatically set them to false.
The variable has since been uninitialized.
Signed-off-by: Jules Irenge <[email protected]>
---
kernel/context_tracking.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/context_tracking.c b/kernel/context_tracking.c
index 36a98c48aedc..21881c534152 100644
--- a/kernel/context_tracking.c
+++ b/kernel/context_tracking.c
@@ -190,7 +190,7 @@ NOKPROBE_SYMBOL(context_tracking_user_exit);
void __init context_tracking_cpu_set(int cpu)
{
- static __initdata bool initialized = false;
+ static __initdata bool initialized;
if (!per_cpu(context_tracking.active, cpu)) {
per_cpu(context_tracking.active, cpu) = true;
--
2.26.2
Checkpatch tool reports an error
"ERROR: spaces required around that == (ctx:VxV)"
To fix this space has been added between the variable,
the operator and the value.
Add the missing required space.
Signed-off-by: Jules Irenge <[email protected]>
---
kernel/acct.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/acct.c b/kernel/acct.c
index b0c5b3a9f5af..d7cc5f917e11 100644
--- a/kernel/acct.c
+++ b/kernel/acct.c
@@ -451,7 +451,7 @@ static void fill_ac(acct_t *ac)
do_div(elapsed, AHZ);
btime = ktime_get_real_seconds() - elapsed;
ac->ac_btime = clamp_t(time64_t, btime, 0, U32_MAX);
-#if ACCT_VERSION==2
+#if ACCT_VERSION == 2
ac->ac_ahz = AHZ;
#endif
--
2.26.2
Checkpatch tool reports an error at variable audit_sig_sid declaration
"ERROR: do not initialise globals to 0"
To fix this, the global variable has been uninitialized.
Signed-off-by: Jules Irenge <[email protected]>
---
kernel/audit.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 8c201f414226..7b1a38a211a9 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -125,7 +125,7 @@ static u32 audit_backlog_wait_time = AUDIT_BACKLOG_WAIT_TIME;
/* The identity of the user shutting down the audit system. */
kuid_t audit_sig_uid = INVALID_UID;
pid_t audit_sig_pid = -1;
-u32 audit_sig_sid = 0;
+u32 audit_sig_sid;
/* Records can be lost in several ways:
0) [suppressed in audit_alloc]
--
2.26.2
On Sat, 2020-08-01 at 19:46 +0100, Jules Irenge wrote:
> Checkpatch tool reports an error at variable audit_sig_sid declaration
[]
> diff --git a/kernel/audit.c b/kernel/audit.c
[]
> @@ -125,7 +125,7 @@ static u32 audit_backlog_wait_time = AUDIT_BACKLOG_WAIT_TIME;
> /* The identity of the user shutting down the audit system. */
> kuid_t audit_sig_uid = INVALID_UID;
> pid_t audit_sig_pid = -1;
> -u32 audit_sig_sid = 0;
> +u32 audit_sig_sid;
All of these are unused outside of audit.c and might as
well be static and removed from the .h file.
On Sat, 2020-08-01 at 19:45 +0100, Jules Irenge wrote:
> Hi
> I am proposing these 4 patches.
> I am currently learning the core kernel the hard way.
> I will appreciate any feedback negative or positive.
> Thanks
Generally, whitespace only changes outside of drivers/staging
are not encouraged.
> Jules Irenge (4):
> acct: Add required space between variable and operator
> audit: uninitialize global variable audit_sig_sid
> audit: uninitialize static variables
> context_tracking: uninitialize static variables
>
> kernel/acct.c | 2 +-
> kernel/audit.c | 10 +++++-----
> kernel/context_tracking.c | 2 +-
> 3 files changed, 7 insertions(+), 7 deletions(-)
>
On Sat, Aug 1, 2020 at 2:46 PM Jules Irenge <[email protected]> wrote:
>
> Checkpatch tool reports an error at variable declaration
>
> "ERROR: do not initialise statics to 0"
>
> This is due to the fact that these variables are stored in the buffer
> In the .bss section, one can not set an initial value
> Here we can trust the compiler to automatically set them to zero.
> The variable has since been uninitialized.
>
> Signed-off-by: Jules Irenge <[email protected]>
> ---
> kernel/audit.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
In general this is fine, but it will have to wait until after the
merge window closes.
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 7b1a38a211a9..7d79ecb58b01 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -311,8 +311,8 @@ void audit_panic(const char *message)
>
> static inline int audit_rate_check(void)
> {
> - static unsigned long last_check = 0;
> - static int messages = 0;
> + static unsigned long last_check;
> + static int messages;
> static DEFINE_SPINLOCK(lock);
> unsigned long flags;
> unsigned long now;
> @@ -348,7 +348,7 @@ static inline int audit_rate_check(void)
> */
> void audit_log_lost(const char *message)
> {
> - static unsigned long last_msg = 0;
> + static unsigned long last_msg;
> static DEFINE_SPINLOCK(lock);
> unsigned long flags;
> unsigned long now;
> @@ -713,7 +713,7 @@ static int kauditd_send_queue(struct sock *sk, u32 portid,
> {
> int rc = 0;
> struct sk_buff *skb;
> - static unsigned int failed = 0;
> + static unsigned int failed;
>
> /* NOTE: kauditd_thread takes care of all our locking, we just use
> * the netlink info passed to us (e.g. sk and portid) */
> --
> 2.26.2
--
paul moore
http://www.paul-moore.com
On Sat, Aug 1, 2020 at 2:55 PM Joe Perches <[email protected]> wrote:
> On Sat, 2020-08-01 at 19:46 +0100, Jules Irenge wrote:
> > Checkpatch tool reports an error at variable audit_sig_sid declaration
> []
> > diff --git a/kernel/audit.c b/kernel/audit.c
> []
> > @@ -125,7 +125,7 @@ static u32 audit_backlog_wait_time = AUDIT_BACKLOG_WAIT_TIME;
> > /* The identity of the user shutting down the audit system. */
> > kuid_t audit_sig_uid = INVALID_UID;
> > pid_t audit_sig_pid = -1;
> > -u32 audit_sig_sid = 0;
> > +u32 audit_sig_sid;
>
> All of these are unused outside of audit.c and might as
> well be static and removed from the .h file.
There's plenty of time before the merge window closes, doing this
would definitely make this patch much more useful than the typical
checkpatch noise.
--
paul moore
http://www.paul-moore.com
On Sat, 1 Aug 2020, Joe Perches wrote:
> On Sat, 2020-08-01 at 19:46 +0100, Jules Irenge wrote:
> > Checkpatch tool reports an error at variable audit_sig_sid declaration
> []
> > diff --git a/kernel/audit.c b/kernel/audit.c
> []
> > @@ -125,7 +125,7 @@ static u32 audit_backlog_wait_time = AUDIT_BACKLOG_WAIT_TIME;
> > /* The identity of the user shutting down the audit system. */
> > kuid_t audit_sig_uid = INVALID_UID;
> > pid_t audit_sig_pid = -1;
> > -u32 audit_sig_sid = 0;
> > +u32 audit_sig_sid;
>
> All of these are unused outside of audit.c and might as
> well be static and removed from the .h file.
>
Thanks for reply, I will resend a second version with the recommendation,
namely make the above static and remove them from the .h file.
Jules
On Sat, 1 Aug 2020, Joe Perches wrote:
> On Sat, 2020-08-01 at 19:45 +0100, Jules Irenge wrote:
> > Hi
> > I am proposing these 4 patches.
> > I am currently learning the core kernel the hard way.
> > I will appreciate any feedback negative or positive.
> > Thanks
>
> Generally, whitespace only changes outside of drivers/staging
> are not encouraged.
>
> > Jules Irenge (4):
> > acct: Add required space between variable and operator
> > audit: uninitialize global variable audit_sig_sid
> > audit: uninitialize static variables
> > context_tracking: uninitialize static variables
> >
> > kernel/acct.c | 2 +-
> > kernel/audit.c | 10 +++++-----
> > kernel/context_tracking.c | 2 +-
> > 3 files changed, 7 insertions(+), 7 deletions(-)
> >
>
Thanks, I take good note.
Kind regards,
Jules
On Sat, Aug 01, 2020 at 07:46:03PM +0100, Jules Irenge wrote:
> Checkpatch tool reports an error at a staic variable declaration
>
> "ERROR: do not initialise statics to false"
>
> This is due to the fact that this variable is stored in the buffer
> In the .bss section, one can not set an initial value
> Here we can trust the compiler to automatically set them to false.
> The variable has since been uninitialized.
>
> Signed-off-by: Jules Irenge <[email protected]>
> ---
> kernel/context_tracking.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/context_tracking.c b/kernel/context_tracking.c
> index 36a98c48aedc..21881c534152 100644
> --- a/kernel/context_tracking.c
> +++ b/kernel/context_tracking.c
> @@ -190,7 +190,7 @@ NOKPROBE_SYMBOL(context_tracking_user_exit);
>
> void __init context_tracking_cpu_set(int cpu)
> {
> - static __initdata bool initialized = false;
> + static __initdata bool initialized;
So personally I prefer having the '= false' there. It used to be that
compilers were stupid and would put any initialized static variable in
.data, even if it was initialized with 0. But AFAIK compilers are no
longer that stupid.
On Mon, 3 Aug 2020, [email protected] wrote:
> So personally I prefer having the '= false' there. It used to be that
> compilers were stupid and would put any initialized static variable in
> .data, even if it was initialized with 0. But AFAIK compilers are no
> longer that stupid.
>
Thanks for the reply, I completely understand the precaution measure.