> v9fs
>
> I'm not sure that this has a sufficiently high
> usefulness-to-maintenance-cost ratio.
Personally I think this is very useful to have. It provides a portable
way to have simple userland or remote filesystems, and it's been around
for a long time in others OSes.
That beeing said there's a few issues with the code still I'd like to
see fixed:
- there's three sparse warnings still. Two of them are easily fixed
by moving externs to headers, one doesn't look fixable until we get
a sane in-kernel api for socket operations
- some dentry handling looks rather odd. Why are you for example
calling d_drop in v9fs_vfs_symlink, v9fs_vfs_mknod and v9fs_vfs_link?
Shouldn't all these call d_instantatiate to actually reuse the
dentry as in v9fs_vfs_create? Also what's the issue with
v9fs_fid_insert? It would seem better and more logical to me to
always set d_fsdata in create/mknod/symlink/open before hashing it
and then beeing able to rely on it beeing non-NULL.
- buf_check_sizep, buf_check_size and buf_check_sizev should be made
inlines, and lose the implict return. Please don't hide such
things in macros
- please avoid using hlist_for_each, usually hlist_for_each_entry is
a much better choice
- dito for list_for_each_safe vs list_for_each_entry_safe
- can you please check whether lib/idr.c fullfills your needs so we
can get rid of idpool.c?
- v9fs_inode2v9ses has lots of useless checks, inode->i_sb can never
be NULL, and inode->i_sb->s_fs_info can't be either once set in
fill_inode, which is before the first inode on the filesystem is
created. Also the argument is never NULL. Because of that you
can also kill all the return value checks in the callers.
- do you really need to keep v9fs_dentry_delete just for the dprintk?
- no need to check for a NULL file in v9fs_dir_readdir, the VFS gurantees
it's not. And if it was you'd better be off panic because something
is enormously fscked.
- Dito for v9fs_file_open
- And the inode in v9fs_file_lock
- And dir, file, file->d_inode, sb, v9ses in v9fs_remove.
- And dir, sb and v9ses in v9fs_vfs_lookup
- And dir, sb and v9ses in v9fs_vfs_symlink
- And dir, sb and v9ses in v9fs_vfs_link
- And dir, sb and v9ses in v9fs_vfs_mknod
- copy_from_user returns the bytes actually copied in the failure case,
but you should return -EFAULT instead of that number in v9fs_file_write
- No need to implement v9fs_file_mmap, do_mmap_pgoff makes sure to error
out if it's not present (and actually returns the correct errno)
- I think it's pretty similar for all these checks for fid (=private_data)
checks. You always set them in open, so they can't be NULL
- kfree can be called with a NULL argument just fine, you can remove
lots of ifs for that. You also often set pointers to NULL just before
freeing a structure - that's pretty useless as slab debugging will
catch bugs with stary references very well, and overwrites these NULLs
ASAP.
- The call to ->put_inode in the error case of v9fs_get_inode is very
wrong. You'd actually panic if you ever hit this as v9fs doesn't
implement a ->put_inode :-)
- All the ISDIR checks in v9fs_remove can go, VFS makes sure to only
call ->remove and ->rmdir on directories, and only the right one
for each kind of child.
- Please try to use generic_readlink instead of your own
v9fs_vfs_readlink, as you're implemting ->follow_link and ->put_link
that should just work
- the last error case in v9fs_get_sb needs a dput on ->s_root
Also did you look into the VFS/NFS lookup intent bits to solve your
atomic create and open issue?
-
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/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
