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

[Pavel Emelyanov]

On 09/05/2012 09:21 PM, Cyrill Gorcunov wrote:
> 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. 

What is the new pid argument for?

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

No garbage in patches please.

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

Please.

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

OK, let's assume we have two tasks:

t1 = {
	pid = 1;
	sid = 1;
	pgid = 1;
	fd[0] = tty_master (index = 3);
};

t2 = {
	pid = 2;
	sid = 1;
	pgid = 1;
	fd[0] = tty_slave (index = 3);
};

and the tty_slave with index 3 will have sid == 1;

What will be there in the image file with ttys? I expect 2 entries, but it looks
like there will be 3.

>>> +	tty_flush(lfd);
>>
>> What for?
> 
> This should flush terminals before dumping, we should not leave them unflushed

Why?

> (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 

Assigned on what?

> it means we're dumping control terminal
> and do not need all parameters but just a few.
> 

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

OK. Don't forget to put this in todo list.

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

OK. Then s/No sid or group/We can't setup sid or group/, s/should/will/ please.