diff mbox

[linux-cifs-client] cifs: Fix incorrect return code being printed in cFYI messages

Message ID 4A4370BA.9050506@suse.de (mailing list archive)
State New, archived
Headers show

Commit Message

Suresh Jayaraman June 25, 2009, 12:42 p.m. UTC 
FreeXid() along with freeing Xid does add a cifsFYI debug message that
prints rc (return code) as well. In some code paths where we set/return
error code after calling FreeXid(), incorrect error code is being
printed when cifsFYI is enabled.

This could be misleading in few cases. For eg.
In cifs_open() if cifs_fill_filedata() returns a valid pointer to
cifsFileInfo, FreeXid() prints rc=-13 whereas 0 is actually being
returned. Fix this by setting rc before calling FreeXid().

Basically convert

FreeXid(xid);			rc = -ERR;
return -ERR;		=>	FreeXid(xid);
				return rc;

Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
---
 fs/cifs/dir.c   |    6 ++++--
 fs/cifs/file.c  |   24 ++++++++++++++++--------
 fs/cifs/inode.c |   15 ++++++++++-----
 fs/cifs/link.c  |    3 ++-
 fs/cifs/xattr.c |   12 ++++++++----
 5 files changed, 40 insertions(+), 20 deletions(-)

Comments

Christoph Hellwig June 25, 2009, 3:50 p.m. UTC | #1 
On Thu, Jun 25, 2009 at 06:12:34PM +0530, Suresh Jayaraman wrote:
> 
> FreeXid() along with freeing Xid does add a cifsFYI debug message that
> prints rc (return code) as well. In some code paths where we set/return
> error code after calling FreeXid(), incorrect error code is being
> printed when cifsFYI is enabled.
> 
> This could be misleading in few cases. For eg.
> In cifs_open() if cifs_fill_filedata() returns a valid pointer to
> cifsFileInfo, FreeXid() prints rc=-13 whereas 0 is actually being
> returned. Fix this by setting rc before calling FreeXid().
> 
> Basically convert

I'd say just kill the xid crap instead of adding cumbersome workarounds
due to the awkward way they are defined.
Steve French June 25, 2009, 3:52 p.m. UTC | #2 
I don't mind killing them, if there is a quick and easy way to catch
the function returns for all of the cifs vfs exits, but we use these
all the time for debugging, so need an alternative to catch all of the
entry/exits from the vfs to cifs.

On Thu, Jun 25, 2009 at 10:50 AM, Christoph Hellwig<hch@infradead.org> wrote:
> On Thu, Jun 25, 2009 at 06:12:34PM +0530, Suresh Jayaraman wrote:
>>
>> FreeXid() along with freeing Xid does add a cifsFYI debug message that
>> prints rc (return code) as well. In some code paths where we set/return
>> error code after calling FreeXid(), incorrect error code is being
>> printed when cifsFYI is enabled.
>>
>> This could be misleading in few cases. For eg.
>> In cifs_open() if cifs_fill_filedata() returns a valid pointer to
>> cifsFileInfo, FreeXid() prints rc=-13 whereas 0 is actually being
>> returned. Fix this by setting rc before calling FreeXid().
>>
>> Basically convert
>
> I'd say just kill the xid crap instead of adding cumbersome workarounds
> due to the awkward way they are defined.
>
>
Jeff Layton June 25, 2009, 5:13 p.m. UTC | #3 
On Thu, 25 Jun 2009 18:12:34 +0530
Suresh Jayaraman <sjayaraman@suse.de> wrote:

> 
> FreeXid() along with freeing Xid does add a cifsFYI debug message that
> prints rc (return code) as well. In some code paths where we set/return
> error code after calling FreeXid(), incorrect error code is being
> printed when cifsFYI is enabled.
> 
> This could be misleading in few cases. For eg.
> In cifs_open() if cifs_fill_filedata() returns a valid pointer to
> cifsFileInfo, FreeXid() prints rc=-13 whereas 0 is actually being
> returned. Fix this by setting rc before calling FreeXid().
> 
> Basically convert
> 
> FreeXid(xid);			rc = -ERR;
> return -ERR;		=>	FreeXid(xid);
> 				return rc;
> 
> Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
> ---
>  fs/cifs/dir.c   |    6 ++++--
>  fs/cifs/file.c  |   24 ++++++++++++++++--------
>  fs/cifs/inode.c |   15 ++++++++++-----
>  fs/cifs/link.c  |    3 ++-
>  fs/cifs/xattr.c |   12 ++++++++----
>  5 files changed, 40 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> index 3758965..7dc6b74 100644
> --- a/fs/cifs/dir.c
> +++ b/fs/cifs/dir.c
> @@ -307,8 +307,9 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
>  
>  	full_path = build_path_from_dentry(direntry);
>  	if (full_path == NULL) {
> +		rc = -ENOMEM;
>  		FreeXid(xid);
> -		return -ENOMEM;
> +		return rc;
>  	}
>  
>  	if (oplockEnabled)
> @@ -540,8 +541,9 @@ int cifs_mknod(struct inode *inode, struct dentry *direntry, int mode,
>  			buf = kmalloc(sizeof(FILE_ALL_INFO), GFP_KERNEL);
>  			if (buf == NULL) {
>  				kfree(full_path);
> +				rc = -ENOMEM;
>  				FreeXid(xid);
> -				return -ENOMEM;
> +				return rc;
>  			}
>  
>  			rc = CIFSSMBOpen(xid, pTcon, full_path,
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 0686684..ebdbe62 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -300,14 +300,16 @@ int cifs_open(struct inode *inode, struct file *file)
>  	pCifsInode = CIFS_I(file->f_path.dentry->d_inode);
>  	pCifsFile = cifs_fill_filedata(file);
>  	if (pCifsFile) {
> +		rc = 0;
>  		FreeXid(xid);
> -		return 0;
> +		return rc;
>  	}
>  
>  	full_path = build_path_from_dentry(file->f_path.dentry);
>  	if (full_path == NULL) {
> +		rc = -ENOMEM;
>  		FreeXid(xid);
> -		return -ENOMEM;
> +		return rc;
>  	}
>  
>  	cFYI(1, ("inode = 0x%p file flags are 0x%x for %s",
> @@ -494,8 +496,9 @@ static int cifs_reopen_file(struct file *file, bool can_flush)
>  	mutex_unlock(&pCifsFile->fh_mutex);
>  	if (!pCifsFile->invalidHandle) {
>  		mutex_lock(&pCifsFile->fh_mutex);
> +		rc = 0;
>  		FreeXid(xid);
> -		return 0;
> +		return rc;
>  	}
>  
>  	if (file->f_path.dentry == NULL) {
> @@ -845,8 +848,9 @@ int cifs_lock(struct file *file, int cmd, struct file_lock *pfLock)
>  	tcon = cifs_sb->tcon;
>  
>  	if (file->private_data == NULL) {
> +		rc = -EBADF;
>  		FreeXid(xid);
> -		return -EBADF;
> +		return rc;
>  	}
>  	netfid = ((struct cifsFileInfo *)file->private_data)->netfid;
>  
> @@ -1805,8 +1809,9 @@ ssize_t cifs_user_read(struct file *file, char __user *read_data,
>  	pTcon = cifs_sb->tcon;
>  
>  	if (file->private_data == NULL) {
> +		rc = -EBADF;
>  		FreeXid(xid);
> -		return -EBADF;
> +		return rc;
>  	}
>  	open_file = (struct cifsFileInfo *)file->private_data;
>  
> @@ -1885,8 +1890,9 @@ static ssize_t cifs_read(struct file *file, char *read_data, size_t read_size,
>  	pTcon = cifs_sb->tcon;
>  
>  	if (file->private_data == NULL) {
> +		rc = -EBADF;
>  		FreeXid(xid);
> -		return -EBADF;
> +		return rc;
>  	}
>  	open_file = (struct cifsFileInfo *)file->private_data;
>  
> @@ -2019,8 +2025,9 @@ static int cifs_readpages(struct file *file, struct address_space *mapping,
>  
>  	xid = GetXid();
>  	if (file->private_data == NULL) {
> +		rc = -EBADF;
>  		FreeXid(xid);
> -		return -EBADF;
> +		return rc;
>  	}
>  	open_file = (struct cifsFileInfo *)file->private_data;
>  	cifs_sb = CIFS_SB(file->f_path.dentry->d_sb);
> @@ -2185,8 +2192,9 @@ static int cifs_readpage(struct file *file, struct page *page)
>  	xid = GetXid();
>  
>  	if (file->private_data == NULL) {
> +		rc = -EBADF;
>  		FreeXid(xid);
> -		return -EBADF;
> +		return rc;
>  	}
>  
>  	cFYI(1, ("readpage %p at offset %d 0x%x\n",
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index fad882b..155c9e7 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -988,8 +988,9 @@ int cifs_unlink(struct inode *dir, struct dentry *dentry)
>  	 * sb->s_vfs_rename_mutex here */
>  	full_path = build_path_from_dentry(dentry);
>  	if (full_path == NULL) {
> +		rc = -ENOMEM;
>  		FreeXid(xid);
> -		return -ENOMEM;
> +		return rc;
>  	}
>  
>  	if ((tcon->ses->capabilities & CAP_UNIX) &&
> @@ -1118,8 +1119,9 @@ int cifs_mkdir(struct inode *inode, struct dentry *direntry, int mode)
>  
>  	full_path = build_path_from_dentry(direntry);
>  	if (full_path == NULL) {
> +		rc = -ENOMEM;
>  		FreeXid(xid);
> -		return -ENOMEM;
> +		return rc;
>  	}
>  
>  	if ((pTcon->ses->capabilities & CAP_UNIX) &&
> @@ -1303,8 +1305,9 @@ int cifs_rmdir(struct inode *inode, struct dentry *direntry)
>  
>  	full_path = build_path_from_dentry(direntry);
>  	if (full_path == NULL) {
> +		rc = -ENOMEM;
>  		FreeXid(xid);
> -		return -ENOMEM;
> +		return rc;
>  	}
>  
>  	rc = CIFSSMBRmDir(xid, pTcon, full_path, cifs_sb->local_nls,
> @@ -1508,8 +1511,9 @@ int cifs_revalidate(struct dentry *direntry)
>  	   since that would deadlock */
>  	full_path = build_path_from_dentry(direntry);
>  	if (full_path == NULL) {
> +		rc = -ENOMEM;
>  		FreeXid(xid);
> -		return -ENOMEM;
> +		return rc;
>  	}
>  	cFYI(1, ("Revalidate: %s inode 0x%p count %d dentry: 0x%p d_time %ld "
>  		 "jiffies %ld", full_path, direntry->d_inode,
> @@ -1911,8 +1915,9 @@ cifs_setattr_nounix(struct dentry *direntry, struct iattr *attrs)
>  
>  	full_path = build_path_from_dentry(direntry);
>  	if (full_path == NULL) {
> +		rc = -ENOMEM;
>  		FreeXid(xid);
> -		return -ENOMEM;
> +		return rc;
>  	}
>  
>  	/*
> diff --git a/fs/cifs/link.c b/fs/cifs/link.c
> index cd83c53..fc1e048 100644
> --- a/fs/cifs/link.c
> +++ b/fs/cifs/link.c
> @@ -172,8 +172,9 @@ cifs_symlink(struct inode *inode, struct dentry *direntry, const char *symname)
>  	full_path = build_path_from_dentry(direntry);
>  
>  	if (full_path == NULL) {
> +		rc = -ENOMEM;
>  		FreeXid(xid);
> -		return -ENOMEM;
> +		return rc;
>  	}
>  
>  	cFYI(1, ("Full path: %s", full_path));
> diff --git a/fs/cifs/xattr.c b/fs/cifs/xattr.c
> index e9527ee..a75afa3 100644
> --- a/fs/cifs/xattr.c
> +++ b/fs/cifs/xattr.c
> @@ -64,8 +64,9 @@ int cifs_removexattr(struct dentry *direntry, const char *ea_name)
>  
>  	full_path = build_path_from_dentry(direntry);
>  	if (full_path == NULL) {
> +		rc = -ENOMEM;
>  		FreeXid(xid);
> -		return -ENOMEM;
> +		return rc;
>  	}
>  	if (ea_name == NULL) {
>  		cFYI(1, ("Null xattr names not supported"));
> @@ -118,8 +119,9 @@ int cifs_setxattr(struct dentry *direntry, const char *ea_name,
>  
>  	full_path = build_path_from_dentry(direntry);
>  	if (full_path == NULL) {
> +		rc = -ENOMEM;
>  		FreeXid(xid);
> -		return -ENOMEM;
> +		return rc;
>  	}
>  	/* return dos attributes as pseudo xattr */
>  	/* return alt name if available as pseudo attr */
> @@ -225,8 +227,9 @@ ssize_t cifs_getxattr(struct dentry *direntry, const char *ea_name,
>  
>  	full_path = build_path_from_dentry(direntry);
>  	if (full_path == NULL) {
> +		rc = -ENOMEM;
>  		FreeXid(xid);
> -		return -ENOMEM;
> +		return rc;
>  	}
>  	/* return dos attributes as pseudo xattr */
>  	/* return alt name if available as pseudo attr */
> @@ -351,8 +354,9 @@ ssize_t cifs_listxattr(struct dentry *direntry, char *data, size_t buf_size)
>  
>  	full_path = build_path_from_dentry(direntry);
>  	if (full_path == NULL) {
> +		rc = -ENOMEM;
>  		FreeXid(xid);
> -		return -ENOMEM;
> +		return rc;
>  	}
>  	/* return dos attributes as pseudo xattr */
>  	/* return alt name if available as pseudo attr */
> _______________________________________________
> linux-cifs-client mailing list
> linux-cifs-client@lists.samba.org
> https://lists.samba.org/mailman/listinfo/linux-cifs-client
> 

I agree with Christoph. It would be good to see this moved to something
better (particularly since GetXid involves taking a global spinlock
too). In the meantime however, this appears to be harmless and might
help with debugging so...

Acked-by: Jeff Layton <jlayton@redhat.com>
diff mbox

Patch

diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 3758965..7dc6b74 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -307,8 +307,9 @@  cifs_create(struct inode *inode, struct dentry *direntry, int mode,
 
 	full_path = build_path_from_dentry(direntry);
 	if (full_path == NULL) {
+		rc = -ENOMEM;
 		FreeXid(xid);
-		return -ENOMEM;
+		return rc;
 	}
 
 	if (oplockEnabled)
@@ -540,8 +541,9 @@  int cifs_mknod(struct inode *inode, struct dentry *direntry, int mode,
 			buf = kmalloc(sizeof(FILE_ALL_INFO), GFP_KERNEL);
 			if (buf == NULL) {
 				kfree(full_path);
+				rc = -ENOMEM;
 				FreeXid(xid);
-				return -ENOMEM;
+				return rc;
 			}
 
 			rc = CIFSSMBOpen(xid, pTcon, full_path,
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 0686684..ebdbe62 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -300,14 +300,16 @@  int cifs_open(struct inode *inode, struct file *file)
 	pCifsInode = CIFS_I(file->f_path.dentry->d_inode);
 	pCifsFile = cifs_fill_filedata(file);
 	if (pCifsFile) {
+		rc = 0;
 		FreeXid(xid);
-		return 0;
+		return rc;
 	}
 
 	full_path = build_path_from_dentry(file->f_path.dentry);
 	if (full_path == NULL) {
+		rc = -ENOMEM;
 		FreeXid(xid);
-		return -ENOMEM;
+		return rc;
 	}
 
 	cFYI(1, ("inode = 0x%p file flags are 0x%x for %s",
@@ -494,8 +496,9 @@  static int cifs_reopen_file(struct file *file, bool can_flush)
 	mutex_unlock(&pCifsFile->fh_mutex);
 	if (!pCifsFile->invalidHandle) {
 		mutex_lock(&pCifsFile->fh_mutex);
+		rc = 0;
 		FreeXid(xid);
-		return 0;
+		return rc;
 	}
 
 	if (file->f_path.dentry == NULL) {
@@ -845,8 +848,9 @@  int cifs_lock(struct file *file, int cmd, struct file_lock *pfLock)
 	tcon = cifs_sb->tcon;
 
 	if (file->private_data == NULL) {
+		rc = -EBADF;
 		FreeXid(xid);
-		return -EBADF;
+		return rc;
 	}
 	netfid = ((struct cifsFileInfo *)file->private_data)->netfid;
 
@@ -1805,8 +1809,9 @@  ssize_t cifs_user_read(struct file *file, char __user *read_data,
 	pTcon = cifs_sb->tcon;
 
 	if (file->private_data == NULL) {
+		rc = -EBADF;
 		FreeXid(xid);
-		return -EBADF;
+		return rc;
 	}
 	open_file = (struct cifsFileInfo *)file->private_data;
 
@@ -1885,8 +1890,9 @@  static ssize_t cifs_read(struct file *file, char *read_data, size_t read_size,
 	pTcon = cifs_sb->tcon;
 
 	if (file->private_data == NULL) {
+		rc = -EBADF;
 		FreeXid(xid);
-		return -EBADF;
+		return rc;
 	}
 	open_file = (struct cifsFileInfo *)file->private_data;
 
@@ -2019,8 +2025,9 @@  static int cifs_readpages(struct file *file, struct address_space *mapping,
 
 	xid = GetXid();
 	if (file->private_data == NULL) {
+		rc = -EBADF;
 		FreeXid(xid);
-		return -EBADF;
+		return rc;
 	}
 	open_file = (struct cifsFileInfo *)file->private_data;
 	cifs_sb = CIFS_SB(file->f_path.dentry->d_sb);
@@ -2185,8 +2192,9 @@  static int cifs_readpage(struct file *file, struct page *page)
 	xid = GetXid();
 
 	if (file->private_data == NULL) {
+		rc = -EBADF;
 		FreeXid(xid);
-		return -EBADF;
+		return rc;
 	}
 
 	cFYI(1, ("readpage %p at offset %d 0x%x\n",
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index fad882b..155c9e7 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -988,8 +988,9 @@  int cifs_unlink(struct inode *dir, struct dentry *dentry)
 	 * sb->s_vfs_rename_mutex here */
 	full_path = build_path_from_dentry(dentry);
 	if (full_path == NULL) {
+		rc = -ENOMEM;
 		FreeXid(xid);
-		return -ENOMEM;
+		return rc;
 	}
 
 	if ((tcon->ses->capabilities & CAP_UNIX) &&
@@ -1118,8 +1119,9 @@  int cifs_mkdir(struct inode *inode, struct dentry *direntry, int mode)
 
 	full_path = build_path_from_dentry(direntry);
 	if (full_path == NULL) {
+		rc = -ENOMEM;
 		FreeXid(xid);
-		return -ENOMEM;
+		return rc;
 	}
 
 	if ((pTcon->ses->capabilities & CAP_UNIX) &&
@@ -1303,8 +1305,9 @@  int cifs_rmdir(struct inode *inode, struct dentry *direntry)
 
 	full_path = build_path_from_dentry(direntry);
 	if (full_path == NULL) {
+		rc = -ENOMEM;
 		FreeXid(xid);
-		return -ENOMEM;
+		return rc;
 	}
 
 	rc = CIFSSMBRmDir(xid, pTcon, full_path, cifs_sb->local_nls,
@@ -1508,8 +1511,9 @@  int cifs_revalidate(struct dentry *direntry)
 	   since that would deadlock */
 	full_path = build_path_from_dentry(direntry);
 	if (full_path == NULL) {
+		rc = -ENOMEM;
 		FreeXid(xid);
-		return -ENOMEM;
+		return rc;
 	}
 	cFYI(1, ("Revalidate: %s inode 0x%p count %d dentry: 0x%p d_time %ld "
 		 "jiffies %ld", full_path, direntry->d_inode,
@@ -1911,8 +1915,9 @@  cifs_setattr_nounix(struct dentry *direntry, struct iattr *attrs)
 
 	full_path = build_path_from_dentry(direntry);
 	if (full_path == NULL) {
+		rc = -ENOMEM;
 		FreeXid(xid);
-		return -ENOMEM;
+		return rc;
 	}
 
 	/*
diff --git a/fs/cifs/link.c b/fs/cifs/link.c
index cd83c53..fc1e048 100644
--- a/fs/cifs/link.c
+++ b/fs/cifs/link.c
@@ -172,8 +172,9 @@  cifs_symlink(struct inode *inode, struct dentry *direntry, const char *symname)
 	full_path = build_path_from_dentry(direntry);
 
 	if (full_path == NULL) {
+		rc = -ENOMEM;
 		FreeXid(xid);
-		return -ENOMEM;
+		return rc;
 	}
 
 	cFYI(1, ("Full path: %s", full_path));
diff --git a/fs/cifs/xattr.c b/fs/cifs/xattr.c
index e9527ee..a75afa3 100644
--- a/fs/cifs/xattr.c
+++ b/fs/cifs/xattr.c
@@ -64,8 +64,9 @@  int cifs_removexattr(struct dentry *direntry, const char *ea_name)
 
 	full_path = build_path_from_dentry(direntry);
 	if (full_path == NULL) {
+		rc = -ENOMEM;
 		FreeXid(xid);
-		return -ENOMEM;
+		return rc;
 	}
 	if (ea_name == NULL) {
 		cFYI(1, ("Null xattr names not supported"));
@@ -118,8 +119,9 @@  int cifs_setxattr(struct dentry *direntry, const char *ea_name,
 
 	full_path = build_path_from_dentry(direntry);
 	if (full_path == NULL) {
+		rc = -ENOMEM;
 		FreeXid(xid);
-		return -ENOMEM;
+		return rc;
 	}
 	/* return dos attributes as pseudo xattr */
 	/* return alt name if available as pseudo attr */
@@ -225,8 +227,9 @@  ssize_t cifs_getxattr(struct dentry *direntry, const char *ea_name,
 
 	full_path = build_path_from_dentry(direntry);
 	if (full_path == NULL) {
+		rc = -ENOMEM;
 		FreeXid(xid);
-		return -ENOMEM;
+		return rc;
 	}
 	/* return dos attributes as pseudo xattr */
 	/* return alt name if available as pseudo attr */
@@ -351,8 +354,9 @@  ssize_t cifs_listxattr(struct dentry *direntry, char *data, size_t buf_size)
 
 	full_path = build_path_from_dentry(direntry);
 	if (full_path == NULL) {
+		rc = -ENOMEM;
 		FreeXid(xid);
-		return -ENOMEM;
+		return rc;
 	}
 	/* return dos attributes as pseudo xattr */
 	/* return alt name if available as pseudo attr */