I gave the code in -mm a quick look, and it still needs a lot of work.
- please kill ASSERT and always use BUG_ON.
- you don't need semaphore.h anymore
- please always use <linux/scatterlist.h> instead of <asm/scatterlist.h>
- 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
- why are so many fields on your data structures signed?
- please kill the reaming uint*_t uses
- there's definitly too many ifndefs in ecryptfs_kernel.h. Either
remove the or provide a good explanation why the macros could have
been defined already
( - you need endianess annotations and conversion for your ondisk data
structure. it's totally unacceptable a encrypted filesystem from a BE
machine can't be read on a LE one)
- pleaese get rid of the horrible (NULL == something) style, it makes
the code really hard to read
- in ecryptfs_d_revalidate just replacing the vfsmount/dentry in the
nameidata is dangerous, after that they aren't coherent with the
lookup data anymore. Either find a way to get a real nameidata that's
valid for a lookup on your filesystem or find away to get rid of passing
down the nameidata everywhere and replace it by a real lookup intent
structure. (or even better restructure the code to allow for a high-level
method replacing the lookup intents)
(ditto for various namespace operations in inode.c)
- 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
- in read_inode_size_from_header please do the kmap after calling ->readpage.
that also allows to swwitch to the more efficien kmap_atomic. also instead
of the memcpy just do
u64 *data_size == kmap_atomic(page, ..)
as kmap_atomic returns a void * (and even if it didn't you could cast
the return value) also you don't need i_size_write there since the inode
isn't life yet.
- ecryptfs_fsync is good sign for various issues all over the code:
o if the various _to_private methods could fail you have much worse
problems, they shouldn't return errors.
o the lower_* are too long and hurt the eye, just use l*
o file->f_op and inode->i_fop for a file instances of a file are
always the same, no need to duplicate all the code
o i_fop can't be NULL ever
With all that fixed the code in this case should look something like:
----------------- snip -----------------
static int
stackfs_fsync(struct file *file, struct dentry *dentry, int datasync)
{
struct file *lfile = file ? lower_file(file) : NULL;
struct dentry *ldentry = lower_dentry(dentry);
struct inode *linode = ldentry->d_inode;
int rc = -EINVAL;
if (linode->i_fop->fsync) {
mutex_lock(&linode->i_mutex);
rc = linode->i_fop->fsync(lfile, ldentry, datasync);
mutex_unlock(&ldentry->d_inode->i_mutex);
}
return rc;
}
----------------- snip -----------------
- 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
- copying things like lock_parent, unlock_parent and unlock_dir
- 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.
that's how far I got today, that's not even half-through yet.
-
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]
