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

[Pavel Emelyanov]

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.

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

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

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