[CRIU] [PATCH v5 05/11] files: Add fd_open_state::finalize method()

[Kirill Tkhai]

On 13.07.2016 16:50, Pavel Emelyanov wrote:
> On 07/12/2016 10:40 AM, Kirill Tkhai wrote:
>>
>>
>> On 12.07.2016 10:22, Kirill Tkhai wrote:
>>>
>>>
>>> On 04.07.2016 20:28, Pavel Emelyanov wrote:
>>>> On 06/28/2016 03:59 PM, Kirill Tkhai wrote:
>>>>> On 28.06.2016 15:28, Pavel Emelyanov wrote:
>>>>>> On 06/16/2016 04:53 PM, Kirill Tkhai wrote:
>>>>>>> Add a method, which allows to close temporary files,
>>>>>>> used on restore stage. This will be used in next patches.
>>>>>>>
>>>>>>> v5: New
>>>>>>>
>>>>>>> Signed-off-by: Kirill Tkhai <ktkhai at virtuozzo.com>
>>>>>>> ---
>>>>>>>  criu/files.c         |    9 +++++++++
>>>>>>>  criu/include/files.h |    3 +++
>>>>>>>  2 files changed, 12 insertions(+)
>>>>>>>
>>>>>>> diff --git a/criu/files.c b/criu/files.c
>>>>>>> index b9fdbdc..036e054 100644
>>>>>>> --- a/criu/files.c
>>>>>>> +++ b/criu/files.c
>>>>>>> @@ -676,6 +676,7 @@ static int collect_fd(int pid, FdinfoEntry *e, struct rst_info *rst_info)
>>>>>>>  	futex_init(&new_le->real_pid);
>>>>>>>  	new_le->pid = pid;
>>>>>>>  	new_le->fe = e;
>>>>>>> +	new_le->flags = 0;
>>>>>>>  
>>>>>>>  	fdesc = find_file_desc(e);
>>>>>>>  	if (fdesc == NULL) {
>>>>>>> @@ -854,12 +855,14 @@ static int open_transport_fd(int pid, struct fdinfo_list_entry *fle);
>>>>>>>  static int open_fd(int pid, struct fdinfo_list_entry *fle);
>>>>>>>  static int receive_fd(int pid, struct fdinfo_list_entry *fle);
>>>>>>>  static int post_open_fd(int pid, struct fdinfo_list_entry *fle);
>>>>>>> +static int finalize_fd(int pid, struct fdinfo_list_entry *fle);
>>>>>>>  
>>>>>>>  static struct fd_open_state states[] = {
>>>>>>>  	{ "prepare",		open_transport_fd,	true,},
>>>>>>>  	{ "create",		open_fd,		true,},
>>>>>>>  	{ "receive",		receive_fd,		false,},
>>>>>>>  	{ "post_create",	post_open_fd,		false,},
>>>>>>> +	{ "finalize",		finalize_fd,		true,},
>>>>>>>  };
>>>>>>>  
>>>>>>>  #define want_recv_stage()	do { states[2].required = true; } while (0)
>>>>>>> @@ -993,6 +996,12 @@ static int post_open_fd(int pid, struct fdinfo_list_entry *fle)
>>>>>>>  	return d->ops->post_open(d, fle->fe->fd);
>>>>>>>  }
>>>>>>>  
>>>>>>> +static int finalize_fd(int pid, struct fdinfo_list_entry *fle)
>>>>>>> +{
>>>>>>> +	if (fle->flags & FD_LE_GHOST)
>>>>>>> +		close(fle->fe->fd);
>>>>>>
>>>>>> Hm... Why can't we just do this in post_open_fd-s?
>>>>>
>>>>> Promiscous queues are restored in post-open stage.
>>>>> All ghost fle must be alive at this moment.
>>>>
>>>> OK, so we call post_open callbacks and close these guys right after it. No?
>>>
>>> Sure.
>>
>> We have ghost SocketA and alive SocketB. SocketB needs SocketA to restore its queue.
>> The first socket in file list is SocketA, the second is SocketB.
>>
>> So, when we call close() in SocketA post_open(), SocketB can't use it in its post_open()
>> and can't restore its queue.
> 
> I see two solutions:
> 
> 1. So put the socketB before socketA in the list
> 2. After the ->post open stage is complete walk the fdlist again and
>    close the ghost ones WITHOUT the explicit "finalize" stage.

I don't think it's good, because it makes the code more complicated and
difficult to understand. Two above are crutches, and it seems it's not
a place it costs to use one.

Ghost engine is generic and is not socket's, so they should be closed
in generic way too.

Why are you against finalize stage? It's simple and easy to support.

>>>>>>> +	return 0;
>>>>>>> +}
>>>>>>>  
>>>>>>>  static int serve_out_fd(int pid, int fd, struct file_desc *d)
>>>>>>>  {
>>>>>>> diff --git a/criu/include/files.h b/criu/include/files.h
>>>>>>> index 5e3d6dc..06a1f98 100644
>>>>>>> --- a/criu/include/files.h
>>>>>>> +++ b/criu/include/files.h
>>>>>>> @@ -62,6 +62,7 @@ extern int fill_fdlink(int lfd, const struct fd_parms *p, struct fd_link *link);
>>>>>>>  
>>>>>>>  struct file_desc;
>>>>>>>  
>>>>>>> +
>>>>>>>  struct fdinfo_list_entry {
>>>>>>>  	struct list_head	desc_list;	/* To chain on  @fd_info_head */
>>>>>>>  	struct file_desc	*desc;		/* Associated file descriptor */
>>>>>>> @@ -70,6 +71,8 @@ struct fdinfo_list_entry {
>>>>>>>  	int			pid;
>>>>>>>  	futex_t			real_pid;
>>>>>>>  	FdinfoEntry		*fe;
>>>>>>> +#define FD_LE_GHOST		0x1
>>>>>>> +	unsigned		flags;
>>>>>>>  };
>>>>>>>  
>>>>>>>  /* reports whether fd_a takes prio over fd_b */
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> CRIU mailing list
>>>>>>> CRIU at openvz.org
>>>>>>> https://lists.openvz.org/mailman/listinfo/criu
>>>>>>> .
>>>>>>>
>>>>>>
>>>>> .
>>>>>
>>>>
>> .
>>
>