[CRIU] Re: [PATCH 3/4] tty: Add checkpoint/restore for unix
terminals
Pavel Emelyanov
xemul at parallels.com
Mon Aug 13 06:28:52 EDT 2012
> Btw, this problem is addressed in test pty02.
1. Talk to Andrey on this.
2. Patch is corrupted, send such big stuff in attachments.
3. More comments inline.
> Signed-off-by: Cyrill Gorcunov <gorcunov at openvz.org>
> @@ -63,6 +63,7 @@
> #define IFADDR_MAGIC RAW_IMAGE_MAGIC
> #define ROUTE_MAGIC RAW_IMAGE_MAGIC
> #define TMPFS_MAGIC RAW_IMAGE_MAGIC
> +#define TTY_MAGIC 0x59433025 /* Pushkin */
Plz, put non-raw magics in one block.
> #define PAGE_IMAGE_SIZE 4096
> #define PAGE_RSS 1
> @@ -0,0 +1,19 @@
> +#ifndef CR_TTY_H__
> +#define CR_TTY_H__
> +
> +#include "files.h"
> +#include "crtools.h"
> +
> +#define TERM2_NCC 19
Why 19?
> +
> +#define PTMX_PATH "/dev/ptmx"
> +#ifndef PTMX_MINOR
> +# define PTMX_MINOR 2
> +#endif
> +#define PTS_FMT "/dev/pts/%d"
> @@ -0,0 +1,37 @@
> +import "fown.proto";
> +
> +message term2_entry {
> + required uint32 c_iflag = 1;
> + required uint32 c_oflag = 2;
> + required uint32 c_cflag = 3;
> + required uint32 c_lflag = 4;
> + required uint32 c_line = 5;
> + required uint32 c_ispeed = 6;
> + required uint32 c_ospeed = 7;
> +
> + repeated uint32 c_cc = 8;
> +}
Why separate message?
> +message tty_unix98_entry {
> + required uint32 index = 1;
> +}
> +
> +message tty_file_entry {
> + required uint32 id = 1;
> + required uint32 flags = 2;
> + required uint32 sid = 3;
> + required uint32 prgp = 4;
> + required uint64 pos = 5;
> + required uint64 rdev = 6;
> + required fown_entry fown = 7;
> + required term2_entry term2 = 8;
> +
> + enum TtyType {
> + UNKNOWN = 0;
> + UNIX98 = 1;
> + }
> +
> + required TtyType type = 9;
> +
> + optional tty_unix98_entry unix98 = 10;
It is one-entry lenght, why making it a separate message?
> +}
> diff --git a/tty.c b/tty.c
> new file mode 100644
> index 0000000..c80a86a
> +struct tty_file_info;
> +
> +struct tty_private {
> + union {
Union?
> + struct {
> + struct tty_file_info *peer; /* pointer to peer info if there */
> + struct list_head list;
> +
> + bool fake_ptmx; /* fake ptmx needed to create peer */
> + bool send_peer;
> + char path[64];
> + } unix98;
> + };
> +};
> +
> +struct tty_file_info {
> + struct list_head list;
> + struct file_desc d;
> + TtyFileEntry *tfe;
> + struct tty_private priv;
> +
> + int major;
> + int minor;
> + bool create;
> +};
> +
> +static LIST_HEAD(all_ttys);
> +
> +static void from_termios(Term2Entry *d, struct termios *s)
> +{
> + BUG_ON(pb_repeated_size(d, c_cc) < sizeof(s->c_cc));
> +
> + memcpy(d->c_cc, s->c_cc, sizeof(s->c_cc));
> +
> + ASSIGN_MEMBER(d, s, c_iflag);
> + ASSIGN_MEMBER(d, s, c_oflag);
> + ASSIGN_MEMBER(d, s, c_cflag);
> + ASSIGN_MEMBER(d, s, c_lflag);
> + ASSIGN_MEMBER(d, s, c_line);
> +}
> +
> +static void to_termios(struct termios *d, Term2Entry *s)
> +{
> + BUG_ON(s->n_c_cc != TERM2_NCC);
BUG_ON on smth coming from images is wrong.
> + memcpy(d->c_cc, s->c_cc, sizeof(d->c_cc));
> +
> + ASSIGN_MEMBER(d, s, c_iflag);
> + ASSIGN_MEMBER(d, s, c_oflag);
> + ASSIGN_MEMBER(d, s, c_cflag);
> + ASSIGN_MEMBER(d, s, c_lflag);
> + ASSIGN_MEMBER(d, s, c_line);
> +}
> +
> +static int tty_open_ptmx_index(int flags, int index)
> +{
> + int fds[32], i, ret = -1, cur_idx;
> +
> + memset(fds, 0xff, sizeof(fds));
> +
> + 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;
> + }
> +
> + /*
> + * Index match or any index requested.
I don't see or-s in the if () statement.
> + */
> + if (cur_idx == index) {
> + ret = fds[i];
> + fds[i] = -1;
> + break;
> + }
> +
> + pr_debug("ptmx opened with index %d\n", cur_idx);
> +
> + /*
> + * Maybe indices are already borrowed by
> + * someone else, so no need to continue.
> + */
> + if (cur_idx < index && (index - cur_idx) < ARRAY_SIZE(fds))
> + continue;
> + break;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(fds); i++) {
> + if (fds[i] >= 0)
> + close(fds[i]);
> + }
> +
> + if (ret < 0)
> + pr_err("Unable to open %s with specified index %d\n",
> + PTMX_PATH, index);
This is obfuscating. Put the index warning into the loop, before the last break.
> + return ret;
> +}
> +
> +static int unlock_pty_master(int master)
> +{
> + const int lock = 0;
> +
> + if (ioctl(master, TIOCSPTLCK, &lock)) {
Plz, comment on this.
> + pr_err("Unable to unlock pty master device %d\n", master);
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +static int receive_tty(struct tty_file_info *info)
> +{
> + struct fdinfo_list_entry *fle;
> + struct file_desc *info_desc;
> + int fd;
> +
> + info_desc = find_file_desc_raw(FD_TYPES__TTY, info->tfe->id);
> + fle = file_master(info_desc);
info_desc = &info->desc; ?
> + pr_info("\tWaiting tty fd %d (pid %d)\n",
> + fle->fe->fd, fle->pid);
> +
> + fd = recv_fd(fle->fe->fd);
> + close(fle->fe->fd);
> + if (fd < 0) {
> + pr_err("Can't get fd %d\n", fd);
> + return -1;
> + }
File parms restore missed.
> +
> + return fd;
> +}
> +
> +static int tty_open_ptmx(struct tty_file_info *info)
> +{
> + int master_fd = -1, sock = -1, ret = -1;
> + struct tty_file_info *slave;
> + int slave_fd = -1;
> +
> + /* If we fail here we won't connect peer anyway */
> + master_fd = tty_open_ptmx_index(info->tfe->flags | O_RDWR,
> + info->tfe->unix98->index);
> + if (master_fd >= 0) {
> + unlock_pty_master(master_fd);
Make it simpler. Lke
master_fd = tty_open()
if (master_fd < 0)
goto err;
unlock_pty_master();
> + } else {
> + pr_err("Can't open ptmx for %s (%d)\n",
> + info->priv.unix98.path, info->tfe->unix98->index);
> + goto err;
> + }
> +
> + if (list_empty(&info->priv.unix98.list))
> + goto out;
> +
> + sock = socket(PF_UNIX, SOCK_DGRAM, 0);
> + if (sock < 0) {
> + pr_perror("Can't create socket");
> + goto err;
> + }
> +
> + /*
> + * Once master opened, try to open all slaves connected
> + * and send them out to receivers.
> + */
> + list_for_each_entry(slave, &info->priv.unix98.list, priv.unix98.list) {
> + struct file_desc *slave_desc;
> + struct fdinfo_list_entry *fle;
> +
> + /* This slave is special */
> + if (slave->create)
> + continue;
> +
> + slave_fd = open(slave->priv.unix98.path, slave->tfe->flags);
> + if (slave_fd < 0) {
> + pr_perror("Can't open %s", slave->priv.unix98.path);
> + goto err;
> + }
> +
> + slave_desc = find_file_desc_raw(FD_TYPES__TTY, slave->tfe->id);
> + fle = file_master(slave_desc);
Same here -- isn't info->desc applicable?
> + pr_debug("send slave %#x fd %d connected on %s (pid %d)\n",
> + slave->tfe->id, slave_fd, slave->priv.unix98.path, fle->pid);
> +
> + list_for_each_entry(fle, &slave_desc->fd_info_head, desc_list) {
> + if (send_fd_to_peer(slave_fd, fle, sock)) {
You send one fd to many peers. This doesn't look correct. FD sharing is handled
by generic code.
> + pr_perror("Can't send file descriptor");
> + goto err;
> + }
> + }
> +
> + close_safe(&slave_fd);
> + }
> +
> +out:
> + ret = master_fd, master_fd = -1;
No ,-s please.
> +err:
> + close_safe(&slave_fd);
> + close_safe(&sock);
> + close_safe(&master_fd);
> + return ret;
> +}
> +
> +static int tty_open_pts(struct tty_file_info *info)
> +{
> + int slave_fd = -1, master_fd = -1, ret = -1, sock = -1;
> +
> + if (info->priv.unix98.fake_ptmx) {
> + slave_fd = open(info->priv.unix98.path, info->tfe->flags);
> + if (slave_fd >= 0)
> + return slave_fd;
> +
> + master_fd = tty_open_ptmx(info);
> + if (master_fd < 0)
> + return -1;
> + } else if (info->priv.unix98.send_peer) {
> + struct tty_file_info *master = info->priv.unix98.peer;
> + struct file_desc *master_desc;
> + struct fdinfo_list_entry *fle;
> +
> + master_fd = tty_open_ptmx(master);
> + if (master_fd < 0)
> + return -1;
> +
> + sock = socket(PF_UNIX, SOCK_DGRAM, 0);
> + if (sock < 0) {
> + pr_perror("Can't create socket");
> + goto out;
> + }
> +
> + master_desc = find_file_desc_raw(FD_TYPES__TTY, master->tfe->id);
> + fle = file_master(master_desc);
> +
> + list_for_each_entry(fle, &master_desc->fd_info_head, desc_list) {
> + if (send_fd_to_peer(master_fd, fle, sock)) {
> + pr_perror("Can't send file descriptor");
> + goto out;
> + }
> + }
> + }
> +
> + slave_fd = open(info->priv.unix98.path, info->tfe->flags);
> + if (slave_fd < 0) {
> + pr_perror("Can't open %s\n", info->priv.unix98.path);
> + }
> +
> + ret = slave_fd, slave_fd = -1;
No ,-s please.
> +out:
> + close_safe(&sock);
> + close_safe(&slave_fd);
> + close_safe(&master_fd);
> + return ret;
I don't understand the code above. Plz, comment.
> +}
> +
> +static int tty_lookup_open(struct tty_file_info *info)
> +{
> + pr_debug("open for index %d create %d send peer %d fake %d (path %s)\n",
> + info->tfe->unix98->index,
> + !!info->create,
> + !!info->priv.unix98.send_peer,
> + !!info->priv.unix98.fake_ptmx,
> + info->priv.unix98.path);
> +
> + if (!info->create)
> + return receive_tty(info);
> +
> + switch (info->major) {
> + case UNIX98_PTY_SLAVE_MAJOR:
> + return tty_open_pts(info);
> + case TTYAUX_MAJOR:
> + return tty_open_ptmx(info);
> + }
> +
> + BUG_ON(1);
> + return -1;
Excessive. This check is already done earlier.
> +}
> +
> +static int tty_get_sid(int fd)
> +{
> + int sid, ret;
> +
> + ret = ioctl(fd, TIOCGSID, &sid);
> + if (ret < 0) {
> + if (errno != ENOTTY) {
> + pr_perror("Can't get sid on %d", fd);
> + return -1;
> + }
> + sid = 0;
> + }
> + return sid;
> +}
> +
> +static int tty_open(struct file_desc *d)
> +{
> + struct tty_file_info *info = container_of(d, struct tty_file_info, d);
> + int tmp, ret = -1;
> + struct termios t = { };
> +
> + tmp = tty_lookup_open(info);
> + if (tmp < 0) {
> + pr_perror("Can't open tty id %#08x [%s]",
> + info->tfe->id, info->priv.unix98.path);
> + return -1;
> + }
> +
> + if (info->tfe->sid) {
> + /* If we're having same sid already -- nothing to do */
> + if (tty_get_sid(tmp) != info->tfe->sid) {
> + if (ioctl(tmp, TIOCSCTTY, 1)) {
> + pr_perror("Can't set session %d by %d tty on %d [%s]",
> + (int)info->tfe->sid, (int)getpid(),
> + info->tfe->id, info->priv.unix98.path);
> + goto err;
> + }
> + pr_debug("Restore session %d by %d tty on %d [%s]\n",
> + (int)info->tfe->sid, (int)getpid(),
> + info->tfe->id, info->priv.unix98.path);
> + } else
> + pr_debug("Skip restoring session %d by %d tty on %d [%s]\n",
> + (int)info->tfe->sid, (int)getpid(),
> + info->tfe->id, info->priv.unix98.path);
Why skip? get_sid reported sid match.
> + }
> +
> + if (info->tfe->prgp) {
> + if (ioctl(tmp, TIOCSPGRP, &info->tfe->prgp)) {
> + pr_perror("Can't set tty group on %d [%s]",
> + info->tfe->id, info->priv.unix98.path);
> + goto err;
> + }
> + pr_debug("Restore tty group on %d [%s]\n",
> + info->tfe->id, info->priv.unix98.path);
> + }
> +
> + if (rst_file_params(tmp, info->tfe->fown, info->tfe->flags)) {
> + pr_perror("Can't restore params on tfe %#08x",
> + info->tfe->id);
> + goto err;
> + }
> +
> + to_termios(&t, info->tfe->term2);
> + if (ioctl(tmp, TCSETS, &t) < 0) {
> + pr_perror("Can't set tty params on %d [%s]",
> + info->tfe->id, info->priv.unix98.path);
> + goto err;
> + }
> +
> + ret = tmp, tmp = -1;
> +err:
> + close_safe(&tmp);
> + return ret;
> +}
> +
> +int tty_handle_priv(void)
> +{
> + struct tty_file_info *info, *master, *tmp, *slave;
> + struct fdinfo_list_entry *slave_fle, *master_fle, *this;
> + LIST_HEAD(masters);
> + LIST_HEAD(slaves);
> + LIST_HEAD(standalone_slaves);
>From here to ...
> + /*
> + * Range masters and slaves and init anything needed.
> + */
> + list_for_each_entry_safe(info, tmp, &all_ttys, list) {
> + if (info->major == TTYAUX_MAJOR) {
> + strncpy(info->priv.unix98.path,
> + PTMX_PATH, sizeof(info->priv.unix98.path));
> + INIT_LIST_HEAD(&info->priv.unix98.list);
> + list_move(&info->list, &masters);
> + } else if (info->major == UNIX98_PTY_SLAVE_MAJOR) {
> + snprintf(info->priv.unix98.path,
> + sizeof(info->priv.unix98.path),
> + PTS_FMT, info->tfe->unix98->index);
> + INIT_LIST_HEAD(&info->priv.unix98.list);
> + list_move(&info->list, &slaves);
> + }
> + }
> +
> +
> + /*
> + * Chain slaves on masters
> + */
> + list_for_each_entry_safe(slave, tmp, &slaves, list) {
> + bool added = false;
> + slave->priv.unix98.fake_ptmx = true;
> + slave->priv.unix98.send_peer = false;
> +
> + list_for_each_entry(master, &masters, list) {
> + if (slave->tfe->unix98->index == master->tfe->unix98->index) {
> + slave->priv.unix98.peer = master;
> + slave->priv.unix98.fake_ptmx = false;
> +
> + list_add(&slave->priv.unix98.list,
> + &master->priv.unix98.list);
> + added = true;
> + break;
> + }
> + }
> +
> + if (!added)
> + list_move(&slave->list, &standalone_slaves);
> + }
> +
> + /*
> + * Try to resolve the case where slaves are
> + * created earlier than masters, thus we need
> + * to create masters as well but send them out
> + * via SCM to a waiting side.
> + */
> + list_for_each_entry(master, &masters, list) {
> + bool spec = false;
> + master_fle = file_master(&master->d);
> +
> + if (list_empty(&master->priv.unix98.list))
> + continue;
> +
> + INIT_LIST_HEAD(&slaves);
> + this = NULL;
> +
> + list_for_each_entry_safe(slave, tmp, &master->priv.unix98.list, priv.unix98.list) {
> + slave_fle = file_master(&slave->d);
> + slave->create = false;
> +
> + if (slave_fle->pid < master_fle->pid ||
> + (slave_fle->pid == master_fle->pid &&
> + slave_fle->fe->fd < master_fle->fe->fd)) {
> + if (unlikely(!this)) {
> + this = slave_fle;
> + list_move(&slave->priv.unix98.list, &slaves);
> + } else
> + if (slave_fle->fe->fd < this->fe->fd)
> + list_move(&slave->priv.unix98.list, &slaves);
> + else
> + list_move_tail(&slave->priv.unix98.list, &slaves);
> + spec = true;
> + } else
> + list_move_tail(&slave->priv.unix98.list, &slaves);
> + }
> +
> + if (spec) {
> + slave = list_first_entry(&slaves, struct tty_file_info, priv.unix98.list);
> + slave->create = true;
> + slave->priv.unix98.send_peer = true;
> + master->create = false;
> + }
> +
> + list_splice(&slaves, &master->priv.unix98.list);
> + }
> +
> + list_for_each_entry(master, &masters, list) {
> + master_fle = file_master(&master->d);
> +
> + pr_info("master %#x index %d fd %d pid %d (create %d sid %d pgrp %d)\n",
> + master->tfe->id, master->tfe->unix98->index,
> + master_fle->fe->fd, master_fle->pid, !!master->create,
> + !!master->tfe->sid, !!master->tfe->prgp);
> +
> + list_for_each_entry(slave, &master->priv.unix98.list, priv.unix98.list) {
> + slave_fle = file_master(&slave->d);
> +
> + pr_info(" `- slave %#x index %d fd %d pid %d (create %d send %d sid %d pgrp %d)\n",
> + slave->tfe->id, slave->tfe->unix98->index,
> + slave_fle->fe->fd, slave_fle->pid, !!slave->create,
> + !!slave->priv.unix98.send_peer,
> + !!slave->tfe->sid, !!slave->tfe->prgp);
> + }
> + }
> +
> + list_for_each_entry(slave, &standalone_slaves, list) {
> + slave_fle = file_master(&slave->d);
> +
> + pr_info("standalone slave %#x index %d fd %d pid %d (create %d sid %d pgrp %d)\n",
> + slave->tfe->id, slave->tfe->unix98->index,
> + slave_fle->fe->fd, slave_fle->pid, !!slave->create,
> + !!slave->tfe->sid, !!slave->tfe->prgp);
> + }
...here: A descriptive, very descriptive comment is required as well.
> + return 0;
> +}
> +
> +int collect_tty(void)
> +{
> + struct tty_file_info *info = NULL;
> + int fd, ret = -1;
> +
> + fd = open_image_ro(CR_FD_TTY);
> + if (fd < 0)
> + return -1;
> +
> + while (1) {
> + info = xzalloc(sizeof(*info));
> + if (!info) {
> + ret = -1;
> + break;
> + }
> +
> + ret = pb_read_one_eof(fd, &info->tfe, PB_TTY);
> + if (ret <= 0)
> + break;
> +
> + if (info->tfe->type != TTY_FILE_ENTRY__TTY_TYPE__UNIX98) {
> + pr_err("Unsuported tty type %d\n", info->tfe->type);
> + ret = -1;
> + goto out_free;
> + }
> +
> + info->create = true;
> + info->major = major(info->tfe->rdev);
> + info->minor = minor(info->tfe->rdev);
> +
> + /* Partial ordering is needed for search speedup */
> + switch (info->major) {
> + case TTYAUX_MAJOR:
> + case UNIX98_PTY_SLAVE_MAJOR:
> + break;
> + default:
> + pr_err("Unsupported tty type (major %d)\n", info->major);
> + ret = -1;
> + goto out_free;
> + }
> +
> + pr_info("Collected tty ID %#x\n", info->tfe->id);
> +
> + list_add(&info->list, &all_ttys);
> + file_desc_add(&info->d, info->tfe->id, &tty_desc_ops);
Plz, use collect_image().
> + }
> +
> +out:
> + xfree(info);
> + close(fd);
> + return ret;
> +
> +out_free:
> + tty_file_entry__free_unpacked(info->tfe, NULL);
> + goto out;
> +}
More information about the CRIU
mailing list