Re: [Ecryptfs-devel] [PATCH] ecryptfs: some inode attrs, and a question

[hooanon05@yahoo.co.jp]

Dave Kleikamp:
> Does this function make sense (un-compiled, un-tested)?
> 
> void ecryptfs_update_inode_from_lower(struct dentry *dentry)
> {
> 	struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
> 
> 	if (d_unhashed(lower_dentry))
> 		d_drop(dentry);
> 	else {
> 		struct inode *inode = dentry->d_inode;
> 		struct inode *lower_inode = lower_dentry->d_inode;
> 
> 		inode->i_nlink = lower_inode->i_nlink;
> 		inode->i_ctime = lower_inode->i_ctime;
> 		/* Should anything else go here ? */
> 	}
> }

Yes, testing by d_unhashed() is best, I think.
But all inode attributes are maintained by ecryptfs_interpose(), so you
don't need to handle them.

And I found another bug in ecryptfs_link().
Here is a patch including that.


J. R. Okajima

----------------------------------------------------------------------
refine ecryptfs_link()

ecryptfs_link() doesn't need to,,,
- copy i_nlink and i_size for the target inode since they are already
  handled by ecryptfs_interpose(). (I don't know why i_size was
  necessary)
- maintain dir attributes based upon the lower dir, instead of
  lower_new_dentry. (another bug)
- several d_drop()s. do it only when the lower_new_dentry is d_drop()ed
  by the lower fs or vfs_link() failed.
- testing lower_new_dentry->d_inode after vfs_link() succeeded.

Signed-off-by: J. R. Okajima <hooanon05@yahoo.co.jp>

diff -u -p -r1.1 inode.c
--- inode.c	19 Dec 2008 03:05:27 -0000	1.1
+++ inode.c	17 Jan 2009 17:26:53 -0000
@@ -405,10 +407,8 @@ static int ecryptfs_link(struct dentry *
 	struct dentry *lower_old_dentry;
 	struct dentry *lower_new_dentry;
 	struct dentry *lower_dir_dentry;
-	u64 file_size_save;
 	int rc;
 
-	file_size_save = i_size_read(old_dentry->d_inode);
 	lower_old_dentry = ecryptfs_dentry_to_lower(old_dentry);
 	lower_new_dentry = ecryptfs_dentry_to_lower(new_dentry);
 	dget(lower_old_dentry);
@@ -416,23 +416,23 @@ static int ecryptfs_link(struct dentry *
 	lower_dir_dentry = lock_parent(lower_new_dentry);
 	rc = vfs_link(lower_old_dentry, lower_dir_dentry->d_inode,
 		      lower_new_dentry);
-	if (rc || !lower_new_dentry->d_inode)
-		goto out_lock;
-	rc = ecryptfs_interpose(lower_new_dentry, new_dentry, dir->i_sb, 0);
-	if (rc)
-		goto out_lock;
-	fsstack_copy_attr_times(dir, lower_new_dentry->d_inode);
-	fsstack_copy_inode_size(dir, lower_new_dentry->d_inode);
-	old_dentry->d_inode->i_nlink =
-		ecryptfs_inode_to_lower(old_dentry->d_inode)->i_nlink;
-	i_size_write(new_dentry->d_inode, file_size_save);
-out_lock:
+	if (!rc) {
+		fsstack_copy_attr_times(dir, lower_dir_dentry->d_inode);
+		fsstack_copy_inode_size(dir, lower_dir_dentry->d_inode);
+		if (!d_unhashed(lower_new_dentry)
+		    /* && lower_new_dentry->d_inode */) {
+			rc = ecryptfs_interpose(lower_new_dentry, new_dentry,
+						dir->i_sb, 0);
+			if (!rc)
+				goto out_lock; /* success */
+		}
+	}
+ out_drop:
+	d_drop(new_dentry);
+ out_lock:
 	unlock_dir(lower_dir_dentry);
 	dput(lower_new_dentry);
 	dput(lower_old_dentry);
-	d_drop(lower_old_dentry);
-	d_drop(new_dentry);
-	d_drop(old_dentry);
 	return rc;
 }