[PATCH] prevent NULL returns from d_obtain_alias

[PATCH] prevent NULL returns from d_obtain_alias

[Christoph Hellwig]

As Miklos pointed out a while ago returning NULL from the export
operations and thus d_obtain_alias is not a good idea - the callers
in exportfs convert it to an -ESTALE anyway (or as the audit pointed
out don't deal with it at all in case of ->get_parent), and possibly
returning either NULL or an ERR_PTR value form one function is
confusing.  By auditing the callers I also found various other places
that couldn't deal with the NULL return.  I guess an unlikely error
path of an unlikely pass like this simply doesn't get tested.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: vfs-2.6/fs/dcache.c
===================================================================
--- vfs-2.6.orig/fs/dcache.c	2008-10-11 04:42:11.000000000 -0400
+++ vfs-2.6/fs/dcache.c	2008-10-11 04:52:45.000000000 -0400
@@ -1123,10 +1123,10 @@ static inline struct hlist_head *d_hash(
  * allocating a new one.
  *
  * On successful return, the reference to the inode has been transferred
- * to the dentry.  If %NULL is returned (indicating kmalloc failure),
- * the reference on the inode has been released.  To make it easier
- * to use in export operations a NULL or IS_ERR inode may be passed in
- * and will be casted to the corresponding NULL or IS_ERR dentry.
+ * to the dentry.  In case of an error the reference on the inode is released.
+ * To make it easier to use in export operations a %NULL or IS_ERR inode may
+ * be passed in and will be the error will be propagate to the return value,
+ * with a %NULL @inode replaced by ERR_PTR(-ESTALE).
  */
 struct dentry *d_obtain_alias(struct inode *inode)
 {
@@ -1135,7 +1135,7 @@ struct dentry *d_obtain_alias(struct ino
 	struct dentry *res;
 
 	if (!inode)
-		return NULL;
+		return ERR_PTR(-ESTALE);
 	if (IS_ERR(inode))
 		return ERR_CAST(inode);
 
Index: vfs-2.6/fs/exportfs/expfs.c
===================================================================
--- vfs-2.6.orig/fs/exportfs/expfs.c	2008-10-11 04:52:51.000000000 -0400
+++ vfs-2.6/fs/exportfs/expfs.c	2008-10-11 04:53:49.000000000 -0400
@@ -367,8 +367,6 @@ struct dentry *exportfs_decode_fh(struct
 	 * Try to get any dentry for the given file handle from the filesystem.
 	 */
 	result = nop->fh_to_dentry(mnt->mnt_sb, fid, fh_len, fileid_type);
-	if (!result)
-		result = ERR_PTR(-ESTALE);
 	if (IS_ERR(result))
 		return result;
 
@@ -422,8 +420,6 @@ struct dentry *exportfs_decode_fh(struct
 
 		target_dir = nop->fh_to_parent(mnt->mnt_sb, fid,
 				fh_len, fileid_type);
-		if (!target_dir)
-			goto err_result;
 		err = PTR_ERR(target_dir);
 		if (IS_ERR(target_dir))
 			goto err_result;
Index: vfs-2.6/fs/fat/inode.c
===================================================================
--- vfs-2.6.orig/fs/fat/inode.c	2008-10-11 04:48:35.000000000 -0400
+++ vfs-2.6/fs/fat/inode.c	2008-10-11 04:50:10.000000000 -0400
@@ -697,7 +697,7 @@ static struct dentry *fat_fh_to_dentry(s
 	 * of luck.  But all that is for another day
 	 */
 	result = d_obtain_alias(inode);
-	if (result && !IS_ERR(result))
+	if (!IS_ERR(result))
 		result->d_op = sb->s_root->d_op;
 	return result;
 }
Index: vfs-2.6/fs/fuse/inode.c
===================================================================
--- vfs-2.6.orig/fs/fuse/inode.c	2008-10-11 04:41:20.000000000 -0400
+++ vfs-2.6/fs/fuse/inode.c	2008-10-11 04:50:25.000000000 -0400
@@ -597,7 +597,7 @@ static struct dentry *fuse_get_dentry(st
 		goto out_iput;
 
 	entry = d_obtain_alias(inode);
-	if (entry && !IS_ERR(entry) && get_node_id(inode) != FUSE_ROOT_ID) {
+	if (!IS_ERR(entry) && get_node_id(inode) != FUSE_ROOT_ID) {
 		entry->d_op = &fuse_dentry_operations;
 		fuse_invalidate_entry_cache(entry);
 	}
@@ -699,7 +699,7 @@ static struct dentry *fuse_get_parent(st
 	}
 
 	parent = d_obtain_alias(inode);
-	if (parent && !IS_ERR(parent) && get_node_id(inode) != FUSE_ROOT_ID) {
+	if (!IS_ERR(parent) && get_node_id(inode) != FUSE_ROOT_ID) {
 		parent->d_op = &fuse_dentry_operations;
 		fuse_invalidate_entry_cache(parent);
 	}
Index: vfs-2.6/fs/gfs2/ops_export.c
===================================================================
--- vfs-2.6.orig/fs/gfs2/ops_export.c	2008-10-11 04:48:35.000000000 -0400
+++ vfs-2.6/fs/gfs2/ops_export.c	2008-10-11 04:50:44.000000000 -0400
@@ -139,7 +139,7 @@ static struct dentry *gfs2_get_parent(st
 	gfs2_str2qstr(&dotdot, "..");
 
 	dentry = d_obtain_alias(gfs2_lookupi(child->d_inode, &dotdot, 1));
-	if (dentry && !IS_ERR(dentry))
+	if (!IS_ERR(dentry))
 		dentry->d_op = &gfs2_dops;
 	return dentry;
 }
@@ -223,7 +223,7 @@ static struct dentry *gfs2_get_dentry(st
 
 out_inode:
 	dentry = d_obtain_alias(inode);
-	if (dentry && !IS_ERR(dentry))
+	if (!IS_ERR(dentry))
 		dentry->d_op = &gfs2_dops;
 	return dentry;
 
Index: vfs-2.6/fs/nfs/getroot.c
===================================================================
--- vfs-2.6.orig/fs/nfs/getroot.c	2008-10-11 04:48:36.000000000 -0400
+++ vfs-2.6/fs/nfs/getroot.c	2008-10-11 04:51:44.000000000 -0400
@@ -108,9 +108,9 @@ struct dentry *nfs_get_root(struct super
 	 * exists, we'll pick it up at this point and use it as the root
 	 */
 	mntroot = d_obtain_alias(inode);
-	if (!mntroot) {
+	if (IS_ERR(mntroot)) {
 		dprintk("nfs_get_root: get root dentry failed\n");
-		return ERR_PTR(-ENOMEM);
+		return mntroot;
 	}
 
 	security_d_instantiate(mntroot, inode);
@@ -277,9 +277,9 @@ struct dentry *nfs4_get_root(struct supe
 	 * exists, we'll pick it up at this point and use it as the root
 	 */
 	mntroot = d_obtain_alias(inode);
-	if (!mntroot) {
+	if (IS_ERR(mntroot)) {
 		dprintk("nfs_get_root: get root dentry failed\n");
-		return ERR_PTR(-ENOMEM);
+		return mntroot;
 	}
 
 	security_d_instantiate(mntroot, inode);
Index: vfs-2.6/fs/ocfs2/export.c
===================================================================
--- vfs-2.6.orig/fs/ocfs2/export.c	2008-10-11 04:48:36.000000000 -0400
+++ vfs-2.6/fs/ocfs2/export.c	2008-10-11 04:51:59.000000000 -0400
@@ -69,7 +69,7 @@ static struct dentry *ocfs2_get_dentry(s
 	}
 
 	result = d_obtain_alias(inode);
-	if (result && !IS_ERR(result))
+	if (!IS_ERR(result))
 		result->d_op = &ocfs2_dentry_ops;
 
 	mlog_exit_ptr(result);
@@ -104,7 +104,7 @@ static struct dentry *ocfs2_get_parent(s
 	}
 
 	parent = d_obtain_alias(ocfs2_iget(OCFS2_SB(dir->i_sb), blkno, 0, 0));
-	if (parent && !IS_ERR(parent))
+	if (!IS_ERR(parent))
 		parent->d_op = &ocfs2_dentry_ops;
 
 bail_unlock:
Index: vfs-2.6/fs/xfs/linux-2.6/xfs_ioctl.c
===================================================================
--- vfs-2.6.orig/fs/xfs/linux-2.6/xfs_ioctl.c	2008-10-11 04:48:36.000000000 -0400
+++ vfs-2.6/fs/xfs/linux-2.6/xfs_ioctl.c	2008-10-11 04:52:34.000000000 -0400
@@ -312,7 +312,7 @@ xfs_open_by_handle(
 	}
 
 	dentry = d_obtain_alias(inode);
-	if (dentry == NULL) {
+	if (IS_ERR(dentry)) {
 		put_unused_fd(new_fd);
 		return -XFS_ERROR(ENOMEM);
 	}

Re: [PATCH] prevent NULL returns from d_obtain_alias

[Miklos Szeredi]

On Wed, 15 Oct 2008, Christoph Hellwig wrote:
> As Miklos pointed out a while ago returning NULL from the export
> operations and thus d_obtain_alias is not a good idea - the callers
> in exportfs convert it to an -ESTALE anyway (or as the audit pointed
> out don't deal with it at all in case of ->get_parent), and possibly
> returning either NULL or an ERR_PTR value form one function is
> confusing.  By auditing the callers I also found various other places
> that couldn't deal with the NULL return.  I guess an unlikely error
> path of an unlikely pass like this simply doesn't get tested.

Looks good, except:

> Index: vfs-2.6/fs/xfs/linux-2.6/xfs_ioctl.c
> ===================================================================
> --- vfs-2.6.orig/fs/xfs/linux-2.6/xfs_ioctl.c	2008-10-11 04:48:36.000000000 -0400
> +++ vfs-2.6/fs/xfs/linux-2.6/xfs_ioctl.c	2008-10-11 04:52:34.000000000 -0400
> @@ -312,7 +312,7 @@ xfs_open_by_handle(
>  	}
>  
>  	dentry = d_obtain_alias(inode);
> -	if (dentry == NULL) {
> +	if (IS_ERR(dentry)) {
>  		put_unused_fd(new_fd);
>  		return -XFS_ERROR(ENOMEM);

should be

  		return -XFS_ERROR(-PTR_ERR(dentry));

Thanks,
Miklos

Re: [PATCH] prevent NULL returns from d_obtain_alias

[Christoph Hellwig]

On Thu, Oct 16, 2008 at 07:50:10PM +0200, Miklos Szeredi wrote:
> should be
> 
>   		return -XFS_ERROR(-PTR_ERR(dentry));

No need for XFS_ERROR at all here, it's only used for error injection in
low-level code.  Updated version that propagates the error below:

(Al, if you rebase vfs.git this should probably be folded into the patch
that switches over to d_obtain_alias)


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: vfs-2.6/fs/dcache.c
===================================================================
--- vfs-2.6.orig/fs/dcache.c	2008-10-15 21:26:33.000000000 +0200
+++ vfs-2.6/fs/dcache.c	2008-10-16 20:05:56.000000000 +0200
@@ -1123,10 +1123,10 @@ static inline struct hlist_head *d_hash(
  * allocating a new one.
  *
  * On successful return, the reference to the inode has been transferred
- * to the dentry.  If %NULL is returned (indicating kmalloc failure),
- * the reference on the inode has been released.  To make it easier
- * to use in export operations a NULL or IS_ERR inode may be passed in
- * and will be casted to the corresponding NULL or IS_ERR dentry.
+ * to the dentry.  In case of an error the reference on the inode is released.
+ * To make it easier to use in export operations a %NULL or IS_ERR inode may
+ * be passed in and will be the error will be propagate to the return value,
+ * with a %NULL @inode replaced by ERR_PTR(-ESTALE).
  */
 struct dentry *d_obtain_alias(struct inode *inode)
 {
@@ -1135,7 +1135,7 @@ struct dentry *d_obtain_alias(struct ino
 	struct dentry *res;
 
 	if (!inode)
-		return NULL;
+		return ERR_PTR(-ESTALE);
 	if (IS_ERR(inode))
 		return ERR_CAST(inode);
 
Index: vfs-2.6/fs/exportfs/expfs.c
===================================================================
--- vfs-2.6.orig/fs/exportfs/expfs.c	2008-10-15 21:26:33.000000000 +0200
+++ vfs-2.6/fs/exportfs/expfs.c	2008-10-16 20:05:56.000000000 +0200
@@ -367,8 +367,6 @@ struct dentry *exportfs_decode_fh(struct
 	 * Try to get any dentry for the given file handle from the filesystem.
 	 */
 	result = nop->fh_to_dentry(mnt->mnt_sb, fid, fh_len, fileid_type);
-	if (!result)
-		result = ERR_PTR(-ESTALE);
 	if (IS_ERR(result))
 		return result;
 
@@ -422,8 +420,6 @@ struct dentry *exportfs_decode_fh(struct
 
 		target_dir = nop->fh_to_parent(mnt->mnt_sb, fid,
 				fh_len, fileid_type);
-		if (!target_dir)
-			goto err_result;
 		err = PTR_ERR(target_dir);
 		if (IS_ERR(target_dir))
 			goto err_result;
Index: vfs-2.6/fs/fat/inode.c
===================================================================
--- vfs-2.6.orig/fs/fat/inode.c	2008-10-15 21:26:33.000000000 +0200
+++ vfs-2.6/fs/fat/inode.c	2008-10-16 20:05:56.000000000 +0200
@@ -697,7 +697,7 @@ static struct dentry *fat_fh_to_dentry(s
 	 * of luck.  But all that is for another day
 	 */
 	result = d_obtain_alias(inode);
-	if (result && !IS_ERR(result))
+	if (!IS_ERR(result))
 		result->d_op = sb->s_root->d_op;
 	return result;
 }
Index: vfs-2.6/fs/fuse/inode.c
===================================================================
--- vfs-2.6.orig/fs/fuse/inode.c	2008-10-15 21:26:33.000000000 +0200
+++ vfs-2.6/fs/fuse/inode.c	2008-10-16 20:05:56.000000000 +0200
@@ -597,7 +597,7 @@ static struct dentry *fuse_get_dentry(st
 		goto out_iput;
 
 	entry = d_obtain_alias(inode);
-	if (entry && !IS_ERR(entry) && get_node_id(inode) != FUSE_ROOT_ID) {
+	if (!IS_ERR(entry) && get_node_id(inode) != FUSE_ROOT_ID) {
 		entry->d_op = &fuse_dentry_operations;
 		fuse_invalidate_entry_cache(entry);
 	}
@@ -699,7 +699,7 @@ static struct dentry *fuse_get_parent(st
 	}
 
 	parent = d_obtain_alias(inode);
-	if (parent && !IS_ERR(parent) && get_node_id(inode) != FUSE_ROOT_ID) {
+	if (!IS_ERR(parent) && get_node_id(inode) != FUSE_ROOT_ID) {
 		parent->d_op = &fuse_dentry_operations;
 		fuse_invalidate_entry_cache(parent);
 	}
Index: vfs-2.6/fs/gfs2/ops_export.c
===================================================================
--- vfs-2.6.orig/fs/gfs2/ops_export.c	2008-10-15 21:26:33.000000000 +0200
+++ vfs-2.6/fs/gfs2/ops_export.c	2008-10-16 20:05:56.000000000 +0200
@@ -139,7 +139,7 @@ static struct dentry *gfs2_get_parent(st
 	gfs2_str2qstr(&dotdot, "..");
 
 	dentry = d_obtain_alias(gfs2_lookupi(child->d_inode, &dotdot, 1));
-	if (dentry && !IS_ERR(dentry))
+	if (!IS_ERR(dentry))
 		dentry->d_op = &gfs2_dops;
 	return dentry;
 }
@@ -223,7 +223,7 @@ static struct dentry *gfs2_get_dentry(st
 
 out_inode:
 	dentry = d_obtain_alias(inode);
-	if (dentry && !IS_ERR(dentry))
+	if (!IS_ERR(dentry))
 		dentry->d_op = &gfs2_dops;
 	return dentry;
 
Index: vfs-2.6/fs/nfs/getroot.c
===================================================================
--- vfs-2.6.orig/fs/nfs/getroot.c	2008-10-15 21:26:34.000000000 +0200
+++ vfs-2.6/fs/nfs/getroot.c	2008-10-16 20:05:56.000000000 +0200
@@ -108,9 +108,9 @@ struct dentry *nfs_get_root(struct super
 	 * exists, we'll pick it up at this point and use it as the root
 	 */
 	mntroot = d_obtain_alias(inode);
-	if (!mntroot) {
+	if (IS_ERR(mntroot)) {
 		dprintk("nfs_get_root: get root dentry failed\n");
-		return ERR_PTR(-ENOMEM);
+		return mntroot;
 	}
 
 	security_d_instantiate(mntroot, inode);
@@ -277,9 +277,9 @@ struct dentry *nfs4_get_root(struct supe
 	 * exists, we'll pick it up at this point and use it as the root
 	 */
 	mntroot = d_obtain_alias(inode);
-	if (!mntroot) {
+	if (IS_ERR(mntroot)) {
 		dprintk("nfs_get_root: get root dentry failed\n");
-		return ERR_PTR(-ENOMEM);
+		return mntroot;
 	}
 
 	security_d_instantiate(mntroot, inode);
Index: vfs-2.6/fs/ocfs2/export.c
===================================================================
--- vfs-2.6.orig/fs/ocfs2/export.c	2008-10-15 21:26:33.000000000 +0200
+++ vfs-2.6/fs/ocfs2/export.c	2008-10-16 20:05:56.000000000 +0200
@@ -69,7 +69,7 @@ static struct dentry *ocfs2_get_dentry(s
 	}
 
 	result = d_obtain_alias(inode);
-	if (result && !IS_ERR(result))
+	if (!IS_ERR(result))
 		result->d_op = &ocfs2_dentry_ops;
 
 	mlog_exit_ptr(result);
@@ -104,7 +104,7 @@ static struct dentry *ocfs2_get_parent(s
 	}
 
 	parent = d_obtain_alias(ocfs2_iget(OCFS2_SB(dir->i_sb), blkno, 0, 0));
-	if (parent && !IS_ERR(parent))
+	if (!IS_ERR(parent))
 		parent->d_op = &ocfs2_dentry_ops;
 
 bail_unlock:
Index: vfs-2.6/fs/xfs/linux-2.6/xfs_ioctl.c
===================================================================
--- vfs-2.6.orig/fs/xfs/linux-2.6/xfs_ioctl.c	2008-10-15 21:26:33.000000000 +0200
+++ vfs-2.6/fs/xfs/linux-2.6/xfs_ioctl.c	2008-10-16 20:06:20.000000000 +0200
@@ -312,9 +312,9 @@ xfs_open_by_handle(
 	}
 
 	dentry = d_obtain_alias(inode);
-	if (dentry == NULL) {
+	if (IS_ERR(dentry)) {
 		put_unused_fd(new_fd);
-		return -XFS_ERROR(ENOMEM);
+		return PTR_ERR(dentry);
 	}
 
 	/* Ensure umount returns EBUSY on umounts while this file is open. */

XFS_ERROR use - was Re: [PATCH] prevent NULL returns from d_obtain_alias

[Timothy Shimmin]

Christoph Hellwig wrote:
> On Thu, Oct 16, 2008 at 07:50:10PM +0200, Miklos Szeredi wrote:
>> should be
>>
>>   		return -XFS_ERROR(-PTR_ERR(dentry));
> 
> No need for XFS_ERROR at all here, it's only used for error injection in
> low-level code.  Updated version that propagates the error below:
> 
> (Al, if you rebase vfs.git this should probably be folded into the patch
> that switches over to d_obtain_alias)
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 

[...stuff deleted]

> Index: vfs-2.6/fs/xfs/linux-2.6/xfs_ioctl.c
> ===================================================================
> --- vfs-2.6.orig/fs/xfs/linux-2.6/xfs_ioctl.c	2008-10-15 21:26:33.000000000 +0200
> +++ vfs-2.6/fs/xfs/linux-2.6/xfs_ioctl.c	2008-10-16 20:06:20.000000000 +0200
> @@ -312,9 +312,9 @@ xfs_open_by_handle(
>  	}
>  
>  	dentry = d_obtain_alias(inode);
> -	if (dentry == NULL) {
> +	if (IS_ERR(dentry)) {
>  		put_unused_fd(new_fd);
> -		return -XFS_ERROR(ENOMEM);
> +		return PTR_ERR(dentry);
>  	}
>  
>  	/* Ensure umount returns EBUSY on umounts while this file is open. */
> --

Fair enough.
But XFS_ERROR is used throughout the function.
I've found the whole idea of when and when not to use XFS_ERROR annoying :)

I've never used it (other than calling it to stay consistent with the code).
Looking at the code, it is used to BUG and print a msg on particular error codes set in xfs_etrap[] -
and it does this in xfs_error_trap().
Can one not decide to do this at any error point?
I can't see where we hook in to set up xfs_etrap.

> #ifdef DEBUG
> #define XFS_ERROR_NTRAP 10
> extern int      xfs_etrap[XFS_ERROR_NTRAP];
> extern int      xfs_error_trap(int);
> #define XFS_ERROR(e)    xfs_error_trap(e)
> #else
> #define XFS_ERROR(e)    (e)
> #endif


Cheers,
Tim.

Re: XFS_ERROR use - was Re: [PATCH] prevent NULL returns from d_obtain_alias

[Dave Chinner]

On Fri, Oct 17, 2008 at 11:11:00AM +1100, Timothy Shimmin wrote:
> Fair enough.
> But XFS_ERROR is used throughout the function.
> I've found the whole idea of when and when not to use XFS_ERROR annoying :)
> 
> I've never used it (other than calling it to stay consistent with the code).
> Looking at the code, it is used to BUG and print a msg on particular error codes set in xfs_etrap[] -
> and it does this in xfs_error_trap().
> Can one not decide to do this at any error point?
> I can't see where we hook in to set up xfs_etrap.

You break into the debugger, modify the xfs_etrap array to contain
the set of errors you want to catch, then continue onwards.  I've
used this several times in the past couple of months to locate the
source of strange ENOSPC errors...

The conceptual idea is that every point where an error is first
detected gets wrapped with XFS_ERROR() so you don't need to sprinkle
printk's all through the code to find where the error is coming
from.

ISTR the Irix kernel debugger had a command to set the values
in the etrap array - if it did it never got ported to kdb....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: XFS_ERROR use - was Re: [PATCH] prevent NULL returns from d_obtain_alias

[Eric Sandeen]

Dave Chinner wrote:
> On Fri, Oct 17, 2008 at 11:11:00AM +1100, Timothy Shimmin wrote:
>> Fair enough.
>> But XFS_ERROR is used throughout the function.
>> I've found the whole idea of when and when not to use XFS_ERROR annoying :)
>>
>> I've never used it (other than calling it to stay consistent with the code).
>> Looking at the code, it is used to BUG and print a msg on particular error codes set in xfs_etrap[] -
>> and it does this in xfs_error_trap().
>> Can one not decide to do this at any error point?
>> I can't see where we hook in to set up xfs_etrap.
> 
> You break into the debugger, modify the xfs_etrap array to contain
> the set of errors you want to catch, then continue onwards.

bleah :)

Could these just be turned into tracepoints eventually?  Or maybe for
now allow modifying the array via proc or something... would be easier
to ask a user to do that if you don't have direct access to their kdb
console ;)  It is a handy thing to have, though.

-Eric

Re: XFS_ERROR use - was Re: [PATCH] prevent NULL returns from d_obtain_alias

[Christoph Hellwig]

> 
> Fair enough.
> But XFS_ERROR is used throughout the function.

Can we leave it the simple way for now?  I have to revamp that whole
function anyway as it's extremly buggy in many ways, especially when
used to open directories (can lead to multiple dentries for a single
directory - ouch) and then I'll kill the other uses.

Re: XFS_ERROR use - was Re: [PATCH] prevent NULL returns from d_obtain_alias

[Timothy Shimmin]

Christoph Hellwig wrote:
>> Fair enough.
>> But XFS_ERROR is used throughout the function.
> 
> Can we leave it the simple way for now?  

Yep.

> I have to revamp that whole
> function anyway as it's extremly buggy in many ways, especially when
> used to open directories (can lead to multiple dentries for a single
> directory - ouch) and then I'll kill the other uses.

Oh ok.

In userspace,
we use it for opening directories on xfsdump via jdm_open in order to
do bulkstat driven dirent dumping.
We also use it in xfsrestore - though I am not convinced we should -
it was initially done for "performance" reasons apparently.

--Tim

Re: XFS_ERROR use - was Re: [PATCH] prevent NULL returns from d_obtain_alias

[Christoph Hellwig]

On Mon, Oct 20, 2008 at 11:49:47AM +1100, Timothy Shimmin wrote:
> > I have to revamp that whole
> > function anyway as it's extremly buggy in many ways, especially when
> > used to open directories (can lead to multiple dentries for a single
> > directory - ouch) and then I'll kill the other uses.
> 
> Oh ok.
> 
> In userspace,
> we use it for opening directories on xfsdump via jdm_open in order to
> do bulkstat driven dirent dumping.
> We also use it in xfsrestore - though I am not convinced we should -
> it was initially done for "performance" reasons apparently.

I think the use of the API is fine, the problem is that the current
implemention is buggy.

Re: XFS_ERROR use - was Re: [PATCH] prevent NULL returns from d_obtain_alias

[Timothy Shimmin]

Christoph Hellwig wrote:
> On Mon, Oct 20, 2008 at 11:49:47AM +1100, Timothy Shimmin wrote:
>>> I have to revamp that whole
>>> function anyway as it's extremly buggy in many ways, especially when
>>> used to open directories (can lead to multiple dentries for a single
>>> directory - ouch) and then I'll kill the other uses.
>> Oh ok.
>>
>> In userspace,
>> we use it for opening directories on xfsdump via jdm_open in order to
>> do bulkstat driven dirent dumping.
>> We also use it in xfsrestore - though I am not convinced we should -
>> it was initially done for "performance" reasons apparently.
> 
> I think the use of the API is fine, the problem is that the current
> implemention is buggy.
Yeah, I have no problem for xfsdump as it is driven by bulkstat
but for xfsrestore it seems unnecessary.
In fact, it can restore to other filesystems so it must only be using
this in certain ways on restore.
Having a look, yeah it is predicated by:
  if ( tranp->t_dstdirisxfspr )
and then just uses the fd for XFS_IOC_FSSETDM, XFS_IOC_FSSETXATTR
for those xfs-specific things and in the same function it uses
the path for non-xfs-specific: utime, chown, chmod
So it seems unnecessary to me in restore.

--Tim