[CRIU] [PATCH 1/2] RPC: add version check interface

[Adrian Reber]

On Fri, Mar 31, 2017 at 02:39:42PM -0700, Andrei Vagin wrote:
> On Wed, Mar 15, 2017 at 03:26:17PM +0100, Adrian Reber wrote:
> > From: Adrian Reber <areber at redhat.com>
> > 
> > Instead of parsing the output of 'criu -V' this offers a RPC interface
> > to get CRIU's version. In a follow up patch a test script is included
> > to use the new interface:
> > 
> >  ./version.py
> >  Connecting to CRIU in swrk mode to check the version:
> >  RPC: Success
> >  CRIU major 2
> >  CRIU minor 12
> >  CRIU gitid v2.12-635-g6d3ae4d
> > 
> > This change exports the following version fields:
> >  * major
> >  * minor
> >  * gitid
> >  * sublevel (optional)
> >  * extra (optional)
> >  * name (optional)
> > 
> > A 'gitid' of '0' (the '0' is a string) means that it is not built from a
> > git checkout.
> > 
> > Signed-off-by: Adrian Reber <areber at redhat.com>
> > ---
> >  criu/cr-service.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  images/rpc.proto  | 14 +++++++++++
> >  2 files changed, 83 insertions(+)
> > 
> > diff --git a/criu/cr-service.c b/criu/cr-service.c
> > index 78142b0..7e17199 100644
> > --- a/criu/cr-service.c
> > +++ b/criu/cr-service.c
> > @@ -15,6 +15,7 @@
> >  #include <arpa/inet.h>
> >  #include <sched.h>
> >  
> > +#include "version.h"
> >  #include "crtools.h"
> >  #include "cr_options.h"
> >  #include "external.h"
> > @@ -806,11 +807,76 @@ static int chk_keepopen_req(CriuReq *msg)
> >  		return 0;
> >  	else if (msg->type == CRIU_REQ_TYPE__FEATURE_CHECK)
> >  		return 0;
> > +	else if (msg->type == CRIU_REQ_TYPE__VERSION)
> > +		return 0;
> >  
> >  	return -1;
> >  }
> >  
> >  /*
> > + * Return the version information, depending on the information
> > + * available in version.h
> > + */
> > +static int handle_version(int sk, CriuReq * msg)
> > +{
> > +	CriuResp resp = CRIU_RESP__INIT;
> > +	CriuVersion version = CRIU_VERSION__INIT;
> > +	int pid, status;
> > +	int ret;
> > +
> > +
> > +	pid = fork();
> 
> Why do we need a new process here?

Good question ;-) This was one of the points were I was unsure about the
right solution. Pavel mentioned, that the fork is used for RPC functions
which might exit cleanly. The main problem was that it is not clear how
protobuf-c handles strings. Further down I am strdup()-ing the version
strings into the protobuf message. The protobuf-c documentation
regarding string handling says: TODO

https://github.com/protobuf-c/protobuf-c/wiki/Examples#strings

So I don't know if strdup() is the right tool for protobuf-c. It works,
but it leaves strings allocated which I don't know when or where to
free. Therefore I am going the easy way. fork() strdup() forget.

Do you know what the correct way to handle strings with protobuf-c is?
Then I could remove the fork().

> > +	if (pid < 0) {
> > +		pr_perror("Can't fork");
> > +		goto out;
> > +	}
> > +
> > +	if (pid == 0) {
> > +		/* This assumes we will always have a major and minor version */
> > +		version.major = CRIU_VERSION_MAJOR;
> > +		version.minor = CRIU_VERSION_MINOR;
> > +		version.gitid = xstrdup(CRIU_GITID);
> > +#ifdef CRIU_VERSION_SUBLEVEL
> > +		version.has_sublevel = 1;
> > +		version.sublevel = CRIU_VERSION_SUBLEVEL;
> > +#endif
> > +#ifdef CRIU_VERSION_EXTRA
> > +		version.has_extra = 1;
> > +		version.extra = CRIU_VERSION_EXTRA;
> > +#endif
> > +#ifdef CRIU_VERSION_NAME
> > +		/* This is not actually exported in version.h */
> > +		version.name = xstrdup(CRIU_VERSION_NAME);
> > +#endif
> > +		resp.type = msg->type;
> > +		resp.success = true;
> > +		resp.version = &version;
> > +		return send_criu_msg(sk, &resp);
> > +	}
> > +
> > +	ret = waitpid(pid, &status,  0);
> > +	if (ret == -1)
> > +		goto out;
> > +
> > +	if (WIFEXITED(status) && !WEXITSTATUS(status))
> > +		/*
> > +		 * The child process exited and was able to send the answer.
> > +		 * Nothing more to do here.
> > +		 */
> > +		return 0;
> > +
> > +	/*
> > +	 * The child process was not able to send an answer. Tell
> > +	 * the RPC client that something did not work as expected.
> > +	 */
> > +out:
> > +	resp.type = msg->type;
> > +	resp.success = false;
> > +
> > +	return send_criu_msg(sk, &resp);
> > +}
> > +
> > +/*
> >   * Generic function to handle CRIU_REQ_TYPE__FEATURE_CHECK.
> >   *
> >   * The function will have resp.success = true for most cases
> > @@ -1004,6 +1070,9 @@ more:
> >  	case CRIU_REQ_TYPE__FEATURE_CHECK:
> >  		ret = handle_feature_check(sk, msg);
> >  		break;
> > +	case CRIU_REQ_TYPE__VERSION:
> > +		ret = handle_version(sk, msg);
> > +		break;
> >  
> >  	default:
> >  		send_criu_err(sk, "Invalid req");
> > diff --git a/images/rpc.proto b/images/rpc.proto
> > index f894ae1..bfb8dfc 100644
> > --- a/images/rpc.proto
> > +++ b/images/rpc.proto
> > @@ -140,6 +140,8 @@ enum criu_req_type {
> >  	CPUINFO_CHECK	= 8;
> >  
> >  	FEATURE_CHECK	= 9;
> > +
> > +	VERSION		= 10;
> >  }
> >  
> >  /*
> > @@ -193,4 +195,16 @@ message criu_resp {
> >  	optional int32			cr_errno	= 7;
> >  	optional criu_features		features	= 8;
> >  	optional string			cr_errmsg	= 9;
> > +	optional criu_version		version		= 10;
> > +}
> > +
> > +/* Answer for criu_req_type.VERSION requests */
> > +message criu_version {
> > +	required int32			major		= 1;
> > +	required int32			minor		= 2;
> > +	/* if gitid is '0' then it is not from a git checkout */
> > +	required string			gitid		= 3;
> 
> Maybe it should be optional? ^^^^^

It is always set by the Makefile. sublevel and extra have a ifneq but
the gitid is always specified. It is either the string '0' or the
actual gitid. So, yes it could be optional but I tried to follow what
the Makefile gives me.


> > +	optional int32			sublevel	= 4;
> > +	optional int32			extra		= 5;
> > +	optional string			name		= 6;
> >  }
> > -- 
> > 2.9.3

		Adrian