Returning NULL from fh_to_dentry() and fh_to_parent() is not permitted, so
return -ESTALE instead. exportfs_decode_fh() does not check for NULL, but
will try to dereference it as IS_ERR() does not check for it.
The generic_fh_to_dentry() and generic_fh_to_parent() functions also no longer
return NULL, but return -ESTALE instead.
Signed-off-by: David Howells <[email protected]>
Acked-by: J. Bruce Fields <[email protected]>
---
fs/fat/inode.c | 2 +-
fs/fuse/inode.c | 4 ++--
fs/gfs2/ops_export.c | 4 ++--
fs/isofs/export.c | 4 ++--
fs/libfs.c | 4 ++--
fs/ocfs2/export.c | 4 ++--
fs/udf/namei.c | 4 ++--
fs/xfs/linux-2.6/xfs_export.c | 2 +-
mm/shmem.c | 14 +++++++-------
9 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index d937aaf..cac84f2 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -657,7 +657,7 @@ static struct dentry *fat_fh_to_dentry(struct super_block *sb,
u32 *fh = fid->raw;
if (fh_len < 5 || fh_type != 3)
- return NULL;
+ return ERR_PTR(-ESTALE);
inode = ilookup(sb, fh[0]);
if (!inode || inode->i_generation != fh[1]) {
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 2e99f34..0eb8990 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -653,7 +653,7 @@ static struct dentry *fuse_fh_to_dentry(struct super_block *sb,
struct fuse_inode_handle handle;
if ((fh_type != 0x81 && fh_type != 0x82) || fh_len < 3)
- return NULL;
+ return ERR_PTR(-ESTALE);
handle.nodeid = (u64) fid->raw[0] << 32;
handle.nodeid |= (u64) fid->raw[1];
@@ -667,7 +667,7 @@ static struct dentry *fuse_fh_to_parent(struct super_block *sb,
struct fuse_inode_handle parent;
if (fh_type != 0x82 || fh_len < 6)
- return NULL;
+ return ERR_PTR(-ESTALE);
parent.nodeid = (u64) fid->raw[3] << 32;
parent.nodeid |= (u64) fid->raw[4];
diff --git a/fs/gfs2/ops_export.c b/fs/gfs2/ops_export.c
index bbb8c36..2864873 100644
--- a/fs/gfs2/ops_export.c
+++ b/fs/gfs2/ops_export.c
@@ -254,7 +254,7 @@ static struct dentry *gfs2_fh_to_dentry(struct super_block *sb, struct fid *fid,
this.no_addr |= be32_to_cpu(fh[3]);
return gfs2_get_dentry(sb, &this);
default:
- return NULL;
+ return ERR_PTR(-ESTALE);
}
}
@@ -273,7 +273,7 @@ static struct dentry *gfs2_fh_to_parent(struct super_block *sb, struct fid *fid,
parent.no_addr |= be32_to_cpu(fh[7]);
return gfs2_get_dentry(sb, &parent);
default:
- return NULL;
+ return ERR_PTR(-ESTALE);
}
}
diff --git a/fs/isofs/export.c b/fs/isofs/export.c
index e81a305..1b4ee23 100644
--- a/fs/isofs/export.c
+++ b/fs/isofs/export.c
@@ -164,7 +164,7 @@ static struct dentry *isofs_fh_to_dentry(struct super_block *sb,
struct isofs_fid *ifid = (struct isofs_fid *)fid;
if (fh_len < 3 || fh_type > 2)
- return NULL;
+ return ERR_PTR(-ESTALE);
return isofs_export_iget(sb, ifid->block, ifid->offset,
ifid->generation);
@@ -176,7 +176,7 @@ static struct dentry *isofs_fh_to_parent(struct super_block *sb,
struct isofs_fid *ifid = (struct isofs_fid *)fid;
if (fh_type != 2)
- return NULL;
+ return ERR_PTR(-ESTALE);
return isofs_export_iget(sb,
fh_len > 2 ? ifid->parent_block : 0,
diff --git a/fs/libfs.c b/fs/libfs.c
index e960a83..4fe5b5b 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -751,7 +751,7 @@ struct dentry *generic_fh_to_dentry(struct super_block *sb, struct fid *fid,
struct inode *inode = NULL;
if (fh_len < 2)
- return NULL;
+ return ERR_PTR(-ESTALE);
switch (fh_type) {
case FILEID_INO32_GEN:
@@ -784,7 +784,7 @@ struct dentry *generic_fh_to_parent(struct super_block *sb, struct fid *fid,
struct inode *inode = NULL;
if (fh_len <= 2)
- return NULL;
+ return ERR_PTR(-ESTALE);
switch (fh_type) {
case FILEID_INO32_GEN_PARENT:
diff --git a/fs/ocfs2/export.c b/fs/ocfs2/export.c
index 2f27b33..0f1c80f 100644
--- a/fs/ocfs2/export.c
+++ b/fs/ocfs2/export.c
@@ -182,7 +182,7 @@ static struct dentry *ocfs2_fh_to_dentry(struct super_block *sb,
struct ocfs2_inode_handle handle;
if (fh_len < 3 || fh_type > 2)
- return NULL;
+ return ERR_PTR(-ESTALE);
handle.ih_blkno = (u64)le32_to_cpu(fid->raw[0]) << 32;
handle.ih_blkno |= (u64)le32_to_cpu(fid->raw[1]);
@@ -196,7 +196,7 @@ static struct dentry *ocfs2_fh_to_parent(struct super_block *sb,
struct ocfs2_inode_handle parent;
if (fh_type != 2 || fh_len < 6)
- return NULL;
+ return ERR_PTR(-ESTALE);
parent.ih_blkno = (u64)le32_to_cpu(fid->raw[3]) << 32;
parent.ih_blkno |= (u64)le32_to_cpu(fid->raw[4]);
diff --git a/fs/udf/namei.c b/fs/udf/namei.c
index f84bfaa..e1a1c16 100644
--- a/fs/udf/namei.c
+++ b/fs/udf/namei.c
@@ -1297,7 +1297,7 @@ static struct dentry *udf_fh_to_dentry(struct super_block *sb,
if ((fh_len != 3 && fh_len != 5) ||
(fh_type != FILEID_UDF_WITH_PARENT &&
fh_type != FILEID_UDF_WITHOUT_PARENT))
- return NULL;
+ return ERR_PTR(-ESTALE);
return udf_nfs_get_inode(sb, fid->udf.block, fid->udf.partref,
fid->udf.generation);
@@ -1307,7 +1307,7 @@ static struct dentry *udf_fh_to_parent(struct super_block *sb,
struct fid *fid, int fh_len, int fh_type)
{
if (fh_len != 5 || fh_type != FILEID_UDF_WITH_PARENT)
- return NULL;
+ return ERR_PTR(-ESTALE);
return udf_nfs_get_inode(sb, fid->udf.parent_block,
fid->udf.parent_partref,
diff --git a/fs/xfs/linux-2.6/xfs_export.c b/fs/xfs/linux-2.6/xfs_export.c
index 7f7abec..f9a51c4 100644
--- a/fs/xfs/linux-2.6/xfs_export.c
+++ b/fs/xfs/linux-2.6/xfs_export.c
@@ -150,7 +150,7 @@ xfs_fs_fh_to_dentry(struct super_block *sb, struct fid *fid,
struct inode *inode = NULL;
if (fh_len < xfs_fileid_length(fileid_type))
- return NULL;
+ return ERR_PTR(-ESTALE);
switch (fileid_type) {
case FILEID_INO32_GEN_PARENT:
diff --git a/mm/shmem.c b/mm/shmem.c
index f1b0d48..c9bdbf7 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2050,21 +2050,21 @@ static struct dentry *shmem_fh_to_dentry(struct super_block *sb,
struct fid *fid, int fh_len, int fh_type)
{
struct inode *inode;
- struct dentry *dentry = NULL;
+ struct dentry *dentry;
u64 inum = fid->raw[2];
inum = (inum << 32) | fid->raw[1];
if (fh_len < 3)
- return NULL;
+ return ERR_PTR(-ESTALE);
inode = ilookup5(sb, (unsigned long)(inum + fid->raw[0]),
shmem_match, fid->raw);
- if (inode) {
- dentry = d_find_alias(inode);
- iput(inode);
- }
+ if (!inode)
+ return ERR_PTR(-ESTALE);
- return dentry;
+ dentry = d_find_alias(inode);
+ iput(inode);
+ return dentry ?: ERR_PTR(-ESTALE);
}
static int shmem_encode_fh(struct dentry *dentry, __u32 *fh, int *len,
On Fri, 5 Dec 2008, David Howells wrote:
> Returning NULL from fh_to_dentry() and fh_to_parent() is not permitted, so
> return -ESTALE instead. exportfs_decode_fh() does not check for NULL, but
> will try to dereference it as IS_ERR() does not check for it.
>
> The generic_fh_to_dentry() and generic_fh_to_parent() functions also no longer
> return NULL, but return -ESTALE instead.
>
> Signed-off-by: David Howells <[email protected]>
> Acked-by: J. Bruce Fields <[email protected]>
Acked-by: Hugh Dickins <[email protected]>
Phew! Thanks.
> ---
>
> fs/fat/inode.c | 2 +-
> fs/fuse/inode.c | 4 ++--
> fs/gfs2/ops_export.c | 4 ++--
> fs/isofs/export.c | 4 ++--
> fs/libfs.c | 4 ++--
> fs/ocfs2/export.c | 4 ++--
> fs/udf/namei.c | 4 ++--
> fs/xfs/linux-2.6/xfs_export.c | 2 +-
> mm/shmem.c | 14 +++++++-------
> 9 files changed, 21 insertions(+), 21 deletions(-)
>
>
> diff --git a/fs/fat/inode.c b/fs/fat/inode.c
> index d937aaf..cac84f2 100644
> --- a/fs/fat/inode.c
> +++ b/fs/fat/inode.c
> @@ -657,7 +657,7 @@ static struct dentry *fat_fh_to_dentry(struct super_block *sb,
> u32 *fh = fid->raw;
>
> if (fh_len < 5 || fh_type != 3)
> - return NULL;
> + return ERR_PTR(-ESTALE);
>
> inode = ilookup(sb, fh[0]);
> if (!inode || inode->i_generation != fh[1]) {
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 2e99f34..0eb8990 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -653,7 +653,7 @@ static struct dentry *fuse_fh_to_dentry(struct super_block *sb,
> struct fuse_inode_handle handle;
>
> if ((fh_type != 0x81 && fh_type != 0x82) || fh_len < 3)
> - return NULL;
> + return ERR_PTR(-ESTALE);
>
> handle.nodeid = (u64) fid->raw[0] << 32;
> handle.nodeid |= (u64) fid->raw[1];
> @@ -667,7 +667,7 @@ static struct dentry *fuse_fh_to_parent(struct super_block *sb,
> struct fuse_inode_handle parent;
>
> if (fh_type != 0x82 || fh_len < 6)
> - return NULL;
> + return ERR_PTR(-ESTALE);
>
> parent.nodeid = (u64) fid->raw[3] << 32;
> parent.nodeid |= (u64) fid->raw[4];
> diff --git a/fs/gfs2/ops_export.c b/fs/gfs2/ops_export.c
> index bbb8c36..2864873 100644
> --- a/fs/gfs2/ops_export.c
> +++ b/fs/gfs2/ops_export.c
> @@ -254,7 +254,7 @@ static struct dentry *gfs2_fh_to_dentry(struct super_block *sb, struct fid *fid,
> this.no_addr |= be32_to_cpu(fh[3]);
> return gfs2_get_dentry(sb, &this);
> default:
> - return NULL;
> + return ERR_PTR(-ESTALE);
> }
> }
>
> @@ -273,7 +273,7 @@ static struct dentry *gfs2_fh_to_parent(struct super_block *sb, struct fid *fid,
> parent.no_addr |= be32_to_cpu(fh[7]);
> return gfs2_get_dentry(sb, &parent);
> default:
> - return NULL;
> + return ERR_PTR(-ESTALE);
> }
> }
>
> diff --git a/fs/isofs/export.c b/fs/isofs/export.c
> index e81a305..1b4ee23 100644
> --- a/fs/isofs/export.c
> +++ b/fs/isofs/export.c
> @@ -164,7 +164,7 @@ static struct dentry *isofs_fh_to_dentry(struct super_block *sb,
> struct isofs_fid *ifid = (struct isofs_fid *)fid;
>
> if (fh_len < 3 || fh_type > 2)
> - return NULL;
> + return ERR_PTR(-ESTALE);
>
> return isofs_export_iget(sb, ifid->block, ifid->offset,
> ifid->generation);
> @@ -176,7 +176,7 @@ static struct dentry *isofs_fh_to_parent(struct super_block *sb,
> struct isofs_fid *ifid = (struct isofs_fid *)fid;
>
> if (fh_type != 2)
> - return NULL;
> + return ERR_PTR(-ESTALE);
>
> return isofs_export_iget(sb,
> fh_len > 2 ? ifid->parent_block : 0,
> diff --git a/fs/libfs.c b/fs/libfs.c
> index e960a83..4fe5b5b 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -751,7 +751,7 @@ struct dentry *generic_fh_to_dentry(struct super_block *sb, struct fid *fid,
> struct inode *inode = NULL;
>
> if (fh_len < 2)
> - return NULL;
> + return ERR_PTR(-ESTALE);
>
> switch (fh_type) {
> case FILEID_INO32_GEN:
> @@ -784,7 +784,7 @@ struct dentry *generic_fh_to_parent(struct super_block *sb, struct fid *fid,
> struct inode *inode = NULL;
>
> if (fh_len <= 2)
> - return NULL;
> + return ERR_PTR(-ESTALE);
>
> switch (fh_type) {
> case FILEID_INO32_GEN_PARENT:
> diff --git a/fs/ocfs2/export.c b/fs/ocfs2/export.c
> index 2f27b33..0f1c80f 100644
> --- a/fs/ocfs2/export.c
> +++ b/fs/ocfs2/export.c
> @@ -182,7 +182,7 @@ static struct dentry *ocfs2_fh_to_dentry(struct super_block *sb,
> struct ocfs2_inode_handle handle;
>
> if (fh_len < 3 || fh_type > 2)
> - return NULL;
> + return ERR_PTR(-ESTALE);
>
> handle.ih_blkno = (u64)le32_to_cpu(fid->raw[0]) << 32;
> handle.ih_blkno |= (u64)le32_to_cpu(fid->raw[1]);
> @@ -196,7 +196,7 @@ static struct dentry *ocfs2_fh_to_parent(struct super_block *sb,
> struct ocfs2_inode_handle parent;
>
> if (fh_type != 2 || fh_len < 6)
> - return NULL;
> + return ERR_PTR(-ESTALE);
>
> parent.ih_blkno = (u64)le32_to_cpu(fid->raw[3]) << 32;
> parent.ih_blkno |= (u64)le32_to_cpu(fid->raw[4]);
> diff --git a/fs/udf/namei.c b/fs/udf/namei.c
> index f84bfaa..e1a1c16 100644
> --- a/fs/udf/namei.c
> +++ b/fs/udf/namei.c
> @@ -1297,7 +1297,7 @@ static struct dentry *udf_fh_to_dentry(struct super_block *sb,
> if ((fh_len != 3 && fh_len != 5) ||
> (fh_type != FILEID_UDF_WITH_PARENT &&
> fh_type != FILEID_UDF_WITHOUT_PARENT))
> - return NULL;
> + return ERR_PTR(-ESTALE);
>
> return udf_nfs_get_inode(sb, fid->udf.block, fid->udf.partref,
> fid->udf.generation);
> @@ -1307,7 +1307,7 @@ static struct dentry *udf_fh_to_parent(struct super_block *sb,
> struct fid *fid, int fh_len, int fh_type)
> {
> if (fh_len != 5 || fh_type != FILEID_UDF_WITH_PARENT)
> - return NULL;
> + return ERR_PTR(-ESTALE);
>
> return udf_nfs_get_inode(sb, fid->udf.parent_block,
> fid->udf.parent_partref,
> diff --git a/fs/xfs/linux-2.6/xfs_export.c b/fs/xfs/linux-2.6/xfs_export.c
> index 7f7abec..f9a51c4 100644
> --- a/fs/xfs/linux-2.6/xfs_export.c
> +++ b/fs/xfs/linux-2.6/xfs_export.c
> @@ -150,7 +150,7 @@ xfs_fs_fh_to_dentry(struct super_block *sb, struct fid *fid,
> struct inode *inode = NULL;
>
> if (fh_len < xfs_fileid_length(fileid_type))
> - return NULL;
> + return ERR_PTR(-ESTALE);
>
> switch (fileid_type) {
> case FILEID_INO32_GEN_PARENT:
> diff --git a/mm/shmem.c b/mm/shmem.c
> index f1b0d48..c9bdbf7 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2050,21 +2050,21 @@ static struct dentry *shmem_fh_to_dentry(struct super_block *sb,
> struct fid *fid, int fh_len, int fh_type)
> {
> struct inode *inode;
> - struct dentry *dentry = NULL;
> + struct dentry *dentry;
> u64 inum = fid->raw[2];
> inum = (inum << 32) | fid->raw[1];
>
> if (fh_len < 3)
> - return NULL;
> + return ERR_PTR(-ESTALE);
>
> inode = ilookup5(sb, (unsigned long)(inum + fid->raw[0]),
> shmem_match, fid->raw);
> - if (inode) {
> - dentry = d_find_alias(inode);
> - iput(inode);
> - }
> + if (!inode)
> + return ERR_PTR(-ESTALE);
>
> - return dentry;
> + dentry = d_find_alias(inode);
> + iput(inode);
> + return dentry ?: ERR_PTR(-ESTALE);
> }
>
> static int shmem_encode_fh(struct dentry *dentry, __u32 *fh, int *len,
On Fri, 5 Dec 2008, David Howells wrote:
>
> Returning NULL from fh_to_dentry() and fh_to_parent() is not permitted, so
> return -ESTALE instead. exportfs_decode_fh() does not check for NULL, but
> will try to dereference it as IS_ERR() does not check for it.
>
> The generic_fh_to_dentry() and generic_fh_to_parent() functions also no longer
> return NULL, but return -ESTALE instead.
I really don't like this one.
So just about every fh_to_dentry() returns NULL for some error case.
Instead of considering that a problem, maybe it should tell us something.
There is _one_ single caller of that function afaik, why shouldn't we just
teach the caller that dammit, lots of people want to just return NULL.
It's not like returning NULL doesn't make sense. Quite frankly, I think it
makes a lot more sense than returning -ESTALE, which is a very unnatural
error for most filesystems.
So quite frankly, I'd much rather just see the simpler patch. Have
exportfs handle the case.
Are there other callers?
Linus
---
fs/exportfs/expfs.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
index 80246ba..e9b378c 100644
--- a/fs/exportfs/expfs.c
+++ b/fs/exportfs/expfs.c
@@ -369,6 +369,8 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,
result = nop->fh_to_dentry(mnt->mnt_sb, fid, fh_len, fileid_type);
if (IS_ERR(result))
return result;
+ if (!result)
+ return ERR_PTR(-ESTALE);
if (S_ISDIR(result->d_inode->i_mode)) {
/*
On Fri, Dec 05, 2008 at 08:52:13AM -0800, Linus Torvalds wrote:
>
>
> On Fri, 5 Dec 2008, David Howells wrote:
> >
> > Returning NULL from fh_to_dentry() and fh_to_parent() is not permitted, so
> > return -ESTALE instead. exportfs_decode_fh() does not check for NULL, but
> > will try to dereference it as IS_ERR() does not check for it.
> >
> > The generic_fh_to_dentry() and generic_fh_to_parent() functions also no longer
> > return NULL, but return -ESTALE instead.
>
> I really don't like this one.
>
> So just about every fh_to_dentry() returns NULL for some error case.
> Instead of considering that a problem, maybe it should tell us something.
>
> There is _one_ single caller of that function afaik, why shouldn't we just
> teach the caller that dammit, lots of people want to just return NULL.
>
> It's not like returning NULL doesn't make sense. Quite frankly, I think it
> makes a lot more sense than returning -ESTALE, which is a very unnatural
> error for most filesystems.
>
> So quite frankly, I'd much rather just see the simpler patch. Have
> exportfs handle the case.
>
> Are there other callers?
Not of fh_to_dentry(), but it looks like fh_to_parent() is the same. So
you'd want the below.
This is partially reverting 440037287c5, which appears to have also do
some changing of NULLs to ESTALEs which we might also want to reconsider
at some point. Maybe Christoph could explain what the rationale was.
--b.
diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
index 80246ba..890e018 100644
--- a/fs/exportfs/expfs.c
+++ b/fs/exportfs/expfs.c
@@ -367,6 +367,8 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,
* 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;
@@ -420,6 +422,8 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,
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;
Linus Torvalds <[email protected]> wrote:
> It's not like returning NULL doesn't make sense. Quite frankly, I think it
> makes a lot more sense than returning -ESTALE, which is a very unnatural
> error for most filesystems.
The d_obtain_alias() function will immediately return -ESTALE if given a NULL
inode, though, and sometimes it'll return some other error.
It would also seem odd to sometimes return NULL to indicate an error, and
sometimes return a -ve error code to indicate an error. Perhaps one or the
other should be selected for consistency.
David
On Fri, 5 Dec 2008, David Howells wrote:
>
> The d_obtain_alias() function will immediately return -ESTALE if given a NULL
> inode, though, and sometimes it'll return some other error.
>
> It would also seem odd to sometimes return NULL to indicate an error, and
> sometimes return a -ve error code to indicate an error. Perhaps one or the
> other should be selected for consistency.
Well, the thing is, returning NULL is probably the most natural thing to
do for a filesystem that doesn't really _have_ a valid error code for the
situation. It's more of a "I can't do this" thing, than an error.
Sure, we can make all filesystems return -ESTALE, but not only is the
patch fairly big, it really doesn't look at all better or make any more
sense. ESTALE is really strictly a NFS thing - it doesn't tend to make
sense for other filesystems (well, sure, other filesystems may have inode
versions too and decide to use ESTALE for things, so I'm not claiming that
it's _purely_ a NFS thing, but I think you see my point).
So it really seems to make more sense to just make the ESTALE handling be
a NFS issue.
And notice that I'm not arguing that "fh_to_dentr/parent()" should
_always_ return NULL for errors. There may be real reasons why a
filesystem might want to return some actual error, like EIO. And maybe a
filesystem actually wants to return an explicit ESTALE when that makes
sense (ie when the filesystem _does_ find an inode that matches, but the
generation doesn't match).
But I'm just looking at your patch, and seeing things like
if (fh_len <= 2)
- return NULL;
+ return ERR_PTR(-ESTALE);
and going "That really didn't make the code any prettier or easier to
understand".
Linus
From: J. Bruce Fields <[email protected]>
While 440037287c5 "[PATCH] switch all filesystems over to
d_obtain_alias" removed some cases where fh_to_dentry() and
fh_to_parent() could return NULL, there are still a few NULL returns
left in individual filesystems. Thus it was a mistake for that commit
to remove the handling of NULL returns in the callers.
Revert those parts of 440037287c5 which removed the NULL handling.
(We could, alternatively, modify all implementations to return -ESTALE
instead of NULL, but that proves to require fixing a number of
filesystems, and in some cases it's arguably more natural to return
NULL.)
Thanks to David for original patch and Linus, Christoph, and Hugh for
review.
Signed-off-by: J. Bruce Fields <[email protected]>
Cc: David Howells <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Hugh Dickins <[email protected]>
---
fs/exportfs/expfs.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
On Fri, Dec 05, 2008 at 11:02:00AM -0800, Linus Torvalds wrote:
> So it really seems to make more sense to just make the ESTALE handling be
> a NFS issue.
This thread seems to have died out, and I didn't see any objections to
handling the error in the caller as Linus suggested, so here's the
patch.
Also available from the for-2.6.28 branch at
git://linux-nfs.org/~bfields/linux.git for-2.6.28
--b.
diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
index 80246ba..890e018 100644
--- a/fs/exportfs/expfs.c
+++ b/fs/exportfs/expfs.c
@@ -367,6 +367,8 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,
* 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;
@@ -420,6 +422,8 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,
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;
--
1.5.5.rc1
On Fri, Dec 05, 2008 at 12:42:28PM -0500, J. Bruce Fields wrote:
> Not of fh_to_dentry(), but it looks like fh_to_parent() is the same. So
> you'd want the below.
>
> This is partially reverting 440037287c5, which appears to have also do
> some changing of NULLs to ESTALEs which we might also want to reconsider
> at some point. Maybe Christoph could explain what the rationale was.
Miklos suggested always returning ERR_PTR values instead of a a min
or NULLs and ERR_PTR values to make the interface cleaner. I tend
to agree, but botched up the conversion.
J. Bruce Fields <[email protected]> wrote:
> + if (!result)
> + result = ERR_PTR(-ESTALE);
> if (IS_ERR(result))
> return result;
This is why I think this is a bad idea. This adds an extra conditional test
and branch. In fact, the way you've done it means you have to do the extra
branch whatever[*]; it might be better to have the if-statement you added
return directly.
[*] assuming gcc doesn't manage to be clever enough to omit the second test if
the first is true.
David