[CRIU] [PATCH 1/2] RPC: add version check interface
Andrei Vagin
avagin at virtuozzo.com
Tue Apr 4 23:40:38 PDT 2017
On Tue, Apr 04, 2017 at 11:04:01AM +0200, Adrian Reber wrote:
> 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().
size_t criu_resp__pack
(const CriuResp *message,
uint8_t *out)
here we can sett that the origin message is marked as const, so protobuf
is not going to change it.
I think we can set these strings without xstrdup and we don't need an
extra 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.
I think we can make it optional, because criu -V prints gitid only if it is
set.
case 'V':
pr_msg("Version: %s\n", CRIU_VERSION);
if (strcmp(CRIU_GITID, "0"))
pr_msg("GitID: %s\n", CRIU_GITID);
return 0;
>
>
> > > + optional int32 sublevel = 4;
> > > + optional int32 extra = 5;
> > > + optional string name = 6;
> > > }
> > > --
> > > 2.9.3
>
> Adrian
More information about the CRIU
mailing list