[CRIU] Re: [PATCH 4/4] tty: Add checkpoint/restore for unix
terminals
Pavel Emelyanov
xemul at parallels.com
Wed Sep 5 12:44:26 EDT 2012
> @@ -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?
> @@ -113,6 +114,11 @@ void show_fifo(int fd, struct cr_options *o)
> pb_show_plain(fd, PB_FIFO);
> }
>
> +void show_tty(int fd, struct cr_options *o)
> +{
> + pb_show_plain(fd, PB_TTY);
> +}
> +
> void show_fs(int fd_fs, struct cr_options *o)
> {
> pb_show_vertical(fd_fs, PB_FS);
Showing functionality deserves separate patch.
> @@ -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.
> + close(fe->fd);
> + }
> +
> return 0;
> }
>
> @@ -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?
> +int tty_restore_ctl_terminal(int fd, struct file_desc *d)
> +{
> + struct tty_file_info *info = container_of(d, struct tty_file_info, d);
> + pid_t sid = getsid(getpid());
> +
> + 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.
> +}
The whole thing with "restoring alien ctl ttys" deserves separate patch.
> +struct tty_file_info;
> +
> +struct tty_private {
> + union {
Tabs
> + /*
> + * 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 *
> + } pty;
> + };
> +};
> +
> +struct tty_file_info {
> + struct list_head list;
> + struct file_desc d;
> + TtyFileEntry *tfe;
> + struct tty_private priv;
> +
> + int major;
> + int minor;
> +};
> +
> +static int tty_assign_ctl_terminal(struct tty_file_info *info)
tty_find_restoring_task()
> +{
> + struct pstree_item *item;
> +
> + if (info->tfe->sid == 0)
> + return 0;
> +
> + 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.
> + */
> + 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.
> + if (item->sid == info->tfe->sid) {
> + item->ctl_tty_id = info->tfe->id;
> + return 0;
> + }
> + }
> +
> + pr_err("A process %d is not found\n", info->tfe->sid);
Bad error message.
> + return -1;
> +}
> +
> +/*
> + * 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.
> + */
> +static int tty_handle_priv(void)
tty_setup_slavery
> +static int __dump_one_pty(int lfd, u32 id, const struct fd_parms *p, pid_t sid, pid_t prgp)
> +{
> + TtyFileEntry e = TTY_FILE_ENTRY__INIT;
> + TermiosEntry termios = TERMIOS_ENTRY__INIT;
> + TermiosEntry termios_locked = TERMIOS_ENTRY__INIT;
> + WinsizeEntry winsize = WINSIZE_ENTRY__INIT;
> + TtyPtyEntry pty = TTY_PTY_ENTRY__INIT;
> + struct termios t = { };
> + struct winsize w = { };
> + int ret;
> +
> + pr_info("Dumping tty %d with id %#x\n", lfd, id);
> +
> + e.id = id;
> + e.flags = p->flags;
> + e.pos = p->pos;
> + e.rdev = p->stat.st_rdev;
> + e.fown = (FownEntry *)&p->fown;
> + e.termios = &termios;
> + 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.
> + 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.
> + 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.
> + return -1;
> + }
> + pty.index = ret;
> + if (pty.index > MAX_TTYS) {
> + pr_err("Index %d on tty %x is too big\n", pty.index, id);
> + return -1;
> + }
> +
> + /*
> + * 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.
> + }
> + }
> +
> + tty_flush(lfd);
What for?
> + /*
> + * 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 (!pty_is_dumped(e.rdev, pty.index, sid)) {
> +
> + ret = -1;
> +
> + if (ioctl(lfd, TCGETS, &t) < 0) {
> + pr_perror("Can't get tty params on %d", p->fd);
> + goto out;
> + }
> +
> + termios.n_c_cc = TERM2_NCC;
> + termios.c_cc = xmalloc(pb_repeated_size(&termios, c_cc));
> +
> + termios_locked.n_c_cc = TERM2_NCC;
> + termios_locked.c_cc = xmalloc(pb_repeated_size(&termios_locked, c_cc));
> +
> + if (!termios.c_cc || !termios_locked.c_cc)
> + goto out;
> +
> + termios_conv(&termios, &t);
Isn't the termios_copy name better?
> +
> + if (ioctl(lfd, TIOCGLCKTRMIOS, &t) < 0) {
> + pr_perror("Can't get tty locked params on %d", p->fd);
> + goto out;
> + }
> +
> + termios_conv(&termios_locked, &t);
> +
> + if (ioctl(lfd, TIOCGWINSZ, &w) < 0) {
> + pr_perror("Can't get tty window params on %d", p->fd);
> + goto out;
> + }
> +
> + winsize_conv(&winsize, &w);
Same for winsize_copy.
> +
> + pty_mark_dumped(e.rdev, pty.index);
> + } else {
> + e.termios = NULL;
> + e.termios_locked = NULL;
> + e.winsize = NULL;
> + }
> +
> + ret = pb_write_one(fdset_fd(glob_fdset, CR_FD_TTY), &e, PB_TTY);
I don't see the tty contents dumping.
> +
> +out:
> + xfree(termios.c_cc);
> + xfree(termios_locked.c_cc);
> + return ret;
> +}
> +
> +static int pty_open_ptmx_index(int flags, int index)
> +{
> + int fds[32], i, ret = -1, cur_idx;
> +
> + memset(fds, 0xff, sizeof(fds));
> +
> + mutex_lock(ptmx_index_mutex);
> +
> + for (i = 0; i < ARRAY_SIZE(fds); i++) {
> + fds[i] = open(PTMX_PATH, flags);
> + if (fds[i] < 0) {
> + pr_perror("Can't open %s", PTMX_PATH);
> + break;
> + }
> +
> + if (ioctl(fds[i], TIOCGPTN, &cur_idx)) {
> + pr_perror("Can't obtain current index on %s", PTMX_PATH);
> + break;
> + }
> +
> + pr_debug("\t\tptmx opened with index %d\n", cur_idx);
> +
> + /*
> + * 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.
> + pr_info("ptmx opened with index %d\n", cur_idx);
> + ret = fds[i];
> + fds[i] = -1;
> + break;
> + }
> +
> + /*
> + * 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?
> +
> + pr_err("Unable to open %s with specified index %d\n", PTMX_PATH, index);
> + break;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(fds); i++) {
> + if (fds[i] >= 0)
> + close(fds[i]);
> + }
> +
> + mutex_unlock(ptmx_index_mutex);
> +
> + return ret;
> +}
> +
> +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.
> +
> + 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?
> + termios_conv(&t, info->tfe->termios_locked);
> + if (ioctl(fd, TIOCSLCKTRMIOS, &t) < 0)
> + goto err;
> + }
> +
> + if (info->tfe->termios) {
> + struct termios t = { };
Why is this zeroifying required?
> + termios_conv(&t, info->tfe->termios);
> + if (ioctl(fd, TCSETS, &t) < 0)
> + goto err;
> + }
> +
> + if (info->tfe->winsize) {
> + struct winsize w = { };
Why is this zeroifying required?
> + winsize_conv(&w, info->tfe->winsize);
> + if (ioctl(fd, TIOCSWINSZ, &w) < 0)
> + goto err;
> + }
Need checks for n_foo is enough.
> +
> + return 0;
> +err:
> + pr_perror("Can't set tty params on %d [%s]",
> + info->tfe->id, info->priv.pty.path);
> + return -1;
> +}
> +
> +static int pty_open_ptmx(struct tty_file_info *info)
> +{
> + int master = -1;
> + int pktmode = 1;
> +
> + if (info->major != TTYAUX_MAJOR) {
> + pr_err("Can't restore pty without master\n");
> + return -1;
> + }
> +
> + master = pty_open_ptmx_index(info->tfe->flags, info->tfe->pty->index);
> + if (master < 0) {
> + pr_perror("Can't open %s (index %d)",
> + info->priv.pty.path, info->tfe->pty->index);
> + return -1;
> + }
> +
> + unlock_pty_master(master);
> +
> + if (restore_tty_params(master, info))
> + goto err;
> +
> + /*
> + * 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?
> +
> + if (pty_open_slaves(info))
> + goto err;
> +
> + return master;
> +err:
> + close_safe(&master);
> + return -1;
> +}
> +
> +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???
> + continue;
> +
> + fd = open(pts_name, slave->tfe->flags);
> + if (fd < 0) {
> + pr_perror("Can't open slave %s", pts_name);
> + goto err;
> + }
> +
> + fle = file_master(&slave->d);
> +
> + pr_debug("send slave %#x fd %d connected on %s (pid %d)\n",
> + slave->tfe->id, fd, slave->priv.pty.path, fle->pid);
> +
> + if (send_fd_to_peer(fd, fle, sock)) {
> + pr_perror("Can't send file descriptor");
> + goto err;
> + }
> +
> + close(fd);
> + fd = -1;
> + }
> + ret = 0;
> +
> +err:
> + close_safe(&fd);
> + close_safe(&sock);
> + return ret;
> +}
> +
More information about the CRIU
mailing list