On Sat, 17 Mar 2007, Arnd Bergmann wrote:
> 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.
I personally prefer to have them always on top so I don't have to figure
out where to place a new function, or to re-arrange the order of the
functions if the implementation changes. Plus I like to keep all the data
declarations on top, and this would still require some of the
static functions declarations to preceed it in any case. I really don't
want to waste time with counter-argouments that are most definitely a
personal taste, so if lots of ppl feel raw about that, and it goes in the
coding standard, I'll be happily change it.
> > +static struct vfsmount *aino_mnt __read_mostly;
> > +static struct inode *aino_inode;
> > +static struct file_operations aino_fops = { };
>
> Iirc, file_operations should be const.
Ack! It should, yes.
> > +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
Done!
> > +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().
Indeed, it can't be a module, so no exit function and fs_initcall.
Thx!
- Davide
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
