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

[Kinsbursky Stanislav]

05.03.2012 20:16, 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 на два. Зачем?

Чтобы явно звать openat только для тех типов fd, для которых надо. А не для всех 
подряд, игноря результат (как для сокетов).

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


-- 
Best regards,
Stanislav Kinsbursky