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

[Cyrill Gorcunov]

On Tue, Jun 19, 2012 at 03:43:19PM +0400, Pavel Emelyanov wrote:
> > +
> > +		if (e.len) {
> > +			int ret = read(fd_pipes, local_buf, e.len);
> 
> read_img_buf
> 

Yeah, thanks!

> > +	u16	type;
> > +	u16	len;
> 
> namelen
> 

ok

> > +	if (type >= PIPE_TYPE_MAX) {
> > +		pr_err("Unknown pipe type %d on %d\n", type, p->fd);
> > +		return -1;
> > +	}
> 
> This check is not required.

Yeah, it should be rather BUG_ON, will drop it, thanks!

> > +	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).
> 

Yes, I remember you were pointing me that dumping path
is a way more complex and I should use dump_one_reg_file
approach. I managed to... somehow miss that :( Sorry.
Will update.

> >  		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.
> 

Yes, but it's good for first iteration. Don't get me wrong,
I simply need a trivial solution at first, then I'll speed
up this procedure significantly. I'll think about it.

> > +	/*
> > +	 * 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?
> 

It seems indeed it might be simplified. Thanks!

> > +
> > +	if (restore_pipe_data(where, info))
> 
> Thus you will push the same fifo data into each fifo's end. Why is this correct?

no, same data can't belong to two fifo ends at time, the data addressed by
pipe-ids and if there is no pipe-id found then restore_pipe_data simply
gets out without error code. Or you mean something else?

	Cyrill