[CRIU] [PATCH 2/2] fd: Factor out inheriting FDs code
Saied Kazemi
saied at google.com
Tue Dec 30 10:01:48 PST 2014
The patch looks good and it's definitely an improvement. I made sure
that it still works for my test cases. The only question that I have
is shouldn't we return false in inherited_fd() if dup() fails?
+ *fd_p = dup(i_fd);
+ if (*fd_p < 0)
+ pr_perror("Inherit fd DUP failed");
+ else
+ pr_info("File %s will be restored from fd %d duped "
+ "from inherit fd %d\n", id_str, *fd_p, i_fd);
--Saied
On Tue, Dec 30, 2014 at 5:29 AM, Pavel Emelyanov <xemul at parallels.com> wrote:
> We have two places where we lookup the inherited-fd list
> by name and dup() the descriptor found. I propose to factor
> out this piece in a single inherited_fd() call. When
> we will want to support inheritance for sockets or any
> other files we'll simply add the inherited_fd() call
> there.
>
> I'm also thinking about moving the call to inherited_fd
> into generic level, but the open_path() routine doesn't
> allow to do it in a simple manner.
>
> Also we have not yet finished issue with files-vs-inodes
> mapping. Keeping all the logic in one function should
> make the solution simpler.
>
> Signed-off-by: Pavel Emelyanov <xemul at parallels.com>
> ---
> files-reg.c | 27 ++++++++++++---------------
> files.c | 24 +++++++++++++++++++++++-
> include/files.h | 4 +++-
> pipes.c | 40 ++++++++++++----------------------------
> 4 files changed, 50 insertions(+), 45 deletions(-)
>
> diff --git a/files-reg.c b/files-reg.c
> index b1e9f73..fe24f3e 100644
> --- a/files-reg.c
> +++ b/files-reg.c
> @@ -919,22 +919,10 @@ int open_path(struct file_desc *d,
> int tmp, mntns_root;
> char *orig_path = NULL;
>
> - rfi = container_of(d, struct reg_file_info, d);
> -
> - tmp = inherit_fd_lookup_id(rfi->path);
> - if (tmp >= 0) {
> - int fd;
> -
> - fd = dup(tmp);
> - if (fd == -1) {
> - pr_perror("Can't dup inherit fd %d\n", fd);
> - return -1;
> - }
> - pr_info("File %s will be restored from fd %d duped from inherit fd %d\n",
> - rfi->path, fd, tmp);
> - return fd;
> - }
> + if (inherited_fd(d, &tmp))
> + return tmp;
>
> + rfi = container_of(d, struct reg_file_info, d);
> if (rfi->remap) {
> mutex_lock(ghost_file_mutex);
> if (rfi->remap->is_dir) {
> @@ -1138,10 +1126,19 @@ static int open_fe_fd(struct file_desc *fd)
> return open_path(fd, do_open_reg, NULL);
> }
>
> +static char *reg_file_path(struct file_desc *d, char *buf, size_t s)
> +{
> + struct reg_file_info *rfi;
> +
> + rfi = container_of(d, struct reg_file_info, d);
> + return rfi->path;
> +}
> +
> static struct file_desc_ops reg_desc_ops = {
> .type = FD_TYPES__REG,
> .open = open_fe_fd,
> .collect_fd = collect_reg_fd,
> + .name = reg_file_path,
> };
>
> struct file_desc *try_collect_special_file(u32 id, int optional)
> diff --git a/files.c b/files.c
> index 25ffe9e..412a99a 100644
> --- a/files.c
> +++ b/files.c
> @@ -1344,7 +1344,7 @@ void inherit_fd_log(void)
> /*
> * Look up the inherit fd list by a file identifier.
> */
> -int inherit_fd_lookup_id(char *id)
> +static int inherit_fd_lookup_id(char *id)
> {
> int ret;
> struct inherit_fd *inh;
> @@ -1363,6 +1363,28 @@ int inherit_fd_lookup_id(char *id)
> return ret;
> }
>
> +bool inherited_fd(struct file_desc *d, int *fd_p)
> +{
> + char buf[32], *id_str;
> + int i_fd;
> +
> + if (!d->ops->name)
> + return false;
> +
> + id_str = d->ops->name(d, buf, sizeof(buf));
> + i_fd = inherit_fd_lookup_id(id_str);
> + if (i_fd < 0)
> + return false;
> +
> + *fd_p = dup(i_fd);
> + if (*fd_p < 0)
> + pr_perror("Inherit fd DUP failed");
> + else
> + pr_info("File %s will be restored from fd %d duped "
> + "from inherit fd %d\n", id_str, *fd_p, i_fd);
> + return true;
> +}
> +
> /*
> * Look up the inherit fd list by a file descriptor.
> */
> diff --git a/include/files.h b/include/files.h
> index 5c9ab4e..67cb689 100644
> --- a/include/files.h
> +++ b/include/files.h
> @@ -105,6 +105,7 @@ struct file_desc_ops {
> */
> void (*collect_fd)(struct file_desc *, struct fdinfo_list_entry *,
> struct rst_info *);
> + char * (*name)(struct file_desc *, char *b, size_t s);
> };
>
> static inline void collect_gen_fd(struct fdinfo_list_entry *fle, struct rst_info *ri)
> @@ -170,8 +171,9 @@ extern int dump_unsupp_fd(struct fd_parms *p, int lfd,
>
> extern int inherit_fd_add(char *optarg);
> extern void inherit_fd_log(void);
> -extern int inherit_fd_lookup_id(char *id);
> extern int inherit_fd_resolve_clash(int fd);
> extern int inherit_fd_fini(void);
>
> +extern bool inherited_fd(struct file_desc *, int *fdp);
> +
> #endif /* __CR_FILES_H__ */
> diff --git a/pipes.c b/pipes.c
> index 2b1a4c8..7590472 100644
> --- a/pipes.c
> +++ b/pipes.c
> @@ -293,48 +293,31 @@ static int recv_pipe_fd(struct pipe_info *pi)
> return fd;
> }
>
> -static char *pipe_id_string(int pipe_id)
> +static char *pipe_d_name(struct file_desc *d, char *buf, size_t s)
> {
> - static char idstr[18];
> + struct pipe_info *pi;
>
> - if (snprintf(idstr, sizeof idstr, "pipe:[%d]", pipe_id) >= sizeof idstr) {
> - pr_err("Not enough room for pipe %d identifier string\n", pipe_id);
> + pi = container_of(d, struct pipe_info, d);
> + if (snprintf(buf, s, "pipe:[%d]", pi->pe->pipe_id) >= s) {
> + pr_err("Not enough room for pipe %d identifier string\n",
> + pi->pe->pipe_id);
> return NULL;
> }
> - return idstr;
> +
> + return buf;
> }
>
> static int open_pipe(struct file_desc *d)
> {
> - char *pipe_name;
> struct pipe_info *pi, *p;
> int ret, tmp;
> int pfd[2];
> int sock;
>
> - pi = container_of(d, struct pipe_info, d);
> -
> - /*
> - * If the pipe is in the inherit fd list,
> - * it should be inherited rather than created.
> - */
> - pipe_name = pipe_id_string(pi->pe->pipe_id);
> - if (pipe_name == NULL)
> - return -1;
> - tmp = inherit_fd_lookup_id(pipe_name);
> - if (tmp >= 0) {
> - int fd;
> -
> - fd = dup(tmp);
> - if (fd == -1) {
> - pr_perror("Can't dup inherit fd %d\n", fd);
> - return -1;
> - }
> - pr_info("Pipe %s will be restored from fd %d duped from inherit fd %d\n",
> - pipe_name, fd, tmp);
> - return fd;
> - }
> + if (inherited_fd(d, &tmp))
> + return tmp;
>
> + pi = container_of(d, struct pipe_info, d);
> pr_info("\t\tCreating pipe pipe_id=%#x id=%#x\n", pi->pe->pipe_id, pi->pe->id);
>
> if (!pi->create)
> @@ -396,6 +379,7 @@ static struct file_desc_ops pipe_desc_ops = {
> .type = FD_TYPES__PIPE,
> .open = open_pipe,
> .want_transport = want_transport,
> + .name = pipe_d_name,
> };
>
> static int collect_one_pipe(void *o, ProtobufCMessage *base)
> --
> 1.8.4.2
>
>
More information about the CRIU
mailing list