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

[Andrew Vagin]

On Wed, May 16, 2012 at 03:48:18PM +0400, Pavel Emelyanov wrote:
> 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?

real pid isn't saved in image files, because we don't need to restore
it. A dump file name contains virtual pid too.
> 
> And what happened with the shmem reopen via proc problem?
proc will be mounted in /tmp/crtools-proc.XXXXXX
> 
> 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?
Because we need to call parasite to get a virtual pid, so
dump_pstree is called after dump_task
> 
> > @@ -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?

I'm going to rework this part.

> 
> > +	}
> > +
> > +	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);