[CRIU] Re: [PATCH cr 5/8] pid: restore pid namespace

[Pavel Emelyanov]

On 05/16/2012 12:32 PM, Andrey Vagin wrote:
> Each process has two pids
> real_pid - the pid of a process in crtools's pid namespace.
> pid - the pid of a process in its pid namespace.
>       This pid is get for help parasite and it should be saved after restore.

First of all this patch should be splitted.

Next -- I don't catch the overall idea of -- which pid is used in per-task
image files (e.g. mm-$pid.img one) real or virtual and which inside the
pstree.img one?

And what happened with the shmem reopen via proc problem?

Plus several comments inline.

> @@ -1372,13 +1374,10 @@ try_again:
>  	if (!ret)
>  		ret = check_xids(pstree_list);
>  
> -	if (ret)
> -		return ret;
> -
> -	return dump_pstree(pid, pstree_list);
> +	return ret;
>  }

Why do we need to split pstree collect and dump?

> @@ -1578,6 +1572,32 @@ static int dump_one_task(struct pstree_item *item)
>  		goto err;
>  	}
>  
> +	ret = parasite_dump_misc_seized(parasite_ctl, &misc);
> +	if (ret) {
> +		pr_err("Can't dump misc (pid: %d)\n", pid);
> +		goto err_cure_fdset;
> +	}
> +
> +	item->pid.pid = misc.pid;
> +	item->sid = misc.sid;
> +	item->pgid = misc.pgid;
> +
> +	if (item->parent) {
> +		int i;
> +		for (i = 0; i < item->parent->nr_children; i++)
> +			if (item->parent->children[i].real_pid == item->pid.real_pid) {
> +				item->parent->children[i].pid = item->pid.pid;
> +				break;
> +			}
> +
> +		BUG_ON(i == item->parent->nr_children);

Why is this parent's array step required?

> +	}
> +
> +	ret = -1;
> +	cr_fdset = cr_task_fdset_open(item->pid.pid, O_DUMP);
> +	if (!cr_fdset)
> +		goto err_cure;
> +
>  	ret = dump_task_files_seized(parasite_ctl, cr_fdset, fds, nr_fds);
>  	if (ret) {
>  		pr_err("Dump files (pid: %d) failed with %d\n", pid, ret);
> @@ -506,11 +506,14 @@ static inline int fork_with_pid(int pid, unsigned long ns_clone_flags)
>  		goto err_close;
>  	}
>  
> -	if (write_img_buf(ca.fd, buf, strlen(buf)))
> -		goto err_unlock;
> +	if (pid == 1) {

Need a comment here.

> +		ca.clone_flags |= CLONE_NEWPID;
> +	} else
> +		if (write_img_buf(ca.fd, buf, strlen(buf)))
> +			goto err_unlock;
>  
>  	ret = clone(restore_task_with_children, stack + STACK_SIZE,
> -			ns_clone_flags | SIGCHLD, &ca);
> +			ca.clone_flags | SIGCHLD, &ca);
>  
>  	if (ret < 0)
>  		pr_perror("Can't fork for %d", pid);