| Message ID | 1274466317-28231-2-git-send-email-jlayton@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 21 May 2010 14:45:55 -0400 Josef Bacik <josef@redhat.com> wrote: > On Fri, May 21, 2010 at 02:25:14PM -0400, Jeff Layton wrote: > > cifs_lookup doesn't actually return a dentry. It instantiates the one > > that's passed in, but callers don't have any way to know if the lookup > > succeeded. > > > > Signed-off-by: Jeff Layton <jlayton@redhat.com> > > --- > > fs/cifs/dir.c | 13 +++++++++---- > > 1 files changed, 9 insertions(+), 4 deletions(-) > > > > diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c > > index 391816b..54de8e5 100644 > > --- a/fs/cifs/dir.c > > +++ b/fs/cifs/dir.c > > @@ -639,6 +639,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry, > > struct inode *newInode = NULL; > > char *full_path = NULL; > > struct file *filp; > > + struct dentry *res; > > > > xid = GetXid(); > > > > @@ -738,7 +739,8 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry, > > /* since paths are not looked up by component - the parent > > directories are presumed to be good here */ > > renew_parental_timestamps(direntry); > > - > > + res = direntry; > > + dget(res); > > } else if (rc == -ENOENT) { > > rc = 0; > > direntry->d_time = jiffies; > > @@ -747,17 +749,20 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry, > > else > > direntry->d_op = &cifs_dentry_ops; > > d_add(direntry, NULL); > > - /* if it was once a directory (but how can we tell?) we could do > > - shrink_dcache_parent(direntry); */ > > + res = direntry; > > + dget(res); > > Should probably do > > res = dget(direntry) > Ahh good catch. Will fix. Thanks,
On Fri, May 21, 2010 at 02:25:14PM -0400, Jeff Layton wrote: > cifs_lookup doesn't actually return a dentry. It instantiates the one > that's passed in, but callers don't have any way to know if the lookup > succeeded. > > Signed-off-by: Jeff Layton <jlayton@redhat.com> > --- > fs/cifs/dir.c | 13 +++++++++---- > 1 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c > index 391816b..54de8e5 100644 > --- a/fs/cifs/dir.c > +++ b/fs/cifs/dir.c > @@ -639,6 +639,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry, > struct inode *newInode = NULL; > char *full_path = NULL; > struct file *filp; > + struct dentry *res; > > xid = GetXid(); > > @@ -738,7 +739,8 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry, > /* since paths are not looked up by component - the parent > directories are presumed to be good here */ > renew_parental_timestamps(direntry); > - > + res = direntry; > + dget(res); > } else if (rc == -ENOENT) { > rc = 0; > direntry->d_time = jiffies; > @@ -747,17 +749,20 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry, > else > direntry->d_op = &cifs_dentry_ops; > d_add(direntry, NULL); > - /* if it was once a directory (but how can we tell?) we could do > - shrink_dcache_parent(direntry); */ > + res = direntry; > + dget(res); Should probably do res = dget(direntry) here and above. Thanks, Josef
On Fri, May 21, 2010 at 02:25:14PM -0400, Jeff Layton wrote: > cifs_lookup doesn't actually return a dentry. It instantiates the one > that's passed in, but callers don't have any way to know if the lookup > succeeded. Huh? Of course they do - ->lookup() has every right to do just that; d_add() and return NULL is perfectly legitimate.
On Sat, 22 May 2010 14:30:51 +0100 Al Viro <viro@ZenIV.linux.org.uk> wrote: > On Fri, May 21, 2010 at 02:25:14PM -0400, Jeff Layton wrote: > > cifs_lookup doesn't actually return a dentry. It instantiates the one > > that's passed in, but callers don't have any way to know if the lookup > > succeeded. > > Huh? Of course they do - ->lookup() has every right to do just that; > d_add() and return NULL is perfectly legitimate. OK, bad description on my part... Is one way of doing this preferred over another?
On Sat, May 22, 2010 at 10:08:36AM -0400, Jeff Layton wrote: > On Sat, 22 May 2010 14:30:51 +0100 > Al Viro <viro@ZenIV.linux.org.uk> wrote: > > > On Fri, May 21, 2010 at 02:25:14PM -0400, Jeff Layton wrote: > > > cifs_lookup doesn't actually return a dentry. It instantiates the one > > > that's passed in, but callers don't have any way to know if the lookup > > > succeeded. > > > > Huh? Of course they do - ->lookup() has every right to do just that; > > d_add() and return NULL is perfectly legitimate. > > OK, bad description on my part... Is one way of doing this preferred > over another? Non-NULL, non ERR_PTR() strongly implies that you have found a different dentry and tell the caller to use it instead; returning the argument (after having cached it and bumped its refcount) will work, but it's pretty much a deliberate obfuscation.
On Sat, 22 May 2010 15:46:15 +0100 Al Viro <viro@ZenIV.linux.org.uk> wrote: > On Sat, May 22, 2010 at 10:08:36AM -0400, Jeff Layton wrote: > > On Sat, 22 May 2010 14:30:51 +0100 > > Al Viro <viro@ZenIV.linux.org.uk> wrote: > > > > > On Fri, May 21, 2010 at 02:25:14PM -0400, Jeff Layton wrote: > > > > cifs_lookup doesn't actually return a dentry. It instantiates the one > > > > that's passed in, but callers don't have any way to know if the lookup > > > > succeeded. > > > > > > Huh? Of course they do - ->lookup() has every right to do just that; > > > d_add() and return NULL is perfectly legitimate. > > > > OK, bad description on my part... Is one way of doing this preferred > > over another? > > Non-NULL, non ERR_PTR() strongly implies that you have found a different > dentry and tell the caller to use it instead; returning the argument (after > having cached it and bumped its refcount) will work, but it's pretty much > a deliberate obfuscation. Ok, thanks for the explanation. In that case, I'll plan to drop this patch as the existing behavior is more clear. Cheers,
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c index 391816b..54de8e5 100644 --- a/fs/cifs/dir.c +++ b/fs/cifs/dir.c @@ -639,6 +639,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry, struct inode *newInode = NULL; char *full_path = NULL; struct file *filp; + struct dentry *res; xid = GetXid(); @@ -738,7 +739,8 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry, /* since paths are not looked up by component - the parent directories are presumed to be good here */ renew_parental_timestamps(direntry); - + res = direntry; + dget(res); } else if (rc == -ENOENT) { rc = 0; direntry->d_time = jiffies; @@ -747,17 +749,20 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry, else direntry->d_op = &cifs_dentry_ops; d_add(direntry, NULL); - /* if it was once a directory (but how can we tell?) we could do - shrink_dcache_parent(direntry); */ + res = direntry; + dget(res); } else if (rc != -EACCES) { cERROR(1, "Unexpected lookup error %d", rc); /* We special case check for Access Denied - since that is a common return code */ + res = ERR_PTR(rc); + } else { + res = ERR_PTR(rc); } kfree(full_path); FreeXid(xid); - return ERR_PTR(rc); + return res; } static int
cifs_lookup doesn't actually return a dentry. It instantiates the one that's passed in, but callers don't have any way to know if the lookup succeeded. Signed-off-by: Jeff Layton <jlayton@redhat.com> --- fs/cifs/dir.c | 13 +++++++++---- 1 files changed, 9 insertions(+), 4 deletions(-)