[CRIU] [PATCH 1/2] tty: Write unread pty buffers on post dump stage

[Pavel Emelyanov]

On 05/12/2016 09:21 PM, Cyrill Gorcunov wrote:
> When unread data present on peers we currently simply ignore it but
> actually we can try to fetch it in non(that)destructive way.
> 
> For this sake at the end of dump procedure (because fetching
> queued data may go wrong and we will have to write it back,
> which is heavy, and we need all ttys under our hands)
> we walk over all collected TTYs and link PTYs peers which
> indices are matching. Note to not overload tty_dump_info we
> reuse @list member for new @all_ptys list.
> 
> Once link established we literally read queued data and flush
> it into new tty-data.img. If something go wrong at this moment,
> we stop reading queued data but walk back over already queued
> ones and write them back to restore former state. Same applies
> if the dump has been requested to leave task alive.
> 
> On restore we link peers back and write queued data once
> peer back to live.

OK, this looks sane. I have several cosmetic comments inline.

> diff --git a/criu/cr-dump.c b/criu/cr-dump.c
> index 5ac9fd041e4e..dcb496ce94fc 100644
> --- a/criu/cr-dump.c
> +++ b/criu/cr-dump.c
> @@ -1545,6 +1545,8 @@ static int cr_dump_finish(int ret)
>  {
>  	int post_dump_ret = 0;
>  
> +	tty_dump_queued_data(ret);

The ret code is ignored.
There's already a call to tty_verify_active_pairs() outside of
this call, why not make all tty-post-actions in one call to tty.c?

> +
>  	if (disconnect_from_page_server())
>  		ret = -1;
>  

> diff --git a/criu/tty.c b/criu/tty.c
> index c6e8e32b7013..af7ed94d399b 100644
> --- a/criu/tty.c
> +++ b/criu/tty.c
> @@ -800,6 +815,22 @@ static int restore_tty_params(int fd, struct tty_info *info)
>  	return userns_call(do_restore_tty_parms, UNS_ASYNC, &p, sizeof(p), fd);
>  }
>  
> +static void pty_restore_queued_data(struct tty_info *info, int fd)

Why not abort restore in case of incomplete write?

> +{
> +	if (info && info->tty_data) {
> +		ProtobufCBinaryData bd = info->tty_data->tde->data;
> +		int retval;
> +
> +		pr_debug("restore queued data on %#x (%zu bytes)\n",
> +			 info->tfe->id, (size_t)bd.len);
> +
> +		retval = write(fd, bd.data, bd.len);
> +		if (retval != bd.len)
> +			pr_err("Restored %d bytes while %zu expected\n",
> +			       retval, (size_t)bd.len);
> +	}
> +}
> +
>  static int pty_open_slaves(struct tty_info *info)
>  {
>  	int sock = -1, fd = -1, ret = -1;

> @@ -1238,6 +1272,26 @@ static int tty_setup_slavery(void * unused)
>  	struct tty_info *info, *peer, *m;
>  
>  	/*
> +	 * Setup links for PTY terminal pairs.
> +	 */
> +	list_for_each_entry(info, &all_ttys, list) {
> +		if (!is_pty(info->driver) || info->link)
> +			continue;
> +		peer = info;
> +		list_for_each_entry_continue(peer, &all_ttys, list) {
> +			if (!is_pty(peer->driver) || peer->link)
> +				continue;
> +			if (peer->tie->pty->index == info->tie->pty->index) {

Matching of tty's by indexes is already done below in this function. Is
this different?

> +				info->link = peer;
> +				peer->link = info;
> +
> +				pr_debug("Link PTYs (%#x)\n", info->tfe->id);
> +				break;
> +			}
> +		}
> +	}
> +
> +	/*
>  	 * The image may carry several terminals opened
>  	 * belonging to the same session, so choose the
>  	 * leader which gonna be setting up the controlling
> @@ -1407,6 +1461,18 @@ struct collect_image_info tty_info_cinfo = {
>  	.collect	= collect_one_tty_info_entry,
>  };
>  
> +static struct tty_data_entry *tty_lookup_data(struct tty_info *info)
> +{
> +	struct tty_data_entry *td;
> +
> +	list_for_each_entry(td, &all_tty_data_entries, list) {
> +		if (td->tde->tty_id == info->tie->id)
> +			return td;
> +	}

Any reason why data is put into global list and is then picked by
ttys instead of tty itself is searched at data collect stage and
receives data in its list_head?

> +
> +	return NULL;
> +}
> +
>  static int collect_one_tty(void *obj, ProtobufCMessage *msg, struct cr_img *i)
>  {
>  	struct tty_info *info = obj;

> +
> +static void tty_do_writeback_queued_data(struct tty_dump_info *dinfo)
> +{
> +#define __dinfo_write_reblock(__d)							\
> +	do {										\
> +		if (write((__d)->link->lfd, (__d)->tty_data,				\
> +			  (__d)->tty_data_size) != (__d)->tty_data_size)		\
> +			pr_perror("Can't writeback to tty (%#x)\n", (__d)->id);		\
> +		tty_reblock((__d)->link->id, (__d)->link->lfd, (__d)->link->flags);	\
> +	} while (0)

Why not having this as static-inline function?

> +
> +	if (dinfo->tty_data)
> +		__dinfo_write_reblock(dinfo);
> +	if (dinfo->link->tty_data)
> +		__dinfo_write_reblock(dinfo->link);
> +#undef __dinfo_write_reblock
> +}
> +
> +/*

> +	if (ret || opts.final_state != TASK_DEAD) {
> +		list_for_each_entry(dinfo, &all_ptys, list)
> +			tty_do_writeback_queued_data(dinfo);
> +	}
> +
> +#define __tty_dinfo_clean(__d)		\
> +	close_safe(&(__d)->lfd),	\
> +	xfree((__d)->tty_data),		\
> +	list_del(&(__d)->list),		\
> +	xfree((__d))

The same.

-- Pavel