* [PATCH 2/4] sys: remove kernel_shutdown_prepare's parameter
2013-05-30 7:58 [PATCH 1/4] sys: remove unnecesscary parameter of set_one_prio liguang
@ 2013-05-30 7:58 ` liguang
2013-05-31 22:58 ` Andrew Morton
2013-05-30 7:58 ` [PATCH 3/4] sys/reboot: boolize C_A_D liguang
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: liguang @ 2013-05-30 7:58 UTC (permalink / raw)
To: Paul E. McKenney, David Howells, Thomas Gleixner, Andrew Morton
Cc: Michael Kerrisk, Dave Jones, Kees Cook, Cyrill Gorcunov, Al Viro,
liguang, linux-kernel
kernel_shutdown_prepare's parameter can be removed
by checking global 'system_state', so, maybe we
can save a register to be used :-).
Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
---
kernel/sys.c | 11 ++++++-----
1 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/kernel/sys.c b/kernel/sys.c
index 07c6177..7f56ed4 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -382,11 +382,10 @@ void kernel_restart(char *cmd)
}
EXPORT_SYMBOL_GPL(kernel_restart);
-static void kernel_shutdown_prepare(enum system_states state)
+static void kernel_shutdown_prepare(void)
{
blocking_notifier_call_chain(&reboot_notifier_list,
- (state == SYSTEM_HALT)?SYS_HALT:SYS_POWER_OFF, NULL);
- system_state = state;
+ (system_state == SYSTEM_HALT)?SYS_HALT:SYS_POWER_OFF, NULL);
usermodehelper_disable();
device_shutdown();
}
@@ -397,7 +396,7 @@ static void kernel_shutdown_prepare(enum system_states state)
*/
void kernel_halt(void)
{
- kernel_shutdown_prepare(SYSTEM_HALT);
+ kernel_shutdown_prepare();
disable_nonboot_cpus();
syscore_shutdown();
printk(KERN_EMERG "System halted.\n");
@@ -414,7 +413,7 @@ EXPORT_SYMBOL_GPL(kernel_halt);
*/
void kernel_power_off(void)
{
- kernel_shutdown_prepare(SYSTEM_POWER_OFF);
+ kernel_shutdown_prepare();
if (pm_power_off_prepare)
pm_power_off_prepare();
disable_nonboot_cpus();
@@ -484,11 +483,13 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd,
break;
case LINUX_REBOOT_CMD_HALT:
+ system_state = SYSTEM_HALT;
kernel_halt();
do_exit(0);
panic("cannot halt");
case LINUX_REBOOT_CMD_POWER_OFF:
+ system_state = SYSTEM_POWER_OFF;
kernel_power_off();
do_exit(0);
break;
--
1.7.2.5
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 2/4] sys: remove kernel_shutdown_prepare's parameter
2013-05-30 7:58 ` [PATCH 2/4] sys: remove kernel_shutdown_prepare's parameter liguang
@ 2013-05-31 22:58 ` Andrew Morton
0 siblings, 0 replies; 11+ messages in thread
From: Andrew Morton @ 2013-05-31 22:58 UTC (permalink / raw)
To: liguang
Cc: Paul E. McKenney, David Howells, Thomas Gleixner,
Michael Kerrisk, Dave Jones, Kees Cook, Cyrill Gorcunov, Al Viro,
linux-kernel
On Thu, 30 May 2013 15:58:04 +0800 liguang <lig.fnst@cn.fujitsu.com> wrote:
> kernel_shutdown_prepare's parameter can be removed
> by checking global 'system_state', so, maybe we
> can save a register to be used :-).
>
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -382,11 +382,10 @@ void kernel_restart(char *cmd)
> }
> EXPORT_SYMBOL_GPL(kernel_restart);
>
> -static void kernel_shutdown_prepare(enum system_states state)
> +static void kernel_shutdown_prepare(void)
> {
> blocking_notifier_call_chain(&reboot_notifier_list,
> - (state == SYSTEM_HALT)?SYS_HALT:SYS_POWER_OFF, NULL);
> - system_state = state;
> + (system_state == SYSTEM_HALT)?SYS_HALT:SYS_POWER_OFF, NULL);
> usermodehelper_disable();
> device_shutdown();
> }
> @@ -397,7 +396,7 @@ static void kernel_shutdown_prepare(enum system_states state)
> */
> void kernel_halt(void)
> {
> - kernel_shutdown_prepare(SYSTEM_HALT);
> + kernel_shutdown_prepare();
> disable_nonboot_cpus();
> syscore_shutdown();
> printk(KERN_EMERG "System halted.\n");
> @@ -414,7 +413,7 @@ EXPORT_SYMBOL_GPL(kernel_halt);
> */
> void kernel_power_off(void)
> {
> - kernel_shutdown_prepare(SYSTEM_POWER_OFF);
> + kernel_shutdown_prepare();
> if (pm_power_off_prepare)
> pm_power_off_prepare();
> disable_nonboot_cpus();
> @@ -484,11 +483,13 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd,
> break;
>
> case LINUX_REBOOT_CMD_HALT:
> + system_state = SYSTEM_HALT;
> kernel_halt();
> do_exit(0);
> panic("cannot halt");
>
> case LINUX_REBOOT_CMD_POWER_OFF:
> + system_state = SYSTEM_POWER_OFF;
> kernel_power_off();
> do_exit(0);
> break;
No, this changes behavior for all other callers of kernel_halt() and
kernel_power_off(): system_state will no longer be updated for those
callers.
And passing an argument via a global variable is not nice :(
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/4] sys/reboot: boolize C_A_D
2013-05-30 7:58 [PATCH 1/4] sys: remove unnecesscary parameter of set_one_prio liguang
2013-05-30 7:58 ` [PATCH 2/4] sys: remove kernel_shutdown_prepare's parameter liguang
@ 2013-05-30 7:58 ` liguang
2013-05-31 23:02 ` Andrew Morton
2013-05-30 7:58 ` [PATCH 4/4] sys: fix malformed panic massage of reboot liguang
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: liguang @ 2013-05-30 7:58 UTC (permalink / raw)
To: Paul E. McKenney, David Howells, Thomas Gleixner, Andrew Morton
Cc: Michael Kerrisk, Dave Jones, Kees Cook, Cyrill Gorcunov, Al Viro,
liguang, linux-kernel
Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
---
include/linux/reboot.h | 2 +-
kernel/sys.c | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/linux/reboot.h b/include/linux/reboot.h
index 23b3630..a8c5e4c 100644
--- a/include/linux/reboot.h
+++ b/include/linux/reboot.h
@@ -35,7 +35,7 @@ extern void kernel_restart(char *cmd);
extern void kernel_halt(void);
extern void kernel_power_off(void);
-extern int C_A_D; /* for sysctl */
+extern bool C_A_D; /* for sysctl */
void ctrl_alt_del(void);
#define POWEROFF_CMD_PATH_LEN 256
diff --git a/kernel/sys.c b/kernel/sys.c
index 7f56ed4..6dabfc1 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -119,7 +119,7 @@ EXPORT_SYMBOL(fs_overflowgid);
* this indicates whether you can reboot with ctrl-alt-del: the default is yes
*/
-int C_A_D = 1;
+bool C_A_D = true;
struct pid *cad_pid;
EXPORT_SYMBOL(cad_pid);
@@ -475,11 +475,11 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd,
break;
case LINUX_REBOOT_CMD_CAD_ON:
- C_A_D = 1;
+ C_A_D = true;
break;
case LINUX_REBOOT_CMD_CAD_OFF:
- C_A_D = 0;
+ C_A_D = false;
break;
case LINUX_REBOOT_CMD_HALT:
--
1.7.2.5
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 3/4] sys/reboot: boolize C_A_D
2013-05-30 7:58 ` [PATCH 3/4] sys/reboot: boolize C_A_D liguang
@ 2013-05-31 23:02 ` Andrew Morton
2013-06-03 0:25 ` li guang
2013-06-03 1:09 ` Joe Perches
0 siblings, 2 replies; 11+ messages in thread
From: Andrew Morton @ 2013-05-31 23:02 UTC (permalink / raw)
To: liguang
Cc: Paul E. McKenney, David Howells, Thomas Gleixner,
Michael Kerrisk, Dave Jones, Kees Cook, Cyrill Gorcunov, Al Viro,
linux-kernel
On Thu, 30 May 2013 15:58:05 +0800 liguang <lig.fnst@cn.fujitsu.com> wrote:
> --- a/include/linux/reboot.h
> +++ b/include/linux/reboot.h
> @@ -35,7 +35,7 @@ extern void kernel_restart(char *cmd);
> extern void kernel_halt(void);
> extern void kernel_power_off(void);
>
> -extern int C_A_D; /* for sysctl */
> +extern bool C_A_D; /* for sysctl */
> void ctrl_alt_del(void);
This means that the pointer in kernel/sysctl.c:kern_table.data now
points at a bool but is declared to have size sizeof(int).
That happens to work with current gcc verions, but there's no rule
which states that sizeof(bool) must equal sizeof(int).
And I'm not sure that changing kern_table to use sizeof(bool) is really
worth all the bother.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] sys/reboot: boolize C_A_D
2013-05-31 23:02 ` Andrew Morton
@ 2013-06-03 0:25 ` li guang
2013-06-03 1:09 ` Joe Perches
1 sibling, 0 replies; 11+ messages in thread
From: li guang @ 2013-06-03 0:25 UTC (permalink / raw)
To: Andrew Morton
Cc: Paul E. McKenney, David Howells, Thomas Gleixner,
Michael Kerrisk, Dave Jones, Kees Cook, Cyrill Gorcunov, Al Viro,
linux-kernel
在 2013-05-31五的 16:02 -0700，Andrew Morton写道：
> On Thu, 30 May 2013 15:58:05 +0800 liguang <lig.fnst@cn.fujitsu.com> wrote:
>
> > --- a/include/linux/reboot.h
> > +++ b/include/linux/reboot.h
> > @@ -35,7 +35,7 @@ extern void kernel_restart(char *cmd);
> > extern void kernel_halt(void);
> > extern void kernel_power_off(void);
> >
> > -extern int C_A_D; /* for sysctl */
> > +extern bool C_A_D; /* for sysctl */
> > void ctrl_alt_del(void);
>
> This means that the pointer in kernel/sysctl.c:kern_table.data now
> points at a bool but is declared to have size sizeof(int).
>
> That happens to work with current gcc verions, but there's no rule
> which states that sizeof(bool) must equal sizeof(int).
>
> And I'm not sure that changing kern_table to use sizeof(bool) is really
> worth all the bother.
OK, got it,
Thanks!
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] sys/reboot: boolize C_A_D
2013-05-31 23:02 ` Andrew Morton
2013-06-03 0:25 ` li guang
@ 2013-06-03 1:09 ` Joe Perches
1 sibling, 0 replies; 11+ messages in thread
From: Joe Perches @ 2013-06-03 1:09 UTC (permalink / raw)
To: Andrew Morton
Cc: liguang, Paul E. McKenney, David Howells, Thomas Gleixner,
Michael Kerrisk, Dave Jones, Kees Cook, Cyrill Gorcunov, Al Viro,
linux-kernel
On Fri, 2013-05-31 at 16:02 -0700, Andrew Morton wrote:
> there's no rule
> which states that sizeof(bool) must equal sizeof(int).
For gcc, sizeof(_Bool) isn't sizeof(int) so it would
work for one endian but not the other.
gcc has sizeof(_Bool) == sizeof(unsigned char)
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 4/4] sys: fix malformed panic massage of reboot
2013-05-30 7:58 [PATCH 1/4] sys: remove unnecesscary parameter of set_one_prio liguang
2013-05-30 7:58 ` [PATCH 2/4] sys: remove kernel_shutdown_prepare's parameter liguang
2013-05-30 7:58 ` [PATCH 3/4] sys/reboot: boolize C_A_D liguang
@ 2013-05-30 7:58 ` liguang
2013-05-30 8:08 ` [PATCH 1/4] sys: remove unnecesscary parameter of set_one_prio Joe Perches
2013-05-31 22:53 ` Andrew Morton
4 siblings, 0 replies; 11+ messages in thread
From: liguang @ 2013-05-30 7:58 UTC (permalink / raw)
To: Paul E. McKenney, David Howells, Thomas Gleixner, Andrew Morton
Cc: Michael Kerrisk, Dave Jones, Kees Cook, Cyrill Gorcunov, Al Viro,
liguang, linux-kernel
if LINUX_REBOOT_CMD_HALT for reboot failed,
message "cannot halt" will stay in same line
with next message, so append a '\n' for it.
Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
---
kernel/sys.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/kernel/sys.c b/kernel/sys.c
index 6dabfc1..c04c611 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -486,7 +486,7 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd,
system_state = SYSTEM_HALT;
kernel_halt();
do_exit(0);
- panic("cannot halt");
+ panic("cannot halt.\n");
case LINUX_REBOOT_CMD_POWER_OFF:
system_state = SYSTEM_POWER_OFF;
--
1.7.2.5
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/4] sys: remove unnecesscary parameter of set_one_prio
2013-05-30 7:58 [PATCH 1/4] sys: remove unnecesscary parameter of set_one_prio liguang
` (2 preceding siblings ...)
2013-05-30 7:58 ` [PATCH 4/4] sys: fix malformed panic massage of reboot liguang
@ 2013-05-30 8:08 ` Joe Perches
2013-05-31 23:07 ` Andrew Morton
2013-05-31 22:53 ` Andrew Morton
4 siblings, 1 reply; 11+ messages in thread
From: Joe Perches @ 2013-05-30 8:08 UTC (permalink / raw)
To: liguang
Cc: Paul E. McKenney, David Howells, Thomas Gleixner, Andrew Morton,
Michael Kerrisk, Dave Jones, Kees Cook, Cyrill Gorcunov, Al Viro,
linux-kernel
On Thu, 2013-05-30 at 15:58 +0800, liguang wrote:
[]
> diff --git a/kernel/sys.c b/kernel/sys.c
[]
> -static int set_one_prio(struct task_struct *p, int niceval, int error)
> +static int set_one_prio(struct task_struct *p, int niceval)
Umm, error is forwarded through do_each loops.
Are you sure you can do this without
changing any return code behaviors?
[]
> @@ -211,7 +209,7 @@ SYSCALL_DEFINE3(setpriority, int, which, int, who, int, niceval)
> else
> pgrp = task_pgrp(current);
> do_each_pid_thread(pgrp, PIDTYPE_PGID, p) {
> - error = set_one_prio(p, niceval, error);
> + error = set_one_prio(p, niceval);
> } while_each_pid_thread(pgrp, PIDTYPE_PGID, p);
> break;
> case PRIO_USER:
> @@ -225,7 +223,7 @@ SYSCALL_DEFINE3(setpriority, int, which, int, who, int, niceval)
>
> do_each_thread(g, p) {
> if (uid_eq(task_uid(p), uid))
> - error = set_one_prio(p, niceval, error);
> + error = set_one_prio(p, niceval);
> } while_each_thread(g, p);
> if (!uid_eq(uid, cred->uid))
> free_uid(user); /* For find_user() */
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/4] sys: remove unnecesscary parameter of set_one_prio
2013-05-30 8:08 ` [PATCH 1/4] sys: remove unnecesscary parameter of set_one_prio Joe Perches
@ 2013-05-31 23:07 ` Andrew Morton
0 siblings, 0 replies; 11+ messages in thread
From: Andrew Morton @ 2013-05-31 23:07 UTC (permalink / raw)
To: Joe Perches
Cc: liguang, Paul E. McKenney, David Howells, Thomas Gleixner,
Michael Kerrisk, Dave Jones, Kees Cook, Cyrill Gorcunov, Al Viro,
linux-kernel
On Thu, 30 May 2013 01:08:25 -0700 Joe Perches <joe@perches.com> wrote:
> On Thu, 2013-05-30 at 15:58 +0800, liguang wrote:
> []
> > diff --git a/kernel/sys.c b/kernel/sys.c
> []
> > -static int set_one_prio(struct task_struct *p, int niceval, int error)
> > +static int set_one_prio(struct task_struct *p, int niceval)
>
> Umm, error is forwarded through do_each loops.
>
> Are you sure you can do this without
> changing any return code behaviors?
yes, you're right. The first time around, -ESRCH gets rewritten to 0
and the zero propagates, as long as nobody hits an error which isn't
-ESRCH. I'm not sure this was the most straightforward possible
implementation :(
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] sys: remove unnecesscary parameter of set_one_prio
2013-05-30 7:58 [PATCH 1/4] sys: remove unnecesscary parameter of set_one_prio liguang
` (3 preceding siblings ...)
2013-05-30 8:08 ` [PATCH 1/4] sys: remove unnecesscary parameter of set_one_prio Joe Perches
@ 2013-05-31 22:53 ` Andrew Morton
4 siblings, 0 replies; 11+ messages in thread
From: Andrew Morton @ 2013-05-31 22:53 UTC (permalink / raw)
To: liguang
Cc: Paul E. McKenney, David Howells, Thomas Gleixner,
Michael Kerrisk, Dave Jones, Kees Cook, Cyrill Gorcunov, Al Viro,
linux-kernel
On Thu, 30 May 2013 15:58:03 +0800 liguang <lig.fnst@cn.fujitsu.com> wrote:
> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> ---
> kernel/sys.c | 12 +++++-------
> 1 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/sys.c b/kernel/sys.c
> index b95d3c7..07c6177 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -151,9 +151,9 @@ static bool set_one_prio_perm(struct task_struct *p)
> * set the priority of a task
> * - the caller must hold the RCU read lock
> */
> -static int set_one_prio(struct task_struct *p, int niceval, int error)
> +static int set_one_prio(struct task_struct *p, int niceval)
> {
> - int no_nice;
> + int no_nice, error = 0;
>
> if (!set_one_prio_perm(p)) {
> error = -EPERM;
> @@ -168,8 +168,6 @@ static int set_one_prio(struct task_struct *p, int niceval, int error)
> error = no_nice;
> goto out;
> }
> - if (error == -ESRCH)
> - error = 0;
> set_user_nice(p, niceval);
> out:
> return error;
> @@ -203,7 +201,7 @@ SYSCALL_DEFINE3(setpriority, int, which, int, who, int, niceval)
> else
> p = current;
> if (p)
> - error = set_one_prio(p, niceval, error);
> + error = set_one_prio(p, niceval);
> break;
> case PRIO_PGRP:
> if (who)
> @@ -211,7 +209,7 @@ SYSCALL_DEFINE3(setpriority, int, which, int, who, int, niceval)
> else
> pgrp = task_pgrp(current);
> do_each_pid_thread(pgrp, PIDTYPE_PGID, p) {
> - error = set_one_prio(p, niceval, error);
> + error = set_one_prio(p, niceval);
> } while_each_pid_thread(pgrp, PIDTYPE_PGID, p);
> break;
> case PRIO_USER:
> @@ -225,7 +223,7 @@ SYSCALL_DEFINE3(setpriority, int, which, int, who, int, niceval)
>
> do_each_thread(g, p) {
> if (uid_eq(task_uid(p), uid))
> - error = set_one_prio(p, niceval, error);
> + error = set_one_prio(p, niceval);
> } while_each_thread(g, p);
> if (!uid_eq(uid, cred->uid))
> free_uid(user); /* For find_user() */
Yes, that apepars to be an evuivalent change, but only because
security_task_setnice() cannot return -ESRCH. The existing code is
rather awkward.
A couple of changes:
From: Andrew Morton <akpm@linux-foundation.org>
Subject: kernel-sysc-remove-unnecessary-parameter-of-set_one_prio-fix
clean up definitions, remove unneeded assignment
Cc: liguang <lig.fnst@cn.fujitsu.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
kernel/sys.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff -puN kernel/sys.c~kernel-sysc-remove-unnecessary-parameter-of-set_one_prio-fix kernel/sys.c
--- a/kernel/sys.c~kernel-sysc-remove-unnecessary-parameter-of-set_one_prio-fix
+++ a/kernel/sys.c
@@ -153,7 +153,8 @@ static bool set_one_prio_perm(struct tas
*/
static int set_one_prio(struct task_struct *p, int niceval)
{
- int no_nice, error = 0;
+ int no_nice;
+ int error = 0;
if (!set_one_prio_perm(p)) {
error = -EPERM;
@@ -186,7 +187,6 @@ SYSCALL_DEFINE3(setpriority, int, which,
goto out;
/* normalize: avoid signed division (rounding problems) */
- error = -ESRCH;
if (niceval < -20)
niceval = -20;
if (niceval > 19)
_
^ permalink raw reply [flat|nested] 11+ messages in thread