[CRIU] [PATCH 2/7] files: Add fill_fdlink helper

[Pavel Emelyanov]

On 05/09/2013 11:21 PM, Cyrill Gorcunov wrote:
> On Thu, May 09, 2013 at 11:15:51PM +0400, Pavel Emelyanov wrote:
>> On 05/09/2013 10:49 PM, Cyrill Gorcunov wrote:
>>> On Thu, May 09, 2013 at 09:51:51PM +0400, Pavel Emelyanov wrote:
>>>>>  
>>>>> -	if (S_ISREG(p.stat.st_mode) || S_ISDIR(p.stat.st_mode))
>>>>> +	if (S_ISREG(p.stat.st_mode) || S_ISDIR(p.stat.st_mode)) {
>>>>> +		char path[PATH_MAX + 1];
>>>>> +
>>>>> +		p.nmlink = path;
>>>>> +		p.nmsize = sizeof(path);
>>>>> +
>>>>> +		if (fill_fdlink(lfd, &p)) {
>>>>> +			pr_err("Can't fill fdlink params\n");
>>>>> +			return -1;
>>>>> +		}
>>>>> +
>>>>>  		return dump_reg_file(&p, lfd, fdinfo);
>>>>
>>>> dump_reg_file will end up calling dump_one_reg_file, which in turn, will
>>>> call read_fd_link, so why calling one here via a wrapper?
>>>
>>> This read_fd_link will be called in dump_one_reg_file only if the caller
>>> is not here (we call dump_one_reg_file for mm_exe and other special files).
>>> So to not call read_fd_link twice this optimisation is done.
>>
>> Why not call it _only_ in the deepest function, i.e. dump_one_reg_file?
> 
> Because I'll need to analyze the name first and call the appropriate dupmer
> later. I guess it's not clean from patch itself. Letme show the final code
> 
> 	if (S_ISREG(p.stat.st_mode) || S_ISDIR(p.stat.st_mode)) {
> 		struct ns_proc_entry found;
> 		char path[PATH_MAX + 1];
> 
> 		p.nmlink = path;
> 		p.nmsize = sizeof(path);
> 
> 		if (fill_fdlink(lfd, &p)) {
> 			pr_err("Can't fill fdlink params\n");
> 			return -1;
> 		}
> 
> Here we read link first, because the name will be needed in two
> places
> 
> 		/*
> 		 * Regular file might be a reference to proc namespace
> 		 * entry thus choose which strategy to dump it with.
> 		 */
> 		if (parse_ns_proc(&p.nmlink[1], p.nmsize - 1, &found) == 0) {
> 			p.arg = &found;
> 			return dump_ns_file(&p, lfd, fdinfo);
> 		}
> 
> If the name was found to be ns reference, we pass it to dump_ns_file
> 
> 		return dump_reg_file(&p, lfd, fdinfo);
> 
> Otherwise we pass it to dump_reg_file. Thus we read the link only once
> at top level.
> 	}

I see. Let's change the workflow:

dump_one_reg_file should look like

   read_fd_link()
   do_dump_one_reg_file

regfile_ops->dump should point to do_dump_one_reg_file

dump_reg_file should do

   read_fd_link()

   if (proc_ns)
         dump_gen_file(proc_ns_ops)
   else
         dump_gen_file(regfile_ops)

> .
>