Re: [PATCH 2/11] FUSE - core

[Miklos Szeredi <miklos@xxxxxxxxxx>]

> > + *
> > + *  - the private_data field of the device file
> > + *  - the s_fs_info field of the super block
> > + *  - unused_list, pending, processing lists in fuse_conn
> > + *  - the unique request ID counter reqctr in fuse_conn
> > + *  - the sb (super_block) field in fuse_conn
> > + *  - the file (device file) field in fuse_conn
> > + */
> 
> These comments seem out of date.  There is no unused_lsit, pending or
> processing lists in fuse_conn.  Nor is there a reqctr or file.
> 

[...]

> > +		return NULL;
> > +	spin_lock(&fuse_lock);
> > +	fc->sb = sb;
> > +	spin_unlock(&fuse_lock);
> 
> The lock here looks unnessary, fc is private to this function at this point.

Yes, well these are caused by the split.  Later patches will explain
these.  I could split up the comment too...

> > +static int fuse_read_super(struct super_block *sb, void *data, int silent)
> 
> Can you rename this to fuse_fill_super so its consistent with what the
> VFS calls it?

Yes.

> > +		fuse_inode_cachep = kmem_cache_create("fuse_inode",
> > +						      sizeof(struct inode) + sizeof(struct fuse_inode) ,
> 
> I'm not convinced this will get the right alignments in the case where
> struct inode ever changes size.  You're better off using a new struct
> that contains both and using the size of it here, as well as using it
> for calculating the offset in get_fuse_inode instead of &inode[1].

Good point. I haven't thought of this.

> > +int __init fuse_init(void)
> 
> static?

OK.

> > +{
> > +	printk(KERN_DEBUG "fuse exit\n");
> > +
> > +	fuse_fs_cleanup();
> > +}
> 
> Why not just do the cleanup here?  If you still want to keep fuse_exit
> seperate from fuse_fs_cleanup, may I suggest marking the former __exit?

Again, later patches explain this.

> > +/** Version number of this interface */
> > +#define FUSE_KERNEL_VERSION 5
> > +
> > +/** Minor version number of this interface */
> > +#define FUSE_KERNEL_MINOR_VERSION 1
> 
> I haven't yet looked at the other patches, but is this VERSION info
> negotiated with userspace?

Yes.

Thanks for the comments!

Miklos
-
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/