[CRIU] Re: [PATCH cr 1/2] crtools: link pstree_item-s in a tree (v2)

[Pavel Emelyanov]

> @@ -709,7 +697,10 @@ static int cr_show_all(struct cr_options *opts)
>  	}
>  
>  out:
> -	free_pstree(&pstree_list);
> +	list_for_each_entry(item, &pstree_list, list) {

list_for_each_entry_safe ?

> +		xfree(item->threads);
> +		xfree(item);
> +	}
>  	return ret;
>  }
>  

> @@ -707,11 +711,16 @@ static int restore_root_task(pid_t pid, struct cr_options *opts)
>  
>  out:
>  	if (ret < 0) {
> -		pr_err("Someone can't be restored\n");
>  		struct pstree_item *pi;
> +		pr_err("Someone can't be restored\n");
> +
> +		pi = root_item;
> +		while (pi) {
> +			if ((int) pi->pid > 0)

Why cast to int?
How can a pid be zero?

> +				kill(pi->pid, SIGKILL);
> +			pi = pstree_item_next(pi);
> +		}
>  
> -		list_for_each_entry(pi, &tasks, list)
> -			kill(pi->pid, SIGKILL);
>  		return 1;
>  	}
>  

> @@ -593,15 +611,16 @@ static void restore_pgid(void)
>  static int restore_task_with_children(void *_arg)
>  {
>  	struct cr_clone_arg *ca = _arg;
> +	struct pstree_item *child;
>  	pid_t pid;
> -	int ret, i;
> +	int ret;
>  	sigset_t blockmask;
>  
>  	close_safe(&ca->fd);
>  
>  	pid = getpid();
> -	if (ca->pid != pid) {
> -		pr_err("Pid %d do not match expected %d\n", pid, ca->pid);
> +	if (ca->item->pid != pid) {

Just a nitpick -- you can initialize me (next hunk) earlier and dereference it here.

> +		pr_err("Pid %d do not match expected %d\n", pid, ca->item->pid);
>  		exit(-1);
>  	}
>  
> @@ -609,14 +628,7 @@ static int restore_task_with_children(void *_arg)
>  	if (ret < 0)
>  		exit(1);
>  
> -	list_for_each_entry(me, &tasks, list)
> -		if (me->pid == pid)
> -			break;
> -
> -	if (me == list_entry(&tasks, struct pstree_item, list)) {
> -		pr_err("Pid %d not found in pstree image\n", pid);
> -		exit(1);
> -	}
> +	me = ca->item;
>  
>  	if (ca->clone_flags) {
>  		ret = prepare_namespace(me->pid, ca->clone_flags);

> @@ -178,7 +191,8 @@ static int prepare_shared(void)
>  	if (collect_inotify())
>  		return -1;
>  
> -	list_for_each_entry(pi, &tasks, list) {
> +	pi = root_item;
> +	while (pi) {
>  		ret = prepare_shmem_pid(pi->pid);
>  		if (ret < 0)
>  			break;
> @@ -186,6 +200,8 @@ static int prepare_shared(void)
>  		ret = prepare_fd_pid(pi->pid, pi->rst);
>  		if (ret < 0)
>  			break;
> +
> +		pi = pstree_item_next(pi);

It's perfectly worth having a

for_each_pstree_item(pi)

which is

for (pi = root_item; pi != NULL; pi = pstree_item_next(pi))

>  	}
>  
>  	mark_pipe_master();

> @@ -88,37 +89,50 @@ static int prepare_pstree(void)
>  
>  	while (1) {
>  		struct pstree_entry e;
> -		struct pstree_item *pi;
>  
>  		ret = read_img_eof(ps_fd, &e);
>  		if (ret <= 0)
>  			break;
>  
>  		ret = -1;
> -		pi = xmalloc(sizeof(*pi));
> +		pi = alloc_pstree_item();
>  		if (pi == NULL)
>  			break;
>  
>  		pi->rst = xzalloc(sizeof(*pi->rst));
> -		if (pi->rst == NULL)
> +		if (pi->rst == NULL) {
> +			xfree(pi);
>  			break;
> +		}
>  
>  		pi->pid = e.pid;
>  		pi->pgid = e.pgid;
>  		pi->sid = e.sid;
>  
> -		ret = -1;
> -		pi->nr_children = e.nr_children;
> -		pi->children = xmalloc(e.nr_children * sizeof(u32));
> -		if (!pi->children)
> -			break;
> +		if (e.ppid == 0) {
> +			BUG_ON(root_item);
> +			root_item = pi;
> +			pi->parent = NULL;
> +			INIT_LIST_HEAD(&pi->list);
> +		} else {
> +			struct pstree_item *item = root_item;

Taking into account the way the dump saves pstrees in the image, it's
worth having a fast path here -- cache the last seen parent locally and
don't search for the whole pstree for parent.

> +
> +			while (item) {
> +				if (item->pid == e.ppid)
> +					break;
> +				item = pstree_item_next(item);
> +			}
>  
> -		ret = read_img_buf(ps_fd, pi->children,
> -				e.nr_children * sizeof(u32));
> -		if (ret < 0)
> -			break;
> +			if (item == NULL) {
> +				pr_err("Can't find a parent for %d", pi->pid);
> +				xfree(pi);
> +				break;
> +			}
> +
> +			pi->parent = item;
> +			list_add(&pi->list, &item->children);
> +		}
>  
> -		ret = -1;
>  		pi->nr_threads = e.nr_threads;
>  		pi->threads = xmalloc(e.nr_threads * sizeof(u32));
>  		if (!pi->threads)

> @@ -180,16 +179,18 @@ struct pstree_item {
>  	struct list_head	list;

You put them in a tree and iterate this tree. Thus, why do we need a
separate plain list?

>  	pid_t			pid;		/* leader pid */
>  	struct pstree_item	*parent;
> +	struct list_head	children;	/* array of children */
>  	pid_t			pgid;
>  	pid_t			sid;
>  	int			state;		/* TASK_XXX constants */
> -	int			nr_children;	/* number of children */
>  	int			nr_threads;	/* number of threads */
>  	u32			*threads;	/* array of threads */
> -	u32			*children;	/* array of children */
>  	struct rst_info		*rst;
>  };
>  

> +struct pstree_item *alloc_pstree_item()
> +{
> +struct pstree_item *pstree_item_next(struct pstree_item *item)
>  {

Taking into account the fact you uses this in the restore also it's worth
moving the pstree items management into some generic .c file. E.g. util.c
or new pstree.c

Thanks,
Pavel