[PATCH] sched: add statics, don't return void expressions

[PATCH] sched: add statics, don't return void expressions

[Harvey Harrison]

Noticed by sparse:
kernel/sched.c:760:20: warning: symbol 'sched_feat_names' was not declared. Should it be static?
kernel/sched.c:767:5: warning: symbol 'sched_feat_open' was not declared. Should it be static?
kernel/sched_fair.c:845:3: warning: returning void-valued expression
kernel/sched.c:4386:3: warning: returning void-valued expression

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
 kernel/sched.c      |   10 ++++++----
 kernel/sched_fair.c |    6 ++++--
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/kernel/sched.c b/kernel/sched.c
index 0014b03..81ff960 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -757,14 +757,14 @@ const_debug unsigned int sysctl_sched_features =
 #define SCHED_FEAT(name, enabled)	\
 	#name ,
 
-__read_mostly char *sched_feat_names[] = {
+static __read_mostly char *sched_feat_names[] = {
 #include "sched_features.h"
 	NULL
 };
 
 #undef SCHED_FEAT
 
-int sched_feat_open(struct inode *inode, struct file *filp)
+static int sched_feat_open(struct inode *inode, struct file *filp)
 {
 	filp->private_data = inode->i_private;
 	return 0;
@@ -4382,8 +4382,10 @@ void account_system_time(struct task_struct *p, int hardirq_offset,
 	struct rq *rq = this_rq();
 	cputime64_t tmp;
 
-	if ((p->flags & PF_VCPU) && (irq_count() - hardirq_offset == 0))
-		return account_guest_time(p, cputime);
+	if ((p->flags & PF_VCPU) && (irq_count() - hardirq_offset == 0)) {
+		account_guest_time(p, cputime);
+		return;
+	}
 
 	p->stime = cputime_add(p->stime, cputime);
 
diff --git a/kernel/sched_fair.c b/kernel/sched_fair.c
index 89fa32b..0bd575e 100644
--- a/kernel/sched_fair.c
+++ b/kernel/sched_fair.c
@@ -841,8 +841,10 @@ entity_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr, int queued)
 	 * queued ticks are scheduled to match the slice, so don't bother
 	 * validating it and just reschedule.
 	 */
-	if (queued)
-		return resched_task(rq_of(cfs_rq)->curr);
+	if (queued) {
+		resched_task(rq_of(cfs_rq)->curr);
+		return;
+	}
 	/*
 	 * don't let the period tick interfere with the hrtick preemption
 	 */
-- 
1.5.5.1.270.g89765

Re: [PATCH] sched: add statics, don't return void expressions

[Peter Zijlstra]

On Thu, 2008-04-24 at 18:17 -0700, Harvey Harrison wrote:
> Noticed by sparse:
> kernel/sched.c:760:20: warning: symbol 'sched_feat_names' was not declared. Should it be static?
> kernel/sched.c:767:5: warning: symbol 'sched_feat_open' was not declared. Should it be static?
> kernel/sched_fair.c:845:3: warning: returning void-valued expression
> kernel/sched.c:4386:3: warning: returning void-valued expression

I'm still of two minds about that latter warning, I think:

void foo(void);

void bar(void)
{
	return foo();
}

isn't wrong, as the return types match.

Re: [PATCH] sched: add statics, don't return void expressions

[Jan Engelhardt]

On Friday 2008-04-25 14:17, Peter Zijlstra wrote:
>On Thu, 2008-04-24 at 18:17 -0700, Harvey Harrison wrote:
>> Noticed by sparse:
>> kernel/sched.c:760:20: warning: symbol 'sched_feat_names' was not declared. Should it be static?
>> kernel/sched.c:767:5: warning: symbol 'sched_feat_open' was not declared. Should it be static?
>> kernel/sched_fair.c:845:3: warning: returning void-valued expression
>> kernel/sched.c:4386:3: warning: returning void-valued expression
>
>I'm still of two minds about that latter warning, I think:
>
>void foo(void);
>
>void bar(void)
>{
>	return foo();
>}
>
>isn't wrong, as the return types match.

But you could save a keyword and use

void bar(void)
{
	foo();
}

Re: [PATCH] sched: add statics, don't return void expressions

[Peter Zijlstra]

On Fri, 2008-04-25 at 14:32 +0200, Jan Engelhardt wrote:
> On Friday 2008-04-25 14:17, Peter Zijlstra wrote:
> >On Thu, 2008-04-24 at 18:17 -0700, Harvey Harrison wrote:
> >> Noticed by sparse:
> >> kernel/sched.c:760:20: warning: symbol 'sched_feat_names' was not declared. Should it be static?
> >> kernel/sched.c:767:5: warning: symbol 'sched_feat_open' was not declared. Should it be static?
> >> kernel/sched_fair.c:845:3: warning: returning void-valued expression
> >> kernel/sched.c:4386:3: warning: returning void-valued expression
> >
> >I'm still of two minds about that latter warning, I think:
> >
> >void foo(void);
> >
> >void bar(void)
> >{
> >	return foo();
> >}
> >
> >isn't wrong, as the return types match.
> 
> But you could save a keyword and use
> 
> void bar(void)
> {
> 	foo();
> }

Sure, but this was just to show the idiom at hand; an actual use case
would be something like this:

void bar(void)
{
	if (cond)
		return foo();

	/* do stuff ourselves */
}

Leaving out the return here does have side-effects.

Re: [PATCH] sched: add statics, don't return void expressions

[Jan Engelhardt]

On Friday 2008-04-25 14:42, Peter Zijlstra wrote:
>
>Sure, but this was just to show the idiom at hand; an actual use case
>would be something like this:
>
>void bar(void)
>{
>	if (cond)
>		return foo();
>
>	/* do stuff ourselves */
>}
>
>Leaving out the return here does have side-effects.
>
Of course. You do not get that luxury in the middle of the function :^)

Re: [PATCH] sched: add statics, don't return void expressions

[Harvey Harrison]

On Fri, 2008-04-25 at 14:17 +0200, Peter Zijlstra wrote:
> On Thu, 2008-04-24 at 18:17 -0700, Harvey Harrison wrote:
> > Noticed by sparse:
> > kernel/sched.c:760:20: warning: symbol 'sched_feat_names' was not declared. Should it be static?
> > kernel/sched.c:767:5: warning: symbol 'sched_feat_open' was not declared. Should it be static?
> > kernel/sched_fair.c:845:3: warning: returning void-valued expression
> > kernel/sched.c:4386:3: warning: returning void-valued expression
> 
> I'm still of two minds about that latter warning, I think:
> 
> void foo(void);
> 
> void bar(void)
> {
> 	return foo();
> }
> 
> isn't wrong, as the return types match.
> 

True it's not wrong, but it is a trivial way to reduce the sparse noise
somewhat which can make it harder to see real bugs.  Just like the
integer used as NULL pointer warnings.

And even though in this case, I can't see the return type changing to
actually return something, coding it this way makes it obvious if you
ever missed a return site.

No strong feelings.

Harvey