lkml.org 
[lkml]   [2005]   [Jan]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/11] FUSE - core
From
Date
> > + *
> > + * - 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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 14:09    [W:0.052 / U:0.284 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site