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

[Kinsbursky Stanislav]

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.

> 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