[CRIU] [PATCH v2] cr-service: add lazy-pages RPC feature check

[Andrei Vagin]

On Thu, Feb 23, 2017 at 11:17:42AM +0100, Adrian Reber wrote:
> From: Adrian Reber <areber at redhat.com>
> 
> Extend the RPC feature check functionality to also test for lazy-pages
> support. This does not check for certain UFFD features (yet). Right now
> it only checks if kerndat_uffd() returns non-zero.
> 
> The RPC response is now transmitted from the forked process instead of
> encoding all the results into the return code. The parent RPC process
> now only sends an RPC message in the case of a failure.
> 
> Signed-off-by: Adrian Reber <areber at redhat.com>
> ---
>  criu/cr-service.c | 83 ++++++++++++++++++++++++++++++++++++++-----------------
>  images/rpc.proto  |  1 +
>  2 files changed, 59 insertions(+), 25 deletions(-)
> 
> diff --git a/criu/cr-service.c b/criu/cr-service.c
> index 169d637..d1007e7 100644
> --- a/criu/cr-service.c
> +++ b/criu/cr-service.c
> @@ -823,33 +823,21 @@ static int handle_feature_check(int sk, CriuReq * msg)
>  {
>  	CriuResp resp = CRIU_RESP__INIT;
>  	CriuFeatures feat = CRIU_FEATURES__INIT;
> -	bool success = false;
>  	int pid, status;
>  
>  	/* enable setting of an optional message */
>  	feat.has_mem_track = 1;
>  	feat.mem_track = false;
> +	feat.has_lazy_pages = 1;
> +	feat.lazy_pages = false;
>  
> -	/*
> -	 * Check if the requested feature check can be answered.
> -	 *
> -	 * This function is right now hard-coded to memory
> -	 * tracking detection and needs other/better logic to
> -	 * handle multiple feature checks.
> -	 */
> -	if (msg->features->has_mem_track != 1) {
> +	/* Check if the requested feature check can be answered. */
> +	if ((msg->features->has_mem_track != 1) ||
> +	    (msg->features->has_lazy_pages != 1)) {
>  		pr_warn("Feature checking for unknown feature.\n");
>  		goto out;
>  	}
>  
> -	/*
> -	 * From this point on the function will always
> -	 * 'succeed'. If the requested features are supported
> -	 * can be seen if the requested optional parameters are
> -	 * set in the message 'criu_features'.
> -	 */
> -	success = true;
> -
>  	pid = fork();
>  	if (pid < 0) {
>  		pr_perror("Can't fork");
> @@ -857,27 +845,72 @@ static int handle_feature_check(int sk, CriuReq * msg)
>  	}
>  
>  	if (pid == 0) {
> -		int ret = 1;
> +		int ret;
>  
>  		setproctitle("feature-check --rpc");
>  
> -		kerndat_get_dirty_track();
> +		if ((msg->features->has_mem_track == 1) &&
> +		    (msg->features->mem_track == true)) {
> +
> +			feat.mem_track = true;
> +			ret = kerndat_get_dirty_track();
> +
> +			if (ret)
> +				feat.mem_track = false;
> +
> +			if (!kdat.has_dirty_track)
> +				feat.mem_track = false;
> +		}
> +
> +		if ((msg->features->has_lazy_pages == 1) &&
> +		    (msg->features->lazy_pages == true)) {
> +			ret = kerndat_uffd(true);

offtopic: I like an idea when kerndat_* functions fill kdat and return
errors when they meet an unexpected error.

			if (kerndat_uffd())
				goto err;

			if (kdat.laze_pages)
				feat.lazy_pages = true;

> +
> +			/*
> +			 * Not checking for specific UFFD features yet.
> +			 * If no error is returned it is probably
> +			 * enough for basic UFFD functionality. This can
> +			 * be extended in the future for a more detailed
> +			 * UFFD feature check.
> +			 */
> +			if (ret)
> +				feat.lazy_pages = false;
> +			else
> +				feat.lazy_pages = true;
> +		}
>  
> -		if (kdat.has_dirty_track)
> -			ret = 0;
> +		resp.features = &feat;
> +		resp.type = msg->type;
> +		/* The feature check is working, actual results are in resp.features */
> +		resp.success = true;
> +
> +		/*
> +		 * If this point is reached the information about the features
> +		 * is transmitted from the forked CRIU process (here).
> +		 * If an error occured earlier, the feature check response will be
> +		 * be send from the parent process.
> +		 */
> +		ret = send_criu_msg(sk, &resp);
>  
>  		exit(ret);
>  	}
>  
>  	wait(&status);

You don't check an exit code from wait here, so status may be
uninitialized. I recomend to use waitpid if pid is known.

> -	if (!WIFEXITED(status) || WEXITSTATUS(status))
> -		goto out;
> +	if (WIFEXITED(status) && !WEXITSTATUS(status))
> +		/*
> +		 * The child process exited was able to send the answer.
> +		 * Nothing more to do here.
> +		 */
> +		return 0;
>  
> -	feat.mem_track = true;
> +	/*
> +	 * The child process was not able to send an answer. Tell
> +	 * the RPC client that something did not work as expected.
> +	 */
>  out:
>  	resp.features = &feat;

I think we should not set feat here, should we?

>  	resp.type = msg->type;
> -	resp.success = success;
> +	resp.success = false;
>  
>  	return send_criu_msg(sk, &resp);
>  }
> diff --git a/images/rpc.proto b/images/rpc.proto
> index f924b73..f894ae1 100644
> --- a/images/rpc.proto
> +++ b/images/rpc.proto
> @@ -148,6 +148,7 @@ enum criu_req_type {
>   */
>  message criu_features {
>  	optional bool			mem_track	= 1;
> +	optional bool			lazy_pages	= 2;
>  }
>  
>  /*
> -- 
> 2.9.3
>