[CRIU] [PATCH 2/2] fd: Factor out inheriting FDs code

Pavel Emelyanov xemul at parallels.com
Wed Dec 31 02:23:03 PST 2014


On 12/30/2014 09:01 PM, Saied Kazemi wrote:
> 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);

If dup() fails, it means that we had to inherit a file, but failed,
so the error should be propagated back to caller rather than silently
switch to restoring the file from the image.

Thanks,
Pavel

> 
> --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