[CRIU] Re: [PATCH cr 05/10] restore: restore sids of tasks, which have been reparented to init (v2)

[Pavel Emelyanov]

On 06/19/2012 04:46 PM, Andrey Vagin wrote:
> v2: fix variables names and check errors

A comment describing the concept is required.

> Signed-off-by: Andrey Vagin <avagin at openvz.org>
> ---
>  cr-restore.c |   84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 84 insertions(+), 0 deletions(-)
> 
> diff --git a/cr-restore.c b/cr-restore.c
> index 9ba4d4d..1f0e916 100644
> --- a/cr-restore.c
> +++ b/cr-restore.c
> @@ -172,6 +172,87 @@ static int prepare_pstree(void)
>  	return ret;
>  }
>  
> +static int prepare_pstree_ids(void)
> +{
> +	struct pstree_item *item, *child, *helper, *tmp;
> +	LIST_HEAD(helpers);
> +
> +	/* 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
> +	 * immediately after forking children and all children will be
> +	 * reparented to init. */

OK, this comment explains what the first loop does.

> +	list_for_each_entry(item, &root_item->children, list) {
> +		if (item->sid == root_item->sid || item->sid == item->pid)
> +			continue;
> +
> +		helper = alloc_pstree_item();
> +		if (helper == NULL)
> +			return -1;
> +		helper->sid = item->sid;
> +		helper->pgid = item->sid;
> +		helper->pid = item->sid;
> +		helper->state = TASK_HELPER;
> +		helper->parent = root_item;
> +		list_add_tail(&helper->list, &helpers);
> +
> +		pr_info("Add a helper %d for restoring SID %d\n",
> +				helper->pid, helper->sid);
> +
> +		child = list_entry(item->list.prev, struct pstree_item, list);
> +		item = child;
> +
> +		list_for_each_entry_safe_continue(child, tmp, &root_item->children, list) {
> +			if (child->sid != helper->sid)
> +				continue;
> +			if (child->sid == child->pid)
> +				continue;
> +
> +			pr_info("Attach %d to the temporary task %d\n",
> +							child->pid, helper->pid);
> +
> +			child->parent = helper;
> +			list_move(&child->list, &helper->children);
> +		}
> +	}
> +

How about the 2nd loop -- what is it for?

> +	for_each_pstree_item(item) {
> +		if (!item->parent) /* skip the root task */
> +			continue;
> +
> +		/* Try to find a session leader */
> +		if (item->state == TASK_HELPER)
> +			continue;
> +
> +		if (item->sid != item->pid)
> +			continue;
> +
> +		pr_info("Session leader %d\n", item->sid);
> +
> +		/* Try to find helpers, who should be connected to the leader */
> +		list_for_each_entry(child, &helpers, list) {
> +			if (child->state != TASK_HELPER)
> +				continue;
> +
> +			if (child->sid != item->sid)
> +				continue;
> +
> +			child->pgid = item->pgid;
> +			child->pid = ++max_pid;
> +			child->parent = item;
> +			list_move(&child->list, &item->children);
> +
> +			pr_info("Attach %d to the task %d\n",
> +					child->pid, item->pid);
> +
> +			break;
> +		}
> +	}
> +
> +	list_splice(&helpers, &root_item->children);
> +

And the general comment for the whole fn -- we'll need helper tasks for shared
mm/fs/etc. in the future. Try to create an API call for "add a helper task for"
and use it.

> +	return 0;
> +}
> +
>  static int prepare_shared(void)
>  {
>  	int ret = 0;
> @@ -842,6 +923,9 @@ static int restore_all_tasks(pid_t pid, struct cr_options *opts)
>  	if (prepare_shared() < 0)
>  		return -1;
>  
> +	if (prepare_pstree_ids() < 0)
> +		return -1;
> +
>  	return restore_root_task(root_item, opts);
>  }
>