[CRIU] Re: [PATCH 4/4] tty: Add checkpoint/restore for unix terminals

[Cyrill Gorcunov]

On Thu, Sep 06, 2012 at 08:03:45PM +0400, Pavel Emelyanov wrote:
> On 09/05/2012 09:21 PM, Cyrill Gorcunov wrote:
> > On Wed, Sep 05, 2012 at 08:44:26PM +0400, Pavel Emelyanov wrote:
> >>> @@ -242,19 +244,17 @@ static int dump_unsupp_fd(const struct fd_parms *p)
> >>>  	return -1;
> >>>  }
> >>>  
> >>> -static int dump_chrdev(struct fd_parms *p, int lfd, const struct cr_fdset *set)
> >>> +static int dump_chrdev(pid_t pid, struct fd_parms *p, int lfd, const struct cr_fdset *set)
> >>>  {
> >>>  	int maj = major(p->stat.st_rdev);
> >>>  
> >>
> >> What for?
> > 
> > I somehow confused here. 
> 
> What is the new pid argument for?

Ah, that. Indeed it's leftover from previous versions. Thanks!

> >> Huh?
> > 
> > Huh, what? ;)
> 
> No garbage in patches please.

Yeah, sorry.

> > no, char * will require me to allocate a separate slab of
> > memory for that. If you think it's good idea, i'll update.
> 
> Please.

ok, no problem, will do that.

> >>> +	/*
> >>> +	 * There might be a control terminal hooked on, if
> >>> +	 * so dump it first then continue dumping current.
> >>> +	 */
> >>> +	if (major(e.rdev) == TTYAUX_MAJOR) {
> >>> +		sid = tty_get_sid(lfd);
> >>> +		prgp = tty_get_prgp(lfd);
> >>> +		if (sid < 0 || prgp < 0)
> >>> +			return -1;
> >>> +
> >>> +		if (sid) {
> >>> +			if (dump_control_pty(pty.index, sid, prgp))
> >>> +				return -1;
> >>
> >> The dump_control_pty will dump another TtyFileEntry. I don't understand what for.
> > 
> > This is to unify procedure of dumping. Strictly specking we simply need an id
> > for control terminal and file engine will call appropriate ioctl on the slave
> > path (say, /dev/pts/3). But to make code unified and do not introduce some
> > "special" records for control terminals we simply write TtyFileEntry here.
> 
> OK, let's assume we have two tasks:
> 
> t1 = {
> 	pid = 1;
> 	sid = 1;
> 	pgid = 1;
> 	fd[0] = tty_master (index = 3);
> };
> 
> t2 = {
> 	pid = 2;
> 	sid = 1;
> 	pgid = 1;
> 	fd[0] = tty_slave (index = 3);
> };
> 
> and the tty_slave with index 3 will have sid == 1;
> 
> What will be there in the image file with ttys? I expect 2 entries, but it looks
> like there will be 3.

yes, it'll be 3 entries. Letme explain. For such "redundant" entry we generate
a record with unique id, and at restore time we will add Fdinfo entry with
service-tty-fd and bind it to that entry.

Thus when our file engine will meet this service-tty-fd we call set-sid on
such tty device. In the example above it'll be just path to /dev/pts/3.

But i'll think if I can simplify all this scheme (which seems to be a bit
confusing).

> >>> +	tty_flush(lfd);
> >>
> >> What for?
> > 
> > This should flush terminals before dumping, we should not leave them unflushed
> 
> Why?
> 
> > (it's partial solution, since we need to figure out the way to fetch buffered
> >  data from terinals).

As I said it's partial workaround, I need to figure out if there a way to
fetch data from terminal internal buffers.

> >>> +	/*
> >>> +	 * It might be a case where many slave peers hooked on
> >>> +	 * a single /dev/pts/N device. So we dump parameters
> >>> +	 * only once. sid allow us to escape parameters dumping
> >>> +	 * on control terminal.
> >>
> >> What does the last sentence in this comment mean?
> > 
> > If sid is assigned 
> 
> Assigned on what?
> 
> > it means we're dumping control terminal
> > and do not need all parameters but just a few.
> >

nevermind, i'l cooking more simplier version with more precise
commits, forget about this comment please ;) 

> 
> >>> +	ret = pb_write_one(fdset_fd(glob_fdset, CR_FD_TTY), &e, PB_TTY);
> >>
> >> I don't see the tty contents dumping.
> > 
> > If you're about buffered data in terminals, then I yet not discovered
> > how to fetch it from kernel.
> 
> OK. Don't forget to put this in todo list.

yup

> 
> >>> +static int restore_tty_params(int fd, struct tty_file_info *info)
> >>> +{
> >>> +	int ret;
> >>> +
> >>> +	/*
> >>> +	 * No sid or group here, it should be done earlier,
> >>> +	 * becase these settings are special and depends on
> >>> +	 * caller context.
> >>> +	 */
> >>
> >> I don't understand this comment, please, elaborate.
> > 
> > We can't setup sid here, setting up sid is allowed from the task
> > context which sid matches, but function named "restore_tty_params"
> > and I thought mentioning that some params can't be restored here
> > is a good idea.
> 
> OK. Then s/No sid or group/We can't setup sid or group/, s/should/will/ please.

Yeah, sure

	Cyrill