[CRIU] Re: [PATCH 3/4] tty: Add checkpoint/restore for unix
terminals
Cyrill Gorcunov
gorcunov at openvz.org
Mon Aug 13 08:08:26 EDT 2012
On Mon, Aug 13, 2012 at 02:28:52PM +0400, Pavel Emelyanov wrote:
> > 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.
ok
>
> > 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.
sure
>
> > #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?
that's kernel limit
#define NCCS 19
struct termios {
...
cc_t c_cc[NCCS]; /* control characters */
};
> > +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?
This seems to be more convenient, no?
> > +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?
Because there are a few types of ttys, this patch addresses
unix98 ptys only but we will extend it later to support more
ttys.
> > new file mode 100644
> > index 0000000..c80a86a
> > +struct tty_file_info;
> > +
> > +struct tty_private {
> > + union {
>
> Union?
yes, private field should be big enough to carry _any_
tty type we will ever support
> > +
> > +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.
Indeed, i'll fix it up, thanks.
>
> > + 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.
or-s ? if the ioctl fails -- we're in trouble, so I'm not following a bit ;)
> > + 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.
ok
> > +static int unlock_pty_master(int master)
> > +{
> > + const int lock = 0;
> > +
> > + if (ioctl(master, TIOCSPTLCK, &lock)) {
>
> Plz, comment on this.
Once opened the master remains in locked state so before
we're connecting slave here we need to unlock it. Still,
there might be a case where master remains locked after
creation in the dumpee, need to find a way how to figure
out lock bit. I guess I need to try to "connect" from
our dumping code and if connect rejected -- the master
in locking state.
Pavel, note that this is RFC, since not everything addressed
and I don't like how the final code looks, so I thought posting
it early might be a good idea :)
> > +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();
ok
> > +
> > + slave_desc = find_file_desc_raw(FD_TYPES__TTY, slave->tfe->id);
> > + fle = file_master(slave_desc);
>
> Same here -- isn't info->desc applicable?
hmm, good point, will re-check.
> > + 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.
No, this is not FD sharing, the slave might be opened by several callers and we
will have two different FD which open same say /dev/pts/1, but any attempts of
opetning this slave should be done only after master link opened and slave file
is created that is why it's done such way.
> > + close_safe(&slave_fd);
> > + }
> > +
> > +out:
> > + ret = master_fd, master_fd = -1;
>
> No ,-s please.
could you elaborate? you mean no commas please? Actually this
is pretty valid syntax, but sure will update.
> > +
> > + 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.
This code tries to handle cases where slave is opened in task with pid < than
pid of task which opens master device, thus if this happens we need to open
master first and send it to waiter and then we can open slave immediately.
Also on standalone slaves we might need to open fake ptmx device, which we
close once slave opened.
> > +
> > + BUG_ON(1);
> > + return -1;
>
> Excessive. This check is already done earlier.
>
ok
> > + 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.
OK, will re-phrase.
>
> > +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 ...
>
...
> > +
> > + 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.
>
ok
> > +
> > + list_add(&info->list, &all_ttys);
> > + file_desc_add(&info->d, info->tfe->id, &tty_desc_ops);
>
> Plz, use collect_image().
ok. Thanks for comments, Pavel!
Cyrill
More information about the CRIU
mailing list