[CRIU] Re: [PATCH v2 08/11] dump: hide regular file open

[Pavel Emelyanov]

On 03/05/2012 08:12 PM, Kinsbursky Stanislav wrote:
> 05.03.2012 19:42, Pavel Emelyanov пишет:
>> On 03/05/2012 04:38 PM, Kinsbursky Stanislav wrote:
>>
>> This plus #7 move openat from one function into two sub-functions, don't they?
> 
> What? I don't understand. #7 moves openat into sub-function - yes. But not in two.

Мы поменяли один openat на два. Зачем?

>> What for?
> 
> Ok, here is the matter of taste and those, how we like to tell, aestehetic 
> differences.
> IOW, I'd prefer for split this code into two functions (one one them - with 
> passed lfd - is nested to another one). Nested one is called for vma and special 
> files dump, parent one - for regular fd.   And closing opened files is a 
> responsibility on nested function caller - not the function itself.
> This looks better to me.
> But If you prefer one more on-stack flag passed - then I'm remove this code.
> 
>>> Signed-off-by: Stanislav Kinsbursky<skinsbursky at openvz.org>
>>>
>>> ---
>>>   cr-dump.c |   44 ++++++++++++++++++++++++++------------------
>>>   1 files changed, 26 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/cr-dump.c b/cr-dump.c
>>> index 8494820..03062e5 100644
>>> --- a/cr-dump.c
>>> +++ b/cr-dump.c
>>> @@ -99,20 +99,19 @@ struct fd_parms {
>>>   	pid_t		pid;
>>>   };
>>>
>>> -static int dump_one_reg_file(struct fd_parms *p, int lfd,
>>> -			     struct cr_fdset *cr_fdset,
>>> -			     bool do_close_lfd)
>>> +static int dump_one_reg_file_fd(struct fd_parms *p, int lfd,
>>> +				struct cr_fdset *cr_fdset,
>>> +				bool do_close_lfd)
>>>   {
>>>   	struct fdinfo_entry e;
>>>   	char fd_str[128];
>>>   	int len;
>>> -	int ret = -1;
>>>
>>>   	snprintf(fd_str, sizeof(fd_str), "/proc/self/fd/%d", lfd);
>>>   	len = readlink(fd_str, big_buffer, sizeof(big_buffer) - 1);
>>>   	if (len<  0) {
>>>   		pr_perror("Can't readlink %s", fd_str);
>>> -		goto err;
>>> +		return -1;
>>>   	}
>>>
>>>   	big_buffer[len] = '\0';
>>> @@ -140,7 +139,7 @@ static int dump_one_reg_file(struct fd_parms *p, int lfd,
>>>
>>>   		entry = fd_id_entry_collect((u32)p->id, p->pid, p->fd_name);
>>>   		if (!entry)
>>> -			goto err;
>>> +			return -1;
>>>
>>>   		/* Now it might have completely new ID here */
>>>   		e.id	= entry->u.id;
>>> @@ -150,12 +149,25 @@ static int dump_one_reg_file(struct fd_parms *p, int lfd,
>>>   		p->type, len, p->flags, p->pos, p->fd_name);
>>>
>>>   	if (write_img(cr_fdset->fds[CR_FD_FDINFO],&e))
>>> -		goto err;
>>> +		return -1;
>>>   	if (write_img_buf(cr_fdset->fds[CR_FD_FDINFO], big_buffer, e.len))
>>> -		goto err;
>>> +		return -1;
>>> +	return 0;
>>> +}
>>>
>>> -	ret = 0;
>>> -err:
>>> +static int dump_one_reg_file(struct fd_parms *p, struct cr_fdset *cr_fdset)
>>> +{
>>> +	int ret, lfd;
>>> +	char d_name[32];
>>> +
>>> +	snprintf(d_name, sizeof(d_name), "%ld", p->fd_name);
>>> +	lfd = openat(p->pid_fd_dir, d_name, O_RDONLY);
>>> +	if (lfd<  0) {
>>> +		pr_perror("Failed to open regular file %d/%ld\n", p->pid_fd_dir, p->fd_name);
>>> +		return -1;
>>> +	}
>>> +	ret = dump_one_reg_file_fd(p, lfd, cr_fdset, 0);
>>> +	close_safe(&lfd);
>>>   	return ret;
>>>   }
>>>
>>> @@ -175,7 +187,7 @@ static int dump_task_special_files(pid_t pid, struct cr_fdset *cr_fdset)
>>>   	fd = open_proc(pid, "cwd");
>>>   	if (fd<  0)
>>>   		return -1;
>>> -	ret = dump_one_reg_file(&params, fd, cr_fdset, 1);
>>> +	ret = dump_one_reg_file_fd(&params, fd, cr_fdset, 1);
>>>   	if (ret)
>>>   		return ret;
>>>
>>> @@ -190,7 +202,7 @@ static int dump_task_special_files(pid_t pid, struct cr_fdset *cr_fdset)
>>>   	fd = open_proc(pid, "exe");
>>>   	if (fd<  0)
>>>   		return -1;
>>> -	ret = dump_one_reg_file(&params, fd, cr_fdset, 1);
>>> +	ret = dump_one_reg_file_fd(&params, fd, cr_fdset, 1);
>>>
>>>   	return ret;
>>>   }
>>> @@ -338,7 +350,6 @@ static int dump_one_fd(pid_t pid, int pid_fd_dir, char *d_name, struct cr_fdset
>>>   	struct stat fd_stat;
>>>   	int err = -1;
>>>   	struct fd_parms p;
>>> -	int lfd;
>>>
>>>   	err = fstatat(pid_fd_dir, d_name,&fd_stat, 0);
>>>   	if (err<  0) {
>>> @@ -349,8 +360,6 @@ static int dump_one_fd(pid_t pid, int pid_fd_dir, char *d_name, struct cr_fdset
>>>   	if (read_fd_params(pid, pid_fd_dir, d_name,&fd_stat,&p))
>>>   		return -1;
>>>
>>> -	lfd = openat(p.pid_fd_dir, d_name, O_RDONLY);
>>> -
>>>   	switch (fd_stat.st_mode&  S_IFMT) {
>>>   	case S_IFCHR:
>>>   		if (major(fd_stat.st_rdev) != MEM_MAJOR) {
>>> @@ -370,7 +379,7 @@ static int dump_one_fd(pid_t pid, int pid_fd_dir, char *d_name, struct cr_fdset
>>>   		 * Proceed to dump_one_reg_file().
>>>   		 */
>>>   	case S_IFREG:
>>> -		err = dump_one_reg_file(&p, lfd, cr_fdset, 0);
>>> +		err = dump_one_reg_file(&p, cr_fdset);
>>>   		break;
>>>   	case S_IFIFO:
>>>   		err = dump_one_pipe(&p, cr_fdset);
>>> @@ -392,7 +401,6 @@ static int dump_one_fd(pid_t pid, int pid_fd_dir, char *d_name, struct cr_fdset
>>>   	}
>>>   	if (err)
>>>   		pr_err("Can't dump file %ld of that type [%x]\n", p.fd_name, fd_stat.st_mode);
>>> -	close_safe(&lfd);
>>>   	return err;
>>>   }
>>>
>>> @@ -483,7 +491,7 @@ static int dump_task_mappings(pid_t pid, struct list_head *vma_area_list, struct
>>>   			else
>>>   				p.flags = O_RDONLY;
>>>
>>> -			ret = dump_one_reg_file(&p, vma_area->vm_file_fd, cr_fdset, 0);
>>> +			ret = dump_one_reg_file_fd(&p, vma_area->vm_file_fd, cr_fdset, 0);
>>>   			if (ret)
>>>   				goto err;
>>>   		}
>>>
> 
>