Re: [RFC v3][PATCH 8/9] File descriprtors (dump)

[Dave Hansen <dave@linux.vnet.ibm.com>]

On Sun, 2008-09-07 at 00:52 -0400, Oren Laadan wrote:
> >> +	for (i = 0; i < fdt->max_fds; i++) {
> >> +		if (!fcheck_files(files, i)
> >> 			continue;
> >> 		if (n == max) {
> >> +			spin_unlock(&files->file_lock);
> >> +			kfree(fdlist);
> >> +			max *= 2;
> >> +			if (max < 0) {	/* overflow ? */
> >> +				n = -EMFILE;
> >> +				break;
> >> +			}
> >> +			goto repeat;
> >> +		}
> >> +		fdlist[n++] = i;
> >> +	}
> > 
> > My gut also says that there has to be a better way to find a good size
> > for fdlist() than growing it this way.  
> 
> Can you suggest a better way to find the open files of a task ?
> 
> Either I loop twice (loop to count, then allocate, then loop to fill),
> or optimistically try an initial guess and expand on demand.

I'd suggest the double loop.  I think it is much more straightforward
code.

> >> +	hh->f_version = file->f_version;
> >> +	/* FIX: need also file->f_owner */
> >> +
> >> +	switch (inode->i_mode & S_IFMT) {
> >> +	case S_IFREG:
> >> +		fd_type = CR_FD_FILE;
> >> +		break;
> >> +	case S_IFDIR:
> >> +		fd_type = CR_FD_DIR;
> >> +		break;
> >> +	case S_IFLNK:
> >> +		fd_type = CR_FD_LINK;
> >> +		break;
> >> +	default:
> >> +		return -EBADF;
> >> +	}
> > 
> > Why don't we just store (and use) (inode->i_mode & S_IFMT) in fd_type
> > instead of making our own types?
> 
> There will be others that cannot be inferred from inode->i_mode,
> e.g. CR_FD_FILE_UNLINKED, CR_FD_DIR_UNLINKED, CR_FD_SOCK_UNIX,
> CR_FD_SOCK_INET_V4, CR_FD_EVENTPOLL etc.

I think we have a basically different philosophy on these.  I'd say
don't define them unless absolutely necessary.  The less you spell out
explicitly, the more flexibility you have in the future, and the less
code you spend doing simple conversions.

Anyway, I see why you're doing it this way.

-- Dave