Messages in this thread | | | Subject | Re: [PATCH 1/5] VFS: DazukoFS, stackable-fs, file access control | From | John Ogness <> | Date | Sun, 21 Dec 2008 18:56:46 +0100 |
| |
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
| |