[CRIU] Re: [PATCH 3/6] pstree: Allow to dump and restore session non-leaders if --shell-job passed

[Cyrill Gorcunov]

On Wed, Oct 17, 2012 at 02:20:20PM +0400, Pavel Emelyanov wrote:
> > +		if (item->pgid == origin_sid)
> > +			e.pgid = INHERIT_SID;
> > +		else
> > +			e.pgid = item->pgid;
> > +
> > +		e.sid		= (item->sid == origin_sid) ? INHERIT_SID : item->sid;
> 
> Write in the same style.

OK

> > +static int try_inherit_sid(pid_t sid, PstreeEntry *e)
> > +{
> > +	bool modified = false;
> > +
> > +	if (e->sid == INHERIT_SID) {
> > +		e->sid = sid;
> > +		modified = true;
> > +	} else if (e->pgid == INHERIT_SID) {
> 
> Why else? And why ... <see below>

yup, typo

> > +		/*
> > +		 * Inherit SIDs/PGIDs early before building the process tree.
> > +		 */
> > +		ret = try_inherit_sid(current_sid, e);
> > +		if (ret)
> > +			goto err;
> > +
> > +		ret = -1;
> > +
> >  		pi->pid.virt = e->pid;
> >  		max_pid = max((int)e->pid, max_pid);
> >  
> > +		/*
> > +		 * Don't forget to update descending GIDs if the
> > +		 * root task's GID has been altered.
> > +		 */
> > +		if (e->pgid == old_gid)
> > +			e->pgid = current_gid;
> 
> <continued> ... is this check here, but not in the try_inherit_sid()?

old_gid updated once when we update root-task (see below) thus I can't
pass it to try_inherit_sid, otherwise I'll have to define some per-file
variable.

> >  			root_item = pi;
> >  			pi->parent = NULL;
> > +			if (pi->pid.virt == pi->pgid &&
> > +			    pi->pid.virt != pi->sid) {
> > +				pr_info("Migrating GID of a root process %d -> %d \n",
> > +					pi->pgid, current_gid);
> > +				if (!opts.shell_job) {
> > +					pr_err("Found GID %d to be inherited but "
> > +					       "no option passed\n", e->pgid);
> > +					goto err;
> 
> 
> 
> > +				}
> > +				old_gid = pi->pgid;

here we update old_gid

> > +		/*
> > +		 * If the PGID is eq to current one -- this
> > +		 * means we're inheriting group from the current
> > +		 * task so we need to escape creating a helper here.
> > +		 */
> > +		if (current_pgid == item->pgid)
> > +			continue;
> 
> Isn't it handled by gleader check above? Why?

The gleader only present if we have group leader in our tree. But
for tty sake we inherit gid from crtools thus the current code will
try creat group leader helper task which we don't need here.

	Cyrill