[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