Re: [patch 1/13] signal/timer/event fds v6 - anonymous inode source...

[Davide Libenzi]

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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/