Patchwork [1/4] cifs: make cifs_lookup return a dentry

login
register
mail settings
Submitter Jeff Layton
Date May 21, 2010, 6:25 p.m.
Message ID <1274466317-28231-2-git-send-email-jlayton@redhat.com>
Download mbox | patch
Permalink /patch/53200/
State New
Headers show

Comments

Jeff Layton - May 21, 2010, 6:25 p.m.
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(-)
Jeff Layton - May 21, 2010, 6:42 p.m.
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,
Josef Bacik - May 21, 2010, 6:45 p.m.
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
Al Viro - May 22, 2010, 1:30 p.m.
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.
Jeff Layton - May 22, 2010, 2:08 p.m.
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?
Al Viro - May 22, 2010, 2:46 p.m.
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.
Jeff Layton - May 22, 2010, 3:23 p.m.
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,

Patch

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