Messages in this thread | | | From | Arnd Bergmann <> | Subject | Re: [patch 1/13] signal/timer/event fds v6 - anonymous inode source ... | Date | Sat, 17 Mar 2007 21:58:45 +0100 |
| |
On Friday 16 March 2007 01:22:15 Davide Libenzi wrote:
> + > +static int ainofs_delete_dentry(struct dentry *dentry); > +static struct inode *aino_getinode(void); > +static struct inode *aino_mkinode(void); > +static int ainofs_get_sb(struct file_system_type *fs_type, int flags, > + const char *dev_name, void *data, struct vfsmount *mnt); > +
In general, it would be good if you could just reorder your functions so that you don't need any forward declarations like these. It makes reviewing from bottom to top a little easier and it becomes obvious that there are no recursions in the code.
> +static struct vfsmount *aino_mnt __read_mostly; > +static struct inode *aino_inode; > +static struct file_operations aino_fops = { };
Iirc, file_operations should be const.
> +int aino_getfd(int *pfd, struct inode **pinode, struct file **pfile, > + char const *name, const struct file_operations *fops, void *priv) > +{
Since this is meant to be a generic interface that can be used from other subsystems, a kerneldoc style comment would be nice
> +static int __init aino_init(void) > +{ > + > + if (register_filesystem(&aino_fs_type)) > + goto epanic; > + > + aino_mnt = kern_mount(&aino_fs_type); > + if (IS_ERR(aino_mnt)) > + goto epanic; > + > + aino_inode = aino_mkinode(); > + if (IS_ERR(aino_inode)) > + goto epanic; > + > + return 0; > + > +epanic: > + panic("aino_init() failed\n"); > +}
panic() is a little harsh from a loadable module. If you mean the aino support to be used as a module, this should probably just return an error.
> +static void __exit aino_exit(void) > +{ > + iput(aino_inode); > + unregister_filesystem(&aino_fs_type); > + mntput(aino_mnt); > +}
but since the Makefile always has it as built-in, maybe you should instead just kill the exit function and use fs_initcall instead of init_module().
Arnd <>< - 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/
| |