[CRIU] Re: [PATCH 07/12] dump: Use parasite file descriptors draining

[Pavel Emelyanov]

On 03/27/2012 05:17 PM, Cyrill Gorcunov wrote:
> On Tue, Mar 27, 2012 at 04:55:31PM +0400, Pavel Emelyanov wrote:
>>> -static int reg_files_fd = -1;
>>> +static int collect_fds(pid_t pid, int *fd, int *nr_fd)
>>> +{
>>> +	struct dirent *d;
>>> +	DIR *dir;
>>> +	int n;
>>> +
>>> +	pr_info("\n");
>>> +	pr_info("Collecting fds (pid: %d)\n", pid);
>>> +	pr_info("----------------------------------------\n");
>>> +
>>> +	dir = opendir_proc(pid, "fd");
>>> +	if (!dir)
>>> +		return -1;
>>> +
>>> +	n = 0;
>>> +	while ((d = readdir(dir))) {
>>> +		if (d->d_name[0] == '.')
>>> +			continue;
>>
>> This is wrong.
> 
> Why? You can't get file descriptor number started from '.',
> this is not arbitrary directory we're scanning but /proc/fd
> with predefined structure.

Sorry, I was in a hurry and said it wrongly. The old code did scan of proc/pid/fd
in one way, you need to do the same, thus do not rewrite it that much. If you don't
like existing two strcmps fix it with separate patch.

>>> +static void fill_fd_params(pid_t pid, int fd, int lfd, struct fd_parms *p)
>>>  {
>>> -	FILE *file;
>>> -	int ret;
>>> +	p->fd_name	= fd;
>>> +	p->pos		= lseek(lfd, 0, SEEK_CUR);
>>> +	p->flags	= fcntl(lfd, F_GETFL);
>>> +	p->pid		= pid;
>>> +	p->id		= FD_ID_INVALID;
>>
>> Stop reformatting the code in one patch with writing new functionality.
> 
> Hmm. Wait, the old code had
> 
> -static int read_fd_params(pid_t pid, const char *fd, struct fd_parms *p)
>  {
> 	...
> -
> -	p->pid	= pid;
> -	p->id	= FD_ID_INVALID;
> -
> -	return 0;
> }
> 
> the new code
> 
> +static void fill_fd_params(pid_t pid, int fd, int lfd, struct fd_parms *p)
>  {
> +	p->fd_name	= fd;
> +	p->pos		= lseek(lfd, 0, SEEK_CUR);
> +	p->flags	= fcntl(lfd, F_GETFL);
> +	p->pid		= pid;
> +	p->id		= FD_ID_INVALID;
> 
> where is the _reformatting_?

More tabs in between.

>>> +	int nr_fds = PARASITE_MAX_FDS;
>>> +	int fds[nr_fds];
>>
>> I wouldn't allocate that much space on stack.
> 
> It's not that much, but if you prefer, I can use malloc here.

Yes, please.

> .
>