[PATCH] sys_setpgid: simplify pid/ns interaction

[PATCH] sys_setpgid: simplify pid/ns interaction

[Oleg Nesterov]

sys_setpgid() does unneeded conversions from pid_t to "struct pid" and vice
versa. Use "struct pid" more consistently. Saves one find_vpid() and eliminates
the explicit usage of ->nsproxy->pid_ns. Imho, cleanups the code.

Also use the same_thread_group() helper.

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- PT/kernel/sys.c~	2007-11-20 17:16:10.000000000 +0300
+++ PT/kernel/sys.c	2007-11-25 17:16:59.000000000 +0300
@@ -916,8 +916,8 @@ asmlinkage long sys_setpgid(pid_t pid, p
 {
 	struct task_struct *p;
 	struct task_struct *group_leader = current->group_leader;
-	int err = -EINVAL;
-	struct pid_namespace *ns;
+	struct pid *pgrp;
+	int err;
 
 	if (!pid)
 		pid = task_pid_vnr(group_leader);
@@ -929,12 +929,10 @@ asmlinkage long sys_setpgid(pid_t pid, p
 	/* From this point forward we keep holding onto the tasklist lock
 	 * so that our parent does not change from under us. -DaveM
 	 */
-	ns = current->nsproxy->pid_ns;
-
 	write_lock_irq(&tasklist_lock);
 
 	err = -ESRCH;
-	p = find_task_by_pid_ns(pid, ns);
+	p = find_task_by_vpid(pid);
 	if (!p)
 		goto out;
 
@@ -942,7 +940,7 @@ asmlinkage long sys_setpgid(pid_t pid, p
 	if (!thread_group_leader(p))
 		goto out;
 
-	if (p->real_parent->tgid == group_leader->tgid) {
+	if (same_thread_group(p->real_parent, group_leader)) {
 		err = -EPERM;
 		if (task_session(p) != task_session(group_leader))
 			goto out;
@@ -959,10 +957,12 @@ asmlinkage long sys_setpgid(pid_t pid, p
 	if (p->signal->leader)
 		goto out;
 
+	pgrp = task_pid(p);
 	if (pgid != pid) {
 		struct task_struct *g;
 
-		g = find_task_by_pid_type_ns(PIDTYPE_PGID, pgid, ns);
+		pgrp = find_vpid(pgid);
+		g = pid_task(pgrp, PIDTYPE_PGID);
 		if (!g || task_session(g) != task_session(group_leader))
 			goto out;
 	}
@@ -971,13 +971,10 @@ asmlinkage long sys_setpgid(pid_t pid, p
 	if (err)
 		goto out;
 
-	if (task_pgrp_nr_ns(p, ns) != pgid) {
-		struct pid *pid;
-
+	if (task_pgrp(p) != pgrp) {
 		detach_pid(p, PIDTYPE_PGID);
-		pid = find_vpid(pgid);
-		attach_pid(p, PIDTYPE_PGID, pid);
-		set_task_pgrp(p, pid_nr(pid));
+		attach_pid(p, PIDTYPE_PGID, pgrp);
+		set_task_pgrp(p, pid_nr(pgrp));
 	}
 
 	err = 0;

Re: [PATCH] sys_setpgid: simplify pid/ns interaction

[Pavel Emelyanov]

Oleg Nesterov wrote:
> sys_setpgid() does unneeded conversions from pid_t to "struct pid" and vice
> versa. Use "struct pid" more consistently. Saves one find_vpid() and eliminates
> the explicit usage of ->nsproxy->pid_ns. Imho, cleanups the code.
> 
> Also use the same_thread_group() helper.
> 
> Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

Looks good :)

Acked-by: Pavel Emelyanov <xemul@openvz.org>

> --- PT/kernel/sys.c~	2007-11-20 17:16:10.000000000 +0300
> +++ PT/kernel/sys.c	2007-11-25 17:16:59.000000000 +0300
> @@ -916,8 +916,8 @@ asmlinkage long sys_setpgid(pid_t pid, p
>  {
>  	struct task_struct *p;
>  	struct task_struct *group_leader = current->group_leader;
> -	int err = -EINVAL;
> -	struct pid_namespace *ns;
> +	struct pid *pgrp;
> +	int err;
>  
>  	if (!pid)
>  		pid = task_pid_vnr(group_leader);
> @@ -929,12 +929,10 @@ asmlinkage long sys_setpgid(pid_t pid, p
>  	/* From this point forward we keep holding onto the tasklist lock
>  	 * so that our parent does not change from under us. -DaveM
>  	 */
> -	ns = current->nsproxy->pid_ns;
> -
>  	write_lock_irq(&tasklist_lock);
>  
>  	err = -ESRCH;
> -	p = find_task_by_pid_ns(pid, ns);
> +	p = find_task_by_vpid(pid);
>  	if (!p)
>  		goto out;
>  
> @@ -942,7 +940,7 @@ asmlinkage long sys_setpgid(pid_t pid, p
>  	if (!thread_group_leader(p))
>  		goto out;
>  
> -	if (p->real_parent->tgid == group_leader->tgid) {
> +	if (same_thread_group(p->real_parent, group_leader)) {
>  		err = -EPERM;
>  		if (task_session(p) != task_session(group_leader))
>  			goto out;
> @@ -959,10 +957,12 @@ asmlinkage long sys_setpgid(pid_t pid, p
>  	if (p->signal->leader)
>  		goto out;
>  
> +	pgrp = task_pid(p);
>  	if (pgid != pid) {
>  		struct task_struct *g;
>  
> -		g = find_task_by_pid_type_ns(PIDTYPE_PGID, pgid, ns);
> +		pgrp = find_vpid(pgid);
> +		g = pid_task(pgrp, PIDTYPE_PGID);
>  		if (!g || task_session(g) != task_session(group_leader))
>  			goto out;
>  	}
> @@ -971,13 +971,10 @@ asmlinkage long sys_setpgid(pid_t pid, p
>  	if (err)
>  		goto out;
>  
> -	if (task_pgrp_nr_ns(p, ns) != pgid) {
> -		struct pid *pid;
> -
> +	if (task_pgrp(p) != pgrp) {
>  		detach_pid(p, PIDTYPE_PGID);
> -		pid = find_vpid(pgid);
> -		attach_pid(p, PIDTYPE_PGID, pid);
> -		set_task_pgrp(p, pid_nr(pid));
> +		attach_pid(p, PIDTYPE_PGID, pgrp);
> +		set_task_pgrp(p, pid_nr(pgrp));
>  	}
>  
>  	err = 0;
> 
>