Re: eCryptfs: Request for review

[Michael Thompson <michael.craig.thompson@gmail.com>]

On 10/19/05, Charles P. Wright <cwright@cs.sunysb.edu> wrote:
> This is just a brief glance, and by no means complete, but here are a
> few things that I saw.
>
> In dentry.c you shouldn't implement d_delete.  This will cause problems
> with certain lower-level file systems.
Not entirely sure I follow the reasoning why here. Obviously, if it
needs to be, it can be removed, but all this function is doing is a
bunch of sanity checks, then then calling d_delete on the lower
filesystems if it exists. Just trying to get a better understand of
the code you see :)

> In crypto.c:ecryptfs_dump_hex you have variables intermixed with code,
> which won't work on some older compilers (AFAIK gcc 2.95 will barf).
Noted. Changed.

> In ecryptfs_llseek,  If you are adding pages for extra information, you
> need to convert the offset to a lower-level offset, and then convert the
> return back to what user space expects it to be.  You should test fsx on
> your file system, to sort these types of things out.
I believe that this is working, we have run fsx "succesfully" for
10000 iterations. However, its not a total success because we are
bugging out in fs/buffer.c:1822 BUG_ON(PageWriteback(page));
Will look a bit more closely though.

> You should probably rename ecryptfs_write to ecryptfs_dir_write, or just
> use generic_read_dir and NULL for directory read and write,
> respectively.
Changed. Will look into using the generics, not entirely sure what
impact that will have without further investigation.

> In inode.c:
> Instead of get_parent you can use dget_parent.  The core kernel just
> seems to have lock_parent as two lines in the function that need it
> instead of the function these days.
>
> The rest of the VFS locking functions should also be converted from 2.4
> to 2.6 primitives (this is a bug that you inherited from the FiST
> templates).  For example, instead of using double_down, you should use
> lock_rename.
Thanks for those, adding them to our ever growing TODO list :)

> For main.c, you can replace the nasty mount option string processing
> with lib/parser.c.
Will look into doing that :)

Mike