lkml.org 
[lkml]   [2008]   [Dec]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/5] VFS: DazukoFS, stackable-fs, file access control
From
Date
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


\
 
 \ /
  Last update: 2008-12-21 18:59    [W:0.253 / U:0.020 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site