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

[Sukadev Bhattiprolu]

Dave Hansen [haveblue at us.ibm.com] wrote:
| 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.

We are phasing out process_group(), process_session() which return a
pid_t. I guess it also points to not having a special case for swapper.

| 
| 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?


task_struct for swapper is initialized by hand (INIT_TASK, INIT_SIGNALS
etc) but no struct pid is ever allocated and attached to the swapper.
This is normally done in copy_process() and so is done for all other
processes starting with pid_t = 1 (/sbin/init).

I am trying to understand if there is a history to it and if they need to
be kept NULL. 

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