On Sat, May 20, 2006 at 10:57:40AM +0100, Christoph Hellwig wrote:
> I gave the code in -mm a quick look, and it still needs a lot of work.
looking over the code again:
> - please kill ECRYPTFS_SET_FLAG, ECRYPTFS_CLEAR_FLAG and ECRYPTFS_CHECK_FLAG
> and just opencode them, it'll make the code a whole lot more readable
these are still there..
> - please make sure touch_atime goes down to ->setattr for atime updates,
> that way you don't need all that mess in your read/write. and in -mm
> those routines need update for vectored and aio support
you don't seem to have updated the common code for this yet.
> - NEVER EVER do things like copying locks_delete_block and
> posix_lock_file_wait (as ecryptfs_posix_lock and based on a previous
> version) to you code. It will get stale and create a maintaince nightmare.
> talk with the subsystem maintainers on how to make the core functionality
> accesible to you.
> - similarly ecryptfs_setlk is totally non-acceptable. find a way with the
> maintainer to reuse things from fcntl_setlk with a common helper
dito
> - copying things like lock_parent, unlock_parent and unlock_dir
still there. sorry, but there's zero changes to get things merged that
opencode
>
> - please split all the generic stackable filesystem passthorugh routines
> into a separated stackfs layer, in a few files in fs/stackfs/ that
> you depend on. They'll get _GPL exported to all possible stackable
> filesystem. They'll need their own store underlying object helpers,
> but that can be made to work by embedding the generic stackfs data
> as first thing in the ecryptfs object.
>
>
>
More comments:
- what protects accessing d_parent in lock_parent / unlock_parent?
- no need to cast the return value of kmem_cache_alloc, it's void *
- any reason to use the SLAB_* flags instead of GFP_? I'm a bit surprised
SLAB_* still exists at all..
- follow_link handling is wrong. you need to call the underlying
->follow_link in your follow_link implementation and you need to keep
a separate nameidata for it
- please kill that ugly ecryptfs_allocated_caches hack and do normal
goto based unwinding on failure
- using iget with the lower filesystems i_ino does not work. There are
various filesystems were i_ino does not uniqueuely identify an inode.
You will probably need your own sequence numbers. Also please don't
use iget in new code but always the iget_locked variant.
And a more general issue with implementing stackable filesystems:
I think it's probably much better to not stack ontop of a part of the
existing namespace but rather let ecryptfs mount the underlying filesystem
internally using vfs_kern_mount. This gets out of the way of possible
lock order problems when doing namespace operation involving both the
stacked and underlying filesystem aswell as allowing using a stackable
filesystem as the root filesystem.
-
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]
