Re: [PATCH 1/5] VFS: DazukoFS, stackable-fs, file access control

[John Ogness <dazukocode@ogness.net>]

On 2008-12-21, Bastian Blank <bastian@waldi.eu.org> wrote:
> There are several (okay, all) includes missing.

Indeed. I will fix this.

>> +static inline
>> +struct dazukofs_sb_info *GET_SB_INFO(struct super_block *upper_sb)
>
> Coding-style.

The functions were originally macros. The capitalization was inspired
by macros such as IS_ERR, PTR_ERR, MKDEV, etc. That is the
explanation, but it doesn't make it ok. I will change it.

>> +static inline void SET_LOWER_INODE(struct inode *upper_inode,
>> +				   struct inode *lower_inode)
>> +{
>> +	((struct dazukofs_inode_info *)
>> +	 container_of(upper_inode, struct dazukofs_inode_info,
>> +		      vfs_inode))->lower_inode = lower_inode;
>> +}
>
> Please make such cast cascades explicit:
>
> | struct dazukofs_inode_info *info = container_of(...);
> | info->lower_inode = lower_inode;
>
> There are other people which want to read the code.

Agreed.

>> +static int dazukofs_d_revalidate(struct dentry *dentry, struct nameidata *nd)
>> +{
>> +	struct vfsmount *lower_mnt;
>> +	struct dentry *lower_dentry;
>> +	struct vfsmount *vfsmount_save;
>> +	struct dentry *dentry_save;
>> +	int valid;
>> +
>> +	valid = 1;
>> +
>> +	lower_dentry = GET_LOWER_DENTRY(dentry);
>> +
>> +	if (!lower_dentry->d_op || !lower_dentry->d_op->d_revalidate)
>> +		goto out;
>
> Why do you use goto instead of return?

I use a goto here so that only 1 exit point exists within the
function. However, that probably doesn't make much sense when such
basic sanity checks are performed at the beginning of the function. I
will change this.

>> +static int dazukofs_d_hash(struct dentry *dentry, struct qstr *name)
>> +{
>> +	struct dentry *lower_dentry = GET_LOWER_DENTRY(dentry);
>> +
>> +	if (!lower_dentry || !lower_dentry->d_op ||
>> +	    !lower_dentry->d_op->d_hash) {
>> +		return 0;
>> +	}
>
> You mix rather different coding styles through the whole code.

When I wrote the stacking code, it was the first time I started using
the Linux coding style. I will go back through and see what I can
clean up.

> Also why do you say that lower_dentry can be 0 in _hash, but not in
> _revalidate?

_hash doesn't need this check. I will remove it.

>> +static void dazukofs_d_release(struct dentry *dentry)
>> +{
>> +	if (GET_DENTRY_INFO(dentry)) {
>> +		dput(GET_LOWER_DENTRY(dentry));
>> +		mntput(GET_LOWER_MNT(dentry));
>
> Why do you push anything out in other functions while making it
> explicit would make it much easier readable?

For me, writing a stackable filesystem was quite complex.  Using the
all-caps functions for upper/lower layer translations helped me to
easily see what I was doing.  I agree that the all-caps thing is quite
ugly. I will change that, but I would prefer to keep the translation
functions as separate inline functions. (The same technique can be
seen in ecryptfs as well.)

John Ogness