[CRIU] [PATCH 2/2]v2 service: add support for check request

[Ruslan Kuprieiev]

On 19.11.2013 22:40, Pavel Emelyanov wrote:
> On 11/20/2013 02:32 AM, Ruslan Kuprieiev wrote:
>> On 19.11.2013 21:37, Pavel Emelyanov wrote:
>>> On 11/19/2013 06:03 PM, Ruslan Kuprieiev wrote:
>>>> On 19.11.2013 12:10, Pavel Emelyanov wrote:
>>>>> On 11/19/2013 03:44 PM, Ruslan Kuprieiev wrote:
>>>>>> On 19.11.2013 11:36, Pavel Emelyanov wrote:
>>>>>>> On 11/18/2013 06:25 PM, Ruslan Kuprieiev wrote:
>>>>>>>> Signed-off-by: Ruslan Kuprieiev <kupruser at gmail.com>
>>>>>>>> ---
>>>>>>>>      cr-service.c | 37 +++++++++++++++++++++++++++++++++++++
>>>>>>>>      1 file changed, 37 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/cr-service.c b/cr-service.c
>>>>>>>> index 65710fd..c337fce 100644
>>>>>>>> --- a/cr-service.c
>>>>>>>> +++ b/cr-service.c
>>>>>>>> @@ -217,6 +217,41 @@ exit:
>>>>>>>>      	return success ? 0 : 1;
>>>>>>>>      }
>>>>>>>>      
>>>>>>>> +/*
>>>>>>>> + * Only checks criu ability to work in this enviroment.
>>>>>>>> + */
>>>>>>>> +static int check_using_req(int sk, CriuOpts *req)
>>>>>>>> +{
>>>>>>>> +	bool success = false;
>>>>>>>> +	CriuResp msg = CRIU_RESP__INIT;
>>>>>>>> +	CriuCheckResp resp = CRIU_CHECK_RESP__INIT;
>>>>>>>> +
>>>>>>>> +	if (setup_opts_from_req(sk, req) == -1) {
>>>>>>>> +		pr_perror("Arguments treating fail");
>>>>>>>> +		goto exit;
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>> +	if (cr_check() < 0) {
>>>>>>>> +		pr_perror("The kernel support isn't up-to-date");
>>>>>>>> +		goto exit;
>>>>>>>> +	}
>> Here, if cr_check() hasn't returned 0, it means that resp.kernel =
>> false, and it is by default false. If cr_check has returned 0, than we
>> set resp.kernel to true.
> And success to true as well. Show me how resp.success can be != resp.kernel.
>
>>>>>>>> +
>>>>>>>> +	resp.kernel = true;
>>>>>>> Always true argument is pointless.
>>>>>> But resp.kernel = false after init. If cr_check() does not fail, we set
>>>>>> resp.kernel to true, and if cr_check() does fail - resp.kernel = false.
>>>>> So we have two types of responses:
>>>>>
>>>>> * kernel = true,  success = true  for OK
>>>>> * kernel = false, success = false for not-OK
>>>>>
>>>>> Why is just success = <success-or-not> not enough?
>>>> Here is what we've got:
>>>>
>>>> -- resp.success == false  -- options are wrong or something bad happened
>>>> and check wasn't done
>>>>
>>>> -- resp.success == true and resp.check.kernel == true -- check was done
>>>> and kernel features are ok
>>>>
>>>> -- resp.success == true and resp.check.kernel == false -- check was done
>>>> and kernel features are not ok
>>> I don't see this in the code -- resp.success = true and resp.kernel = true
>>> are set together in one place.

Yes, it can't be such for now. But it is a preparation for further work. 
Or you are not sure about adding more functionality to this request? We 
may add check_dump/check_restore/check_images? as a separate ones. What 
do you think?

>>>> And also we may easily add some more fields for more complete system
>>>> state analysis.
>>>>
>>>>>>>> +
>>>>>>>> +	success = true;
>>>>>>>> +exit:
>>>>>>>> +	msg.type = CRIU_REQ_TYPE__CHECK;
>>>>>>>> +	msg.success = success;
>>>>>>>> +	msg.check = &resp;
>>>>>>>> +
>>>>>>>> +	if (send_criu_msg(sk, &msg) == -1) {
>>>>>>>> +		pr_perror("Can't send response");
>>>>>>>> +		success = false;
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>> +	return success ? 0 : 1;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>      static int cr_service_work(int sk)
>>>>>>>>      {
>>>>>>>>      	CriuReq *msg = 0;
>>>>>>>> @@ -233,6 +268,8 @@ static int cr_service_work(int sk)
>>>>>>>>      		return dump_using_req(sk, msg->opts);
>>>>>>>>      	case CRIU_REQ_TYPE__RESTORE:
>>>>>>>>      		return restore_using_req(sk, msg->opts);
>>>>>>>> +	case CRIU_REQ_TYPE__CHECK:
>>>>>>>> +		return check_using_req(sk, msg->opts);
>>>>>>>>      
>>>>>>>>      	default: {
>>>>>>>>      		CriuResp resp = CRIU_RESP__INIT;
>>>>>>>>
>>>>>> .
>>>>>>
>>>> .
>>>>
>> .
>>
>