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

[Pavel Emelyanov]

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

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