cifs: remove useless cargo-cult checks

PATCH In review Christoph Hellwig 2 participants 3 messages Mar 7, 2007 1 message not in the archive

Share & tools

Christoph Hellwig

author

2007-03-07 15:29 UTC #0 lore raw

PATCH 1/1 cifs: remove useless cargo-cult checks

file->f_path.dentry or file->f_path.dentry.d_inode can't be NULL since at least ten years, similar for all but very few arguments passed in from the VFS.

Btw, it would be nice if all CIFS updates could be Cc'ed to lkml to fix things like this or the horrible codingstyle before it hits the tree.

Signed-off-by Christoph Hellwig <[email protected]>

fs/cifs/cifsfs.c +1 -2

Index: linux-2.6/fs/cifs/cifsfs.c
===================================================================
@@ -521,8 +521,7 @@
 		/* some applications poll for the file length in this strange
 		   way so we must seek to end on non-oplocked files by
 		   setting the revalidate time to zero */
-		if(file->f_path.dentry->d_inode)		
-			CIFS_I(file->f_path.dentry->d_inode)->time = 0;
+		CIFS_I(file->f_path.dentry->d_inode)->time = 0;
 
 		retval = cifs_revalidate(file->f_path.dentry);
 		if (retval < 0)

fs/cifs/file.c +19 ~~-65~~

159 lines not on this page · raw message

fs/cifs/readdir.c +3 ~~-24~~

Index: linux-2.6/fs/cifs/readdir.c
===================================================================
@@ -436,9 +436,6 @@
 	cifsFile->invalidHandle = TRUE;
 	cifsFile->srch_inf.endOfSearch = FALSE;
 
-	if(file->f_path.dentry == NULL)
-		return -ENOENT;
-
 	cifs_sb = CIFS_SB(file->f_path.dentry->d_sb);
 	if(cifs_sb == NULL)
 		return -EINVAL;

@@ -610,20 +607,10 @@
    whether we can use the cached search results from the previous search */
 static int is_dir_changed(struct file * file)
 {
-	struct inode * inode;
-	struct cifsInodeInfo *cifsInfo;
-
-	if(file->f_path.dentry == NULL)
-		return 0;
-
-	inode = file->f_path.dentry->d_inode;
-
-	if(inode == NULL)
-		return 0;
-
-	cifsInfo = CIFS_I(inode);
+	struct inode *inode = file->f_path.dentry->d_inode;
+	struct cifsInodeInfo *cifsInfo = CIFS_I(inode);
 
-	if(cifsInfo->time == 0)
+	if (cifsInfo->time == 0)
 		return 1; /* directory was changed, perhaps due to unlink */
 	else
 		return 0;

@@ -843,9 +830,6 @@
 	if((scratch_buf == NULL) || (pfindEntry == NULL) || (pCifsF == NULL))
 		return -ENOENT;
 
-	if(file->f_path.dentry == NULL)
-		return -ENOENT;
-
 	rc = cifs_entry_is_dot(pfindEntry,pCifsF);
 	/* skip . and .. since we added them first */
 	if(rc != 0)

@@ -989,11 +973,6 @@
 
 	xid = GetXid();
 
-	if(file->f_path.dentry == NULL) {
-		FreeXid(xid);
-		return -EIO;
-	}
-
 	cifs_sb = CIFS_SB(file->f_path.dentry->d_sb);
 	pTcon = cifs_sb->tcon;
 	if(pTcon == NULL)

A message that is not in the archive (sent off-list or not yet received): OF8579A98D.13CFCC0A-ON87257297.006728AA-86257297.006794F0@us.ibm.com

Christoph Hellwig

author

2007-03-07 22:18 UTC #2 lore raw

Quoted 6 lines · Steven French

Quoted 1 line
file->f_path.dentry or file->f_path.dentry.d_inode can't be NULL
OK - I don't really mind removing these checks - and I agree that I there is not an obvious way that they can be null, yet we had a case in which file->f_dentry was reported as NULL a few years back in a bug report that we could not reproduce.

There's a good reason you couldn't reproduce it, because it most likely must have been a really bad hack in the submitters kernel. Setting up file->f_dentry is one of the first thing we do after allocating the file struct.

Quoted 4 lines

The change to f_path.dentry did hit all filesystems (not sure who did it last year) and that patch did hit lkml - so this is not something new that just slipped in.

I know - that patch just made the enormous amount of useless checks vissible.

Quoted 2 lines

Is there an easy way to mirror particular patches going into the cifs-2.6.git tree (which is pulled into mm) to lkml?

Maybe some git expert can comment on that.

Quoted 2 lines

The cifs patches go in mm for at least a week before they go into kernel but some of them I would like to post again to lkml.

polling -mm is a little hard as it's an enormous blob, so posting to lkml or -fsdevel would definitively be quite helpfull.

Randy Dunlap

2007-03-07 22:33 UTC #3 lore raw

Quoted 14 lines · On Wed, 7 Mar 2007 23:17:46 +0100 Christoph Hellwig wrote:

Quoted 6 lines · Steven French
Quoted 1 line
file->f_path.dentry or file->f_path.dentry.d_inode can't be NULL
OK - I don't really mind removing these checks - and I agree that I there is not an obvious way that they can be null, yet we had a case in which file->f_dentry was reported as NULL a few years back in a bug report that we could not reproduce.
There's a good reason you couldn't reproduce it, because it most likely must have been a really bad hack in the submitters kernel. Setting up file->f_dentry is one of the first thing we do after allocating the file struct.
Quoted 1 line

Quoted 9 lines

Quoted 3 lines
The change to f_path.dentry did hit all filesystems (not sure who did it last year) and that patch did hit lkml - so this is not something new that just slipped in.
I know - that patch just made the enormous amount of useless checks vissible.
Quoted 2 lines
Is there an easy way to mirror particular patches going into the cifs-2.6.git tree (which is pulled into mm) to lkml?

BTW, this isn't just a cifs issue. There are way too many git trees for Random J Developer <[email protected]> to track.

Quoted 7 lines

Maybe some git expert can comment on that.
Quoted 2 lines
The cifs patches go in mm for at least a week before they go into kernel but some of them I would like to post again to lkml.
polling -mm is a little hard as it's an enormous blob, so posting to lkml or -fsdevel would definitively be quite helpfull.

I prefer -fsdevel or any more focused list (let's not make lkml a dumping ground for everything).

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***