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

[Cyrill Gorcunov]

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. We use maj to figure out which exactly devices
are to be checkpointed

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

	switch (maj) {
	case MEM_MAJOR:
		return dump_reg_file(p, lfd, set);
	case TTYAUX_MAJOR:
	case UNIX98_PTY_MASTER_MAJOR ... (UNIX98_PTY_MASTER_MAJOR + UNIX98_PTY_MAJOR_COUNT - 1):
	case UNIX98_PTY_SLAVE_MAJOR:
		return dump_tty(p, lfd, set);
	}

	return dump_unsupp_fd(p);
}

no?

> Showing functionality deserves separate patch.

OK

> > @@ -394,6 +419,13 @@ static int open_fd(int pid, FdinfoEntry *fe, struct file_desc *d)
> >  	}
> >  
> >  	close(sock);
> > +
> > +	if (fe->fd == get_service_fd(CTL_TTY_OFF)) {
> > +		if (tty_restore_ctl_terminal(fe->fd, d))
> > +			return -1;
> 
> Better introduce the is_service_fd() helper that will look like
> 
> 	return fd >= MAX_SERVICE_FD
> 
> but don't call for this fn again.

Hmm, ok.

> > @@ -420,6 +452,7 @@ static int receive_fd(int pid, FdinfoEntry *fe, struct file_desc *d)
> >  		return -1;
> >  
> >  	fcntl(tmp, F_SETFD, fe->flags);
> > +
> >  	return 0;
> >  }
> >  
> 
> Huh?

Huh, what? ;)

> > +
> > +	BUG_ON(sid != info->tfe->sid);
> > +
> > +	pr_info("Restore session %d by %d tty on %d\n",
> > +		 sid, (int)getpid(), fd);
> > +
> > +	if (tty_set_sid(fd)) {
> > +		pr_perror("Can't set session %d by %d tty on %d",
> > +			  sid, (int)getpid(), fd);
> > +		return -1;
> > +	}
> > +
> > +	return tty_set_prgp(fd, info->tfe->prgp);
> 
> No error message for this one failure.

Yeah, thanks! Good catch!

> The whole thing with "restoring alien ctl ttys" deserves separate patch.

ok

> > +struct tty_private {
> > +  union {
> 
> Tabs

ok

> > +	/*
> > +	 * At moment only unix98 ptys are addressed
> > +	 * but this union structure should be big enough to
> > +	 * keep any data related to another type of ttys.
> > +	 */
> > +	struct {
> > +		struct list_head	sibling;
> > +		char			path[32];
> 
> char *

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

> > +static int tty_assign_ctl_terminal(struct tty_file_info *info)
> 
> tty_find_restoring_task()

ok

> > +	pr_info("Set a control terminal to %d\n", info->tfe->sid);
> > +
> > +	/*
> > +	 * Note we change ctl_tty_id to match one the real
> > +	 * control terminal fd entry has. This is done to
> > +	 * make sure the control terminal fake entry will
> > +	 * be staged into proper fdinfo list the file engine
> > +	 * uses.
> 
> Need a comment (somewhere, even in the respective patch comment) why
> we need to handle alien terminals at all.

ok

> > +	for_each_pstree_item(item) {
> 
> We need a "fast-path" for this guy. I.e. -- if a task opening an fd
> will be the one with item->sid == info->tfe->sid, it should become
> the tty session restorer and avoid this long loop.
> 
> With the separate patch.

ok

> > +	pr_err("A process %d is not found\n", info->tfe->sid);
> 
> Bad error message.

well, will try to provide better one.

> > +/*
> > + * This routine connects peers, also figures out
> > + * the method of how peers are to be created.
> 
> I don't see the method-to-be-created finding.

i'll update this comment, thanks.

> > +static int tty_handle_priv(void)
> 
> tty_setup_slavery

ok

> > +	e.termios_locked	= &termios_locked;
> > +	e.pty			= &pty;
> > +	e.winsize		= &winsize;
> 
> Do optional fields assignment in the respective if () branch below and kill
> the else one.

ok

> > +	e.type			= TTY_FILE_ENTRY__TYPE__PTY;
> 
> This name is still bad. Just move the respective enum out of the message
> in the .proto file and make it look like TTY_TYPE__PTY.

Sure, thanks

> > +	e.sid			= sid;
> > +	e.prgp			= prgp;
> > +
> > +	ret = parse_index(lfd, e.rdev, p);
> > +	if (ret < 0) {
> > +		pr_perror("Can't get tty index on %d", p->fd);
> 
> The parse_index prints error itself.

yeah, overdone.

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

> > +	tty_flush(lfd);
> 
> What for?

This should flush terminals before dumping, we should not leave them unflushed
(it's partial solution, since we need to figure out the way to fetch buffered
 data from terinals).

> > +	/*
> > +	 * 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 it means we're dumping control terminal
and do not need all parameters but just a few.

> > +		if (!termios.c_cc || !termios_locked.c_cc)
> > +			goto out;
> > +
> > +		termios_conv(&termios, &t);
> 
> Isn't the termios_copy name better?

Yeah, thanks

> > +		winsize_conv(&winsize, &w);
> 
> Same for winsize_copy.

ok

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

> > +		/*
> > +		 * Index match or any index requested.
> > +		 */
> > +		if (cur_idx == index || index == -1) {
> 
> If index == -1 we don't have to call the GPTN ioctl at all.

yeah, thanks

> > +		/*
> > +		 * Maybe indices are already borrowed by
> > +		 * someone else, so no need to continue.
> > +		 */
> > +		if (cur_idx < index && (index - cur_idx) < ARRAY_SIZE(fds))
> > +			continue;
> 
> What guarantees that when we open idx 5 then try to open 4 the 4th will be granted?
> What if kernel changes so that indices are _always_ allocated sequentially?
> Do we need to add sorting of ptmx-s before opening?

well, i think yes, sorting ptmx-s will save us from headache in future. I'll fix
this.

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

> > +	ret = rst_file_params(fd, info->tfe->fown, info->tfe->flags);
> > +	if (ret)
> > +		return -1;
> > +
> > +	if (info->tfe->termios_locked) {
> > +		struct termios t = { };
> 
> Why is this zeroifying required?

not required indeed, but when I've been playing with this code
earlier I needed to escape any possible side effect. I'll drop this.

> > +		winsize_conv(&w, info->tfe->winsize);
> > +		if (ioctl(fd, TIOCSWINSZ, &w) < 0)
> > +			goto err;
> > +	}
> 
> Need checks for n_foo is enough.

ok

> > +	/*
> > +	 * PTYs are safe to use packet mode.
> > +	 */
> > +	if (ioctl(master, TIOCPKT, &pktmode) < 0)
> > +		pr_perror("Can't set packed mode");
> 
> What for? Why error isn't handled?

my bad, will fix (as to what for -- screen  requires
packet mode to be set otherwise we loose echos and
packet mode is safe to use in ptys _but_ for other
uarts it might be a problem. strictly speaking we
need some way to fetch this flag from kernel and
i fear without kernel patching it's impossible).

> > +static int pty_open_slaves(struct tty_file_info *info)
> > +{
> > +	int sock = -1, fd = -1, ret = -1;
> > +	struct fdinfo_list_entry *fle;
> > +	struct tty_file_info *slave;
> > +	char pts_name[64];
> > +
> > +	snprintf(pts_name, sizeof(pts_name), PTS_FMT, info->tfe->pty->index);
> > +
> > +	sock = socket(PF_UNIX, SOCK_DGRAM, 0);
> > +	if (sock < 0) {
> > +		pr_perror("Can't create socket");
> > +		goto err;
> > +	}
> > +
> > +	list_for_each_entry(slave, &info->priv.pty.sibling, priv.pty.sibling) {
> > +		if (pty_is_master(slave))
> 
> WHAT???

well, there should be BUG_ON rather, thanks!

	Cyrill