Re: [PATCH 4/4] eCryptfs: ino_t to u64 for filldir

[David Howells <dhowells@redhat.com>]

Michael Halcrow <mhalcrow@us.ibm.com> wrote:

> Is it safe to assume that a pointer will always be equal to or smaller
> than an unsigned long for all architectures?

I presume you're talking about the sizes of the types.  Inside the Linux
kernel:

	sizeof(void *) == sizeof(unsigned long)

must always hold true.  There are too many things that depend on it to do
otherwise.

> Casting a pointer to an unsigned long, in general, makes me a bit
> uncomfortable.

Pointers are just numbers that are used in a particular way.  Go and look in
linux/err.h:-)

> Dave Chinner told me that XFS uses 32-bit inode numbers on 32-bit
> machines, so I imagine that this patch really is only helpful for NFS.

At the moment, maybe, but they could always change it.  And what about Reiser4?

> +int ecryptfs_inode_test(struct inode *inode, void *candidate_lower_inode)
> +{
> +	if (ecryptfs_inode_to_private(inode) && 

Is this part of the condition actually necessary?  Can you not guarantee that
this will always be true?

> +	ecryptfs_set_inode_lower(inode, igrab((struct inode *)lower_inode));

igrab() might fail.  I would recommend doing it before calling iget5_unlocked()
and drop the extraneous reference to lower_inode afterwards if the eCryptFS
inode returned is already set up.

You're also casting lower_inode twice.  Whilst there's nothing actually wrong
with that, it might look better if you assigned it to its own variable at the
top of the function and only do the cast once.

> +static void ecryptfs_read_inode(struct inode *inode) { }
> +

You shouldn't need that any more.  Just leave the read_inode op pointer unset.
The NULL pointer exception handler will let you know if anyone tries to access
it (which they shouldn't - only *you* should call iget*() on your own inodes).


Looks good otherwise.  Just the igrab() thing is a real problem.

David