[Devel] Re: [RFC][PATCH] Use task_pgrp()/task_session() in copy_process

[Dave Hansen]

On Thu, 2007-01-11 at 07:58 -0800, Sukadev Bhattiprolu wrote:
> ===================================================================
> --- lx26-20-rc2-mm1.orig/kernel/fork.c	2007-01-11 07:18:03.383853328 -0800
> +++ lx26-20-rc2-mm1/kernel/fork.c	2007-01-11 07:19:55.550801360 -0800
> @@ -1248,8 +1248,13 @@ static struct task_struct *copy_process(
>  			p->signal->tty = current->signal->tty;
>  			p->signal->pgrp = process_group(current);
>  			set_signal_session(p->signal, process_session(current));
> -			find_attach_pid(p, PIDTYPE_PGID, process_group(p));
> -			find_attach_pid(p, PIDTYPE_SID, process_session(p));
> +			if (current->pid) {
> +				attach_pid(p, PIDTYPE_PGID, task_pgrp(current));
> +				attach_pid(p, PIDTYPE_SID, task_session(current));
> +			} else {
> +				find_attach_pid(p, PIDTYPE_PGID, process_group(current));
> +				find_attach_pid(p, PIDTYPE_SID, process_session(current));
> +			}
>  
>  			list_add_tail_rcu(&p->tasks, &init_task.tasks);
>  			__get_cpu_var(process_counts)++;

I know I've asked this before (and I know I'm going to ask it again),
but why do we need both task_pgrp() and process_group() to both have
similar-sounding names and both take the same kind of argument? :)  This
stuff _really_ needs to get cleaned up.  It makes reviewing these
patches much harder.  

In general, you should keep the hacks (which this is) to boot and
init-time stuff.  If you can initialize a structure so that it plays
nicely for the rest of its life, do that.  Don't put special cases in
common code that everybody will have to look at.  

> Since task_pid() task_pgrp(), task_session() for the swapper are NULL, I
> had to treat swapper as special in this patch and would like some comments.

Can you do some research and find out _why_ these are NULL, and why they
need to be kept NULL?

-- Dave

_______________________________________________
Containers mailing list
Containers at lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers