[CRIU] Re: [PATCH 19/26] pipes: Add c/r for fifo files

[Pavel Emelyanov]

On 06/19/2012 12:30 PM, Cyrill Gorcunov wrote:
> 
> Basically pipes are superset over fifos,
> so we integrate fifo c/r into pipes.c.
> 
> There os acatually a trick used to open fifo files --
> since fifo may block on opening we create a fake fifo
> to be able to proceed without sleeping.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov at openvz.org>
> ---
>  cr-dump.c       |    8 ++-
>  cr-show.c       |   15 ++++-
>  include/image.h |   10 +++
>  include/pipes.h |    3 +
>  pipes.c         |  193 ++++++++++++++++++++++++++++++++++++++++++++++++-------
>  5 files changed, 202 insertions(+), 27 deletions(-)
> 

> @@ -206,9 +207,19 @@ void show_pipes(int fd_pipes, struct cr_options *o)
>  		ret = read_img_eof(fd_pipes, &e);
>  		if (ret <= 0)
>  			goto out;
> -		pr_msg("id: 0x%8x pipeid: 0x%8x flags: 0x%8x ",
> -		       e.id, e.pipe_id, e.flags);
> +		pr_msg("id: 0x%8x pipeid: 0x%8x type: %4s flags: 0x%8x ",
> +		       e.id, e.pipe_id, pipetype2s(e.type), e.flags);
>  		show_fown_cont(&e.fown);
> +
> +		if (e.len) {
> +			int ret = read(fd_pipes, local_buf, e.len);

read_img_buf

> +			if (ret != e.len) {
> +				pr_perror("Can't read %d bytes", e.len);
> +				goto out;
> +			}
> +			local_buf[e.len] = 0;
> +			pr_msg(" --> %s", local_buf);
> +		}
>  		pr_msg("\n");
>  	}
>  

>  struct pipe_entry {
>  	u32	id;
>  	u32	pipe_id;
>  	u32	flags;
>  	fown_t	fown;
> +	u16	type;
> +	u16	len;

namelen

> +	u8	name[0];
>  } __packed;

> -static int dump_one_pipe(int lfd, u32 id, const struct fd_parms *p)
> +static int dump_one_typed_pipe(int type, int lfd, u32 id, const struct fd_parms *p)
>  {
>  	struct pipe_entry pe;
> +	char *path;
>  
> -	pr_info("Dumping pipe %d with id %#x pipe_id %#x\n", lfd, id, p->id);
> +	if (type >= PIPE_TYPE_MAX) {
> +		pr_err("Unknown pipe type %d on %d\n", type, p->fd);
> +		return -1;
> +	}

This check is not required.

> +
> +	pr_info("Dumping %s %d with id %#x pipe_id %#x\n",
> +		pipetype2s(type), lfd, id, p->id);
>  
>  	pe.id		= id;
>  	pe.pipe_id	= p->id;
>  	pe.flags	= p->flags;
>  	pe.fown		= p->fown;
> +	pe.type		= type;
>  
> -	if (write_img(fdset_fd(glob_fdset, CR_FD_PIPES), &pe))
> -		return -1;
> +	switch (type) {
> +	case PIPE_TYPE_ANON:
> +		pe.len = 0;
> +		if (write_img(fdset_fd(glob_fdset, CR_FD_PIPES), &pe))
> +			return -1;
> +		break;
> +	case PIPE_TYPE_FIFO:
> +		path = read_proc_selffd_link(lfd, (int *)&pe.len);
> +		if (!path)
> +			return -1;
> +		if (write_img(fdset_fd(glob_fdset, CR_FD_PIPES), &pe))
> +			return -1;
> +		if (write_img_buf(fdset_fd(glob_fdset, CR_FD_PIPES), path, pe.len))
> +			return -1;

This is wrong. When dumping a file with a path there's _much_ more should be
done rather than reading its path from the proc fd link. Reuse code from
dump_one_reg_file (I'd prefer if you just fix this function and call it).

> +		break;
> +	default:
> +		BUG_ON(1);
> +	}
>  
>  	return dump_one_pipe_data(lfd, id, p);
>  }

> @@ -273,18 +354,38 @@ int collect_pipes(void)
>  		if (pi == NULL)
>  			break;
>  
> +		pi->path = NULL;
> +
>  		ret = read_img_eof(fd, &pi->pe);
>  		if (ret <= 0)
>  			break;
>  
> -		pr_info("Collected pipe entry ID %#x PIPE ID %#x\n",
> -					pi->pe.id, pi->pe.pipe_id);
> +		if (pi->pe.len) {
> +			pi->path = xmalloc(pi->pe.len + 1);
> +			ret = read_img_buf_eof(fd, pi->path, pi->pe.len);
> +			if (ret <= 0) {
> +				ret = -1;
> +				pr_err("Corrupted path in %s %#x\n",
> +				       pipetype2s(pi->pe.type), pi->pe.id);
> +				break;
> +			}
> +			pi->path[pi->pe.len] = '\0';
> +		}
> +
> +		pr_info("Collected %s entry ID %#x PIPE ID %#x\n",
> +			pipetype2s(pi->pe.type), pi->pe.id, pi->pe.pipe_id);
>  
>  		file_desc_add(&pi->d, pi->pe.id, &pipe_desc_ops);
>  
>  		list_for_each_entry(tmp, &pipes, list) {
> -			if (pi->pe.pipe_id == tmp->pe.pipe_id)
> -				break;
> +			/*
> +			 * Only anon pipes are created in one place
> +			 * and sent via SCM, the fifo should be
> +			 * created explicitly.
> +			 */
> +			if (pi->pe.pipe_id == tmp->pe.pipe_id &&
> +			    pi->pe.type == PIPE_TYPE_ANON)

Huh? This will result in a waste of cpu cycles for PIPE_TYPE_FIFO.

> +					break;
>  		}
>  
>  		if (&tmp->list == &pipes)

> +static int open_fifo_fd(struct pipe_info *info, char *path)
> +{
> +	int new_fifo, fake_fifo = -1, where;
> +
> +	/*
> +	 * The fifos (except read-write fifos) do wait until
> +	 * another pipe-end get connected, so to be able to
> +	 * proceed the restoration procedure we open a fake
> +	 * fifo here.
> +	 */
> +	fake_fifo = open(path, O_RDWR);
> +	if (fake_fifo < 0) {
> +		pr_perror("Can't open fake reader on fifo %#08x [%s]",
> +			  info->pe.id, path);
> +		return -1;
> +	}
> +	pr_info("Fake fifo %d created\n", fake_fifo);
> +
> +	new_fifo = open(path, info->pe.flags);
> +	if (new_fifo < 0) {
> +		pr_perror("Can't open fifo %#08x [%s]",
> +			  info->pe.id, path);
> +		goto err_close_fake;
> +	}
> +
> +	/*
> +	 * If this is ro fifo then we need to restore data
> +	 * via fake fifo end.
> +	 */
> +	where = (info->pe.flags & O_ACCMODE) == O_RDONLY ? fake_fifo : new_fifo;

Why not where = fake_fifo always?

> +
> +	if (restore_pipe_data(where, info))

Thus you will push the same fifo data into each fifo's end. Why is this correct?

> +		goto err_close_new;
> +
> +	if (restore_fown(new_fifo, &info->pe.fown) < 0) {
> +		pr_perror("Can't restore owner on fifo %#08x",
> +			  info->pe.id);
> +		goto err_close_new;
> +	}
> +
> +err_close_fake:
> +	close_safe(&fake_fifo);
> +	return new_fifo;
> +
> +err_close_new:
> +	close_safe(&new_fifo);
> +	goto err_close_fake;
> +}
> +