Re: [PATCH 08 of 23] isofs: change uses of f_{dentry, vfsmnt} to use f_path

[Linus Torvalds <torvalds@osdl.org>]

On Sat, 21 Oct 2006, James Morris wrote:
>
> What about something like:
> 
> static inline struct inode *fpath_ino(struct file *file)
> {
> 	return file->f_path.dentry->d_inode;
> }

Generally, unless it saves a _lot_ of typing, we've tried to avoid 
gratuitous hiding of details. And "ino" isn't a good name, it's something 
we've traditionally used for the inode _number_. So it would be 
"fpath_inode()" or "file_inode()" or something.

As it is, the difference between

	file->f_dentry->d_inode
	fpath_inode(file)

is not really enough of a win to merit hiding that it's doing two pointer 
dereferences. Now, whether the extra five characters ("path.") merit it, I 
don't know.  I suspect not. If the line turns long, it's often more 
readable to just add a local variable or two, and do

	struct dentry *dentry = file->f_[path.]dentry;
	struct inode *inode = dentry->d_inode;

which in some situations allow for other readability improvements too (eg 
maybe "dentry" or "inode" is used multiple times).

If this was something where we'd expect things to change in the future, 
maybe it would be worth it for _that_ reason. That doesn't sound very 
likely, though - these things have been fairly stable, and even this patch 
is really about syntactic cleanup than any real change.

Adding these kinds of "abstraction layers" is something that people are 
taught is good, but I personally tend to think that it makes it less 
obvious at the code level what the "costs" are. Unless you know things 
intimately, you really have no way of judging whether "fpath_inode()" is 
something expensive or not. 

I dunno. 

		Linus
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/