[CRIU] [PATCH 2/4] service: page-server: return port back to user

[Ruslan Kuprieiev]

On 17.09.2014 17:59, Pavel Emelyanov wrote:
> On 09/17/2014 06:53 PM, Ruslan Kuprieiev wrote:
>> On 17.09.2014 17:44, Pavel Emelyanov wrote:
>>> On 09/17/2014 11:25 AM, Ruslan Kuprieiev wrote:
>>>> Signed-off-by: Ruslan Kuprieiev <kupruser at gmail.com>
>>>> ---
>>>>    cr-service.c | 44 ++++++++++++++++++++++++++++----------------
>>>>    1 file changed, 28 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/cr-service.c b/cr-service.c
>>>> index d144f47..2af6e8f 100644
>>>> --- a/cr-service.c
>>>> +++ b/cr-service.c
>>>> @@ -505,7 +505,7 @@ static int pre_dump_loop(int sk, CriuReq *msg)
>>>>    
>>>>    static int start_page_server_req(int sk, CriuOpts *req)
>>>>    {
>>>> -	int ret, pid, start_pipe[2];
>>>> +	int ret = -1, pid, start_pipe[2];
>>>>    	ssize_t count;
>>>>    	bool success = false;
>>>>    	CriuResp resp = CRIU_RESP__INIT;
>>>> @@ -532,28 +532,40 @@ static int start_page_server_req(int sk, CriuOpts *req)
>>>>    
>>>>    		pr_debug("Starting page server\n");
>>>>    
>>>> -		ret = cr_page_server(true, start_pipe[1]);
>>>> +		pid = cr_page_server(true, start_pipe[1]);
>>>> +		if (pid <= 0)
>>>> +			goto out_ch;
>>>> +
>>>> +		count = write(start_pipe[1], &opts.ps_port, sizeof(opts.ps_port));
>>>> +		if (count != sizeof(opts.ps_port))
>>>> +			goto out_ch;
>>>> +
>>>> +		ret = pid;
>>>>    out_ch:
>>>> -		count = write(start_pipe[1], &ret, sizeof(ret));
>>>> +		if (ret < 0 && pid > 0)
>>>> +			kill(pid, SIGKILL);
>>>>    		close(start_pipe[1]);
>>>> -		if (count != sizeof(ret))
>>>> -			exit(1);
>>>> -		exit(0);
>>>> +		exit(ret);
>>>>    	}
>>>>    
>>>>    	close(start_pipe[1]);
>>>> -	wait(NULL);
>>>> -	ret = -1;
>>>> -	count = read(start_pipe[0], &ret, sizeof(ret));
>>>> -	if (count != sizeof(ret))
>>>> -		success = false;
>>>> -	else if (ret > 0) {
>>>> -		success = true;
>>>> -		ps.has_pid = true;
>>>> -		ps.pid = ret;
>>>> -		resp.ps = &ps;
>>>> +	wait(&ret);
>>>> +	if (ret <= 0)
>>>> +		goto out;
>>>> +
>>>> +	count = read(start_pipe[0], &opts.ps_port, sizeof(opts.ps_port));
>>>> +	if (count != sizeof(opts.ps_port)) {
>>>> +		kill(ret, SIGKILL);
>>>> +		goto out;
>>>>    	}
>>>>    
>>>> +	success = true;
>>>> +	ps.has_pid = true;
>>>> +	ps.pid = ret;
>>> The ret here would be ... what? Last time it was passed
>>> as pointer to wait() call, but its argument is a) not pointer
>>> to store pid and b) this task's child is not page server task.
>> Last time exit value of child wasn't used at all:
>>
>> -	wait(NULL);
>>
>> And ps pid was transfered through pipe.
> Sure, because it's a working way for a task to know the pid of
> his _grand_ child.

Oh!

>> And yes, sorry, i've used a bad way to transfer ps pid.
>> Will fix.
> When fixing this, don't use two subsequent writes of two int-s.
> Introduce a struct, pack pid and port there and push it into
> pipe with one call.

Ok.

> And, btw, write a test, that would catch the absence of proper
> page server pid. The existing one doesn't do it.

Ok.

Thanks!

>>>> +	ps.has_port = true;
>>>> +	ps.port = opts.ps_port;
>>>> +	resp.ps = &ps;
>>>> +
>>>>    	pr_debug("Page server started\n");
>>>>    out:
>>>>    	resp.type = CRIU_REQ_TYPE__PAGE_SERVER;
>>>>
>> .
>>