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

Submitted by Kirill Tkhai on June 16, 2016, 1:53 p.m.

Details

Message ID 146608521809.15781.13452111533671053615.stgit@pro
State Rejected
Series "Support for packet's msg_name in receive queue of promiscous DGRAM sockets"
Headers show

Commit Message

Kirill Tkhai June 16, 2016, 1:53 p.m.
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@virtuozzo.com>
---
 criu/files.c         |    9 +++++++++
 criu/include/files.h |    3 +++
 2 files changed, 12 insertions(+)

Patch hide | download patch | download mbox

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);
+	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 */

Comments

Pavel Emelianov June 28, 2016, 12:28 p.m.
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@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?

> +	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@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu
> .
>
Kirill Tkhai June 28, 2016, 12:59 p.m.
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@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.

>> +	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@openvz.org
>> https://lists.openvz.org/mailman/listinfo/criu
>> .
>>
>
Pavel Emelianov July 4, 2016, 5:28 p.m.
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@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?

>>> +	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@openvz.org
>>> https://lists.openvz.org/mailman/listinfo/criu
>>> .
>>>
>>
> .
>
Pavel Emelianov July 13, 2016, 1:50 p.m.
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@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.

>>>>>> +	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@openvz.org
>>>>>> https://lists.openvz.org/mailman/listinfo/criu
>>>>>> .
>>>>>>
>>>>>
>>>> .
>>>>
>>>
> .
>
Kirill Tkhai July 13, 2016, 2:02 p.m.
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@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@openvz.org
>>>>>>> https://lists.openvz.org/mailman/listinfo/criu
>>>>>>> .
>>>>>>>
>>>>>>
>>>>> .
>>>>>
>>>>
>> .
>>
>
Pavel Emelianov July 14, 2016, 10:50 a.m.
On 07/13/2016 05:02 PM, Kirill Tkhai wrote:
> 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@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.

Because more stages we have it becomes difficult to name them :) and (which is
more important) to understand __why__ we need that many. Also doing a stage
with all no-op operations in it (you add 4th stage that will be no-op typically)
takes time.

Maybe it's time to re-think the fd restore and introduce the way to "generate"
as many stages as we need? Look at the 'bool required' field of the stage --
two of them are really optional and if we start with the single stage and
let the desc callback request for another stage, we can add new ones without
explicit name and description.

-- Pavel