[CRIU] [PATCH 4/4] cr-service: use inherit-fd keys to ask for FD
Adrian Reber
areber at redhat.com
Tue Aug 7 23:44:01 MSK 2018
On Mon, Aug 06, 2018 at 02:01:05PM -0700, Andrei Vagin wrote:
> On Mon, Aug 06, 2018 at 04:55:00PM +0000, Adrian Reber wrote:
> > From: Adrian Reber <areber at redhat.com>
> >
> > Using inherit-fd in RPC mode uses the notify mechanism to ask the RPC
> > client for the real FD.
> >
> > The basic workflow of inherit-fd in RPC mode is:
> >
> > * RPC clients fills in inherit-fd field:
> > * inheritFd.Key = proto.String(path.Base(nsPath))
> > * inheritFd.Fd is irrelevant as it will be transmitted later
> > * CRIU receives RPC messages and calls inherit_fd_add_rpc()
> > * this transmits the inheritFd.Key back to the RPC client
> > * RPC client uses unix domain socket to send the real FD
> > to CRIU
> >
> > Unfortunately this is more complicated as runc already uses
> > inheritFd.Fd to send the file descriptors for 0, 1, 2. As those are
> > always stdin, stdout, stderr is just worked even if the transmitted
> > values via RPC were not actual FDs but only numbers.
> >
> > To handle this, CRIU still accepts inherit-fds directly as FDs if it is
> > 0, 1, 2. This is a compatibility hack to avoid breaking runc.
> >
> > Signed-off-by: Adrian Reber <areber at redhat.com>
> > ---
> > criu/action-scripts.c | 10 ++++++---
> > criu/cr-service.c | 38 ++++++++++++++++++++++++++++++++---
> > criu/files.c | 10 +++++++++
> > criu/include/action-scripts.h | 4 ++--
> > criu/include/files.h | 1 +
> > criu/tty.c | 2 +-
> > 6 files changed, 56 insertions(+), 9 deletions(-)
> >
> > diff --git a/criu/action-scripts.c b/criu/action-scripts.c
> > index 4e9eb65cf..1a7523527 100644
> > --- a/criu/action-scripts.c
> > +++ b/criu/action-scripts.c
> > @@ -100,7 +100,11 @@ static int run_shell_scripts(const char *action)
> > return retval;
> > }
> >
> > -int rpc_send_fd(enum script_actions act, int fd)
> > +/*
> > + * The name of this function is misleading. Right now it only
> > + * sends an FD in one of three possible use cases.
> > + */
> > +int rpc_send_fd(enum script_actions act, int fd, char *key)
> > {
> > const char *action = action_names[act];
> > int rpc_sk;
> > @@ -113,7 +117,7 @@ int rpc_send_fd(enum script_actions act, int fd)
> > return -1;
> >
> > pr_debug("\tRPC\n");
> > - return send_criu_rpc_script(act, (char *)action, rpc_sk, fd);
> > + return send_criu_rpc_script(act, (char *)action, rpc_sk, fd, key);
> > }
> >
> > int run_scripts(enum script_actions act)
> > @@ -127,7 +131,7 @@ int run_scripts(enum script_actions act)
> > return 0;
> >
> > if (scripts_mode == SCRIPTS_RPC) {
> > - ret = rpc_send_fd(act, -1);
> > + ret = rpc_send_fd(act, -1, NULL);
> > goto out;
> > }
> >
> > diff --git a/criu/cr-service.c b/criu/cr-service.c
> > index 643aba9cf..45d8b3f44 100644
> > --- a/criu/cr-service.c
> > +++ b/criu/cr-service.c
> > @@ -188,9 +188,10 @@ int send_criu_restore_resp(int socket_fd, bool success, int pid)
> > return send_criu_msg(socket_fd, &msg);
> > }
> >
> > -int send_criu_rpc_script(enum script_actions act, char *name, int sk, int fd)
> > +int send_criu_rpc_script(enum script_actions act, char *name, int sk, int fd, char *key)
> > {
> > int ret;
> > + int ret_fd = 0;
> > CriuResp msg = CRIU_RESP__INIT;
> > CriuReq *req;
> > CriuNotify cn = CRIU_NOTIFY__INIT;
> > @@ -211,6 +212,15 @@ int send_criu_rpc_script(enum script_actions act, char *name, int sk, int fd)
> > cn.has_pid = true;
> > cn.pid = root_item->pid->real;
> > break;
> > + case ACT_REQ_INHERIT_FD:
> > + /*
> > + * Sending a 'ACT_REQ_INHERIT_FD' notify message
> > + * only makes sense when 'key' is set.
> > + */
> > + if (!key)
> > + return -1;
> > + cn.inherit_fd_key = key;
> > + break;
> > default:
> > break;
> > }
> > @@ -219,6 +229,18 @@ int send_criu_rpc_script(enum script_actions act, char *name, int sk, int fd)
> > if (ret < 0)
> > return ret;
> >
> > + if (act == ACT_REQ_INHERIT_FD) {
> > + /*
> > + * Sending a ACT_REQ_INHERIT_FD notify message means, that CRIU
> > + * expects to get a FD from the RPC client.
> > + */
> > + ret_fd = recv_fd(sk);
> > + if (ret_fd <= 0) {
> > + pr_perror("recv_fd error\n");
> > + return -1;
> > + }
> > + }
> > +
> > ret = recv_criu_msg(sk, &req);
> > if (ret < 0)
> > return ret;
> > @@ -229,7 +251,7 @@ int send_criu_rpc_script(enum script_actions act, char *name, int sk, int fd)
> > }
> >
> > criu_req__free_unpacked(req, NULL);
> > - return 0;
> > + return ret_fd;
> > }
> >
> > static char images_dir[PATH_MAX];
> > @@ -481,11 +503,21 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
> > }
> >
> > if (req->n_inherit_fd && !opts.swrk_restore) {
> > + /* TODO: Is this really still true, even with callback handling for inherit_fd */
> > pr_err("inherit_fd is not allowed in standalone service\n");
> > goto err;
> > }
> > for (i = 0; i < req->n_inherit_fd; i++) {
> > - if (inherit_fd_add(req->inherit_fd[i]->fd, req->inherit_fd[i]->key))
> > + /*
> > + * For the FDs 0, 1, 2 this falls back to old, non callbacb
> > + * inherit_fd mode. At some point in the future this can be removed.
> > + */
> > + if (req->inherit_fd[i]->fd >=0 && req->inherit_fd[i]->fd <= 2) {
>
> We can add the following patch and handle all non-negative fd
>
> diff --git a/images/rpc.proto b/images/rpc.proto
> index 33ef3302b..04b933674 100644
> --- a/images/rpc.proto
> +++ b/images/rpc.proto
> @@ -25,7 +25,7 @@ message join_namespace {
>
> message inherit_fd {
> required string key = 1;
> - required int32 fd = 2;
> + required int32 fd = 2 [default = -1];
> };
>
> message cgroup_root {
Hmm, how could this be used? With my current CRIU and runc patches, CRIU
would look at 'fd' at only do something special if it is 0, 1, 2. See
below. If it is > 2 or < 0 would be the same. CRIU just ignores all
other FD values as they have to be retrieved via the new callback.
> > + if (inherit_fd_add(req->inherit_fd[i]->fd, req->inherit_fd[i]->key))
> > + goto err;
> > + continue;
> > + }
> > + if (inherit_fd_add_rpc(req->inherit_fd[i]->key))
>
> I think you misunderstood me. I was thinking to request an inherit fd from
> inherit_fd_lookup_id() to avoid fd clashes on restore
I still do not get it. How is this related to inherit_fd_lookup_id()
> (inherit_fd_resolve_clash). It will work this way too, but in case of
> swrk, it will be more efficiant just to run criu swrk will all required
> fds (this way is handled in a previous "if").
I also do not understand this. 'criu swrk with all required fds'? Like
runc starts CRIU in swrk mode, with an additional parameter as an FD?
Right now runc does 'criu swrk 3'. Where '3' is the RPC data channel,
right? And you say to add more FDs after the '3'?
And what do you mean with 'this way is handled in a previous "if"'? No
idea to what this refers?
Sorry, I am lost.
Adrian
> > goto err;
> > }
> >
> > diff --git a/criu/files.c b/criu/files.c
> > index 4ed84305e..6d71bb9df 100644
> > --- a/criu/files.c
> > +++ b/criu/files.c
> > @@ -26,6 +26,7 @@
> > #include "sockets.h"
> > #include "pstree.h"
> > #include "tty.h"
> > +#include "action-scripts.h"
> > #include "pipes.h"
> > #include "fifo.h"
> > #include "eventfd.h"
> > @@ -1572,6 +1573,15 @@ int inherit_fd_parse(char *optarg)
> > return inherit_fd_add(fd, cp);
> > }
> >
> > +int inherit_fd_add_rpc(char *key)
> > +{
> > + int fd;
> > + fd = rpc_send_fd(ACT_REQ_INHERIT_FD, -1, key);
> > + if (fd <= 0)
> > + return -1;
> > + return inherit_fd_add(fd, key);
> > +}
> > +
> > int inherit_fd_add(int fd, char *key)
> > {
> > struct inherit_fd *inh;
> > diff --git a/criu/include/action-scripts.h b/criu/include/action-scripts.h
> > index 4b90feb92..bd96e256d 100644
> > --- a/criu/include/action-scripts.h
> > +++ b/criu/include/action-scripts.h
> > @@ -23,7 +23,7 @@ enum script_actions {
> > extern int add_script(char *path);
> > extern int add_rpc_notify(int sk);
> > extern int run_scripts(enum script_actions);
> > -extern int rpc_send_fd(enum script_actions, int fd);
> > -extern int send_criu_rpc_script(enum script_actions act, char *name, int sk, int fd);
> > +extern int rpc_send_fd(enum script_actions, int fd, char *key);
> > +extern int send_criu_rpc_script(enum script_actions act, char *name, int sk, int fd, char *key);
> >
> > #endif /* __CR_ACTION_SCRIPTS_H__ */
> > diff --git a/criu/include/files.h b/criu/include/files.h
> > index 75757bd1d..9cffe960c 100644
> > --- a/criu/include/files.h
> > +++ b/criu/include/files.h
> > @@ -188,6 +188,7 @@ extern int dump_unsupp_fd(struct fd_parms *p, int lfd,
> > char *more, char *info, FdinfoEntry *);
> >
> > extern int inherit_fd_parse(char *optarg);
> > +extern int inherit_fd_add_rpc(char *key);
> > extern int inherit_fd_add(int fd, char *key);
> > extern void inherit_fd_log(void);
> > extern int inherit_fd_resolve_clash(int fd);
> > diff --git a/criu/tty.c b/criu/tty.c
> > index 30e3d7288..676ad888c 100644
> > --- a/criu/tty.c
> > +++ b/criu/tty.c
> > @@ -984,7 +984,7 @@ static int pty_open_unpaired_slave(struct file_desc *d, struct tty_info *slave)
> > unlock_pty(master);
> >
> > if (opts.orphan_pts_master &&
> > - rpc_send_fd(ACT_ORPHAN_PTS_MASTER, master) == 0) {
> > + rpc_send_fd(ACT_ORPHAN_PTS_MASTER, master, NULL) == 0) {
> >
> > fd = open_tty_reg(slave->reg_d, slave->tfe->flags);
> > if (fd < 0) {
> > --
> > 2.18.0
> >
More information about the CRIU
mailing list