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

[Pavel Emelyanov]

On 10/17/2012 12:32 AM, Cyrill Gorcunov wrote:
> If --shell-job passed we allow to dump and restpre session non-leaders.
> 
> This slightly changes the behaviour or the tool. If previously
> we allowed to dump non-leaders after this commit we refuse to
> do that if not explicitly requested.
> 
> Note that in sake of tty restore (which will be addressed in
> further patches) we do inherit process group for root task.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov at openvz.org>
> ---
>  include/pstree.h |    9 ++++
>  pstree.c         |  136 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 143 insertions(+), 2 deletions(-)
> 
> 
> 0003-pstree-Allow-to-dump-and-restore-session-non-leaders.patch
> 
> 
> diff --git a/include/pstree.h b/include/pstree.h
> index 8536844..0c8c9bb 100644
> --- a/include/pstree.h
> +++ b/include/pstree.h
> @@ -9,6 +9,15 @@
>   */
>  #define INIT_PID	(1)
>  
> +/*
> + * When we need to inherit SIDs/PGIDs we put a
> + * special value in dump image.
> + */
> +#define INHERIT_SID	(-1)
> +
> +/* Some known to be invalid value */
> +#define INVALID_PID	(-2)
> +
>  struct pid {
>  	/*
>  	 * The @real pid is used to fetch tasks during dumping stage,
> diff --git a/pstree.c b/pstree.c
> index 6acf492..80c4c83 100644
> --- a/pstree.c
> +++ b/pstree.c
> @@ -69,10 +69,31 @@ int dump_pstree(struct pstree_item *root_item)
>  	int ret = -1, i;
>  	int pstree_fd;
>  
> +	pid_t origin_sid = INHERIT_SID;
> +
>  	pr_info("\n");
>  	pr_info("Dumping pstree (pid: %d)\n", root_item->pid.real);
>  	pr_info("----------------------------------------\n");
>  
> +	/*
> +	 * Make sure we're dumping session leader, if not an
> +	 * appropriate option must be passed.
> +	 *
> +	 * Also note that if we're not a session leader we
> +	 * can't get the situation where the leader sits somewhere
> +	 * deeper in process tree, thus top-level checking for
> +	 * leader is enough.
> +	 */
> +	if (root_item->pid.virt != root_item->sid) {
> +		if (!opts.shell_job) {
> +			pr_err("The root process %d is not a session leader,"
> +			       "miss option?\n", item->pid.virt);
> +			return -1;
> +		}
> +
> +		origin_sid  = root_item->sid;
> +	}
> +
>  	pstree_fd = open_image(CR_FD_PSTREE, O_DUMP);
>  	if (pstree_fd < 0)
>  		return -1;
> @@ -82,8 +103,37 @@ int dump_pstree(struct pstree_item *root_item)
>  
>  		e.pid		= item->pid.virt;
>  		e.ppid		= item->parent ? item->parent->pid.virt : 0;
> -		e.pgid		= item->pgid;
> -		e.sid		= item->sid;
> +
> +		/*
> +		 * When we're walking over all processes in the tree we
> +		 * can meet two kind of processes -- session leaders and
> +		 * group leaders, and they must remain the same on restore
> +		 * (except root item, which might inherit SID/PGID).
> +		 *
> +		 * While for SIDs iheritance it looks obvious, it's not
> +		 * same clear for GIDs. Here is an example.
> +		 *
> +		 *   PID  GID  SID
> +		 *    2    2    2    shell
> +		 *    3    2    2    `- root-task <-- dump from here
> +		 *    5    2    2      `- task-1
> +		 *    6    6    2      `- task-2
> +		 *
> +		 *   PID  GID  SID
> +		 *    1    1    1    shell
> +		 *    3    1    1    `- root-task <-- restore from here
> +		 *    5    1    1      `- task-1
> +		 *    6    6    1      `- task-2
> +		 *
> +		 * So when we migrate a shell job we need the task-1 to inherit
> +		 * GID from root task SID, thus we put INHERIT_SID in the image.
> +		 */
> +		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.

>  		e.n_threads	= item->nr_threads;
>  
>  		e.threads = xmalloc(sizeof(e.threads[0]) * e.n_threads);
> @@ -107,12 +157,39 @@ err:
>  	return ret;
>  }
>  
> +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>

> +		e->pgid = sid;
> +		modified = true;
> +	}
> +
> +	if (modified) {
> +		if (!opts.shell_job) {
> +			pr_err("Found PID %d to be inherited but "
> +			       "no option passed\n", e->pid);
> +			return -1;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int max_pid = 0;
>  int prepare_pstree(void)
>  {
>  	int ret = 0, i, ps_fd;
>  	struct pstree_item *pi, *parent = NULL;
>  
> +	pid_t current_sid = getsid(getpid());
> +	pid_t current_gid = getpgid(getpid());
> +	pid_t old_gid = INVALID_PID;
> +
>  	pr_info("Reading image tree\n");
>  
>  	ps_fd = open_image_ro(CR_FD_PSTREE);
> @@ -131,9 +208,25 @@ int prepare_pstree(void)
>  		if (pi == NULL)
>  			break;
>  
> +		/*
> +		 * 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()?

> +
>  		pi->pgid = e->pgid;
>  		max_pid = max((int)e->pgid, max_pid);
>  
> @@ -148,6 +241,35 @@ int prepare_pstree(void)
>  			}
>  			root_item = pi;
>  			pi->parent = NULL;
> +
> +			/*
> +			 * Migration of a root task group leader is a bit tricky.
> +			 * When a task yields SIGSTOP, the kernel notifies the parent
> +			 * with SIGCHLD. This means when task is running in a
> +			 * shell, the shell obtains SIGCHLD and sends a task to
> +			 * the background.
> +			 *
> +			 * The situation gets changed once we restore the
> +			 * program -- our tool become an additional stub between
> +			 * the restored program and the shell. So to be able to
> +			 * notify the shell with SIGCHLD from our restored
> +			 * program -- we make the root task to inherit the
> +			 * process group from us.
> +			 *
> +			 * Not that clever solution but at least it works.
> +			 */
> +			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;
> +				pi->pgid = current_gid;
> +			}
>  		} else {
>  			/*
>  			 * Fast path -- if the pstree image is not edited, the
> @@ -205,6 +327,8 @@ int prepare_pstree_ids(void)
>  	struct pstree_item *item, *child, *helper, *tmp;
>  	LIST_HEAD(helpers);
>  
> +	pid_t current_pgid = getpgid(getpid());
> +
>  	/*
>  	 * Some task can be reparented to init. A helper task should be added
>  	 * for restoring sid of such tasks. The helper tasks will be exited
> @@ -331,6 +455,14 @@ int prepare_pstree_ids(void)
>  		if (gleader)
>  			continue;
>  
> +		/*
> +		 * 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?

> +
>  		helper = alloc_pstree_item();
>  		if (helper == NULL)
>  			return -1;
>