Skip to contentSkip to conversation

change allow_write_access/deny_write_access prototype

PATCH In review Christoph Hellwig 4 participants 5 messages Nov 17, 2002

Subsystems (6)
Memory management VFS MIPS exec and binfmt +2 more subsystems
lore
Share & tools
lore

Christoph Hellwig

author
#0 lore raw

PATCH 1/1 change allow_write_access/deny_write_access prototype

Make them take an inode instead of a file, this way we don't need to derefence struct dentry in fs.h and with a few more steps we can get rid of dcache.h in fs.h

arch/mips/kernel/irixelf.c +4 -2
@@ -775,8 +775,10 @@
 	return retval;
 
 out_free_dentry:
-	allow_write_access(interpreter);
-	fput(interpreter);
+	if (interpreter) {
+		allow_write_access(interpreter->f_dentry->d_inode);
+		fput(interpreter);
+	}
 out_free_interp:
 	if (elf_interpreter)
 		kfree(elf_interpreter);

===== arch/parisc/kernel/sys_parisc32.c 1.2 vs edited =====

arch/parisc/kernel/sys_parisc32.c +1 -1
@@ -275,7 +275,7 @@
 
 out_file:
 	if (bprm.file) {
-		allow_write_access(bprm.file);
+		allow_write_access(bprm.file->f_dentry->d_inode);
 		fput(bprm.file);
 	}

===== arch/ppc64/kernel/sys_ppc32.c 1.30 vs edited =====

arch/ppc64/kernel/sys_ppc32.c +1 -1
@@ -3564,7 +3564,7 @@
 
 out_file:
 	if (bprm.file) {
-		allow_write_access(bprm.file);
+		allow_write_access(bprm.file->f_dentry->d_inode);
 		fput(bprm.file);
 	}
 	return retval;

===== arch/s390x/kernel/linux32.c 1.24 vs edited =====

arch/s390x/kernel/linux32.c +5 -4
@@ -3059,12 +3059,12 @@
 	bprm.loader = 0;
 	bprm.exec = 0;
 	if ((bprm.argc = count32(argv)) < 0) {
-		allow_write_access(file);
+		allow_write_access(file->f_dentry->d_inode);
 		fput(file);
 		return bprm.argc;
 	}
 	if ((bprm.envc = count32(envp)) < 0) {
-		allow_write_access(file);
+		allow_write_access(file->f_dentry->d_inode);
 		fput(file);
 		return bprm.envc;
 	}

@@ -3093,9 +3093,10 @@
 
 out:
 	/* Something went wrong, return the inode and free the argument pages*/
-	allow_write_access(bprm.file);
-	if (bprm.file)
+	if (bprm.file) {
+		allow_write_access(bprm.file->f_dentry->d_inode);
 		fput(bprm.file);
+	}
 
 	for (i=0 ; i<MAX_ARG_PAGES ; i++)
 		if (bprm.page[i])

===== arch/sparc64/kernel/sys_sparc32.c 1.45 vs edited =====

arch/sparc64/kernel/sys_sparc32.c +1 -1
@@ -3070,7 +3070,7 @@
 
 out_file:
 	if (bprm.file) {
-		allow_write_access(bprm.file);
+		allow_write_access(bprm.file->f_dentry->d_inode);
 		fput(bprm.file);
 	}
 	return retval;

===== fs/binfmt_elf.c 1.33 vs edited =====

fs/binfmt_elf.c +9 -5
@@ -717,9 +717,11 @@
 						    interpreter,
 						    &interp_load_addr);
 
-		allow_write_access(interpreter);
-		fput(interpreter);
-		kfree(elf_interpreter);
+		if (interpreter) {
+			allow_write_access(interpreter->f_dentry->d_inode);
+			fput(interpreter);
+			kfree(elf_interpreter);
+		}
 
 		if (BAD_ADDR(elf_entry)) {
 			printk(KERN_ERR "Unable to load interpreter\n");

@@ -793,8 +795,10 @@
 
 	/* error cleanup */
 out_free_dentry:
-	allow_write_access(interpreter);
-	fput(interpreter);
+	if (interpreter) {
+		allow_write_access(interpreter->f_dentry->d_inode);
+		fput(interpreter);
+	}
 out_free_interp:
 	if (elf_interpreter)
 		kfree(elf_interpreter);

===== fs/binfmt_em86.c 1.7 vs edited =====

fs/binfmt_em86.c +5 -3
@@ -45,9 +45,11 @@
 	}
 
 	bprm->sh_bang++;	/* Well, the bang-shell is implicit... */
-	allow_write_access(bprm->file);
-	fput(bprm->file);
-	bprm->file = NULL;
+	if (bprm->file) {
+		allow_write_access(bprm->file->f_dentry->d_inode);
+		fput(bprm->file);
+		bprm->file = NULL;
+	}
 
 	/* Unlike in the script case, we don't have to do any hairy
 	 * parsing to find our interpreter... it's hardcoded!

===== fs/binfmt_misc.c 1.17 vs edited =====

fs/binfmt_misc.c +5 -3
@@ -121,9 +121,11 @@
 	if (!fmt)
 		goto _ret;
 
-	allow_write_access(bprm->file);
-	fput(bprm->file);
-	bprm->file = NULL;
+	if (bprm->file) {
+		allow_write_access(bprm->file->f_dentry->d_inode);
+		fput(bprm->file);
+		bprm->file = NULL;
+	}
 
 	/* Build args for interpreter */
 	if (!(fmt->flags & MISC_FMT_PRESERVE_ARGV0)) {

===== fs/binfmt_script.c 1.6 vs edited =====

fs/binfmt_script.c +5 -3
@@ -31,9 +31,11 @@
 	 */
 
 	bprm->sh_bang++;
-	allow_write_access(bprm->file);
-	fput(bprm->file);
-	bprm->file = NULL;
+	if (bprm->file) {
+		allow_write_access(bprm->file->f_dentry->d_inode);
+		fput(bprm->file);
+		bprm->file = NULL;
+	}
 
 	bprm->buf[BINPRM_BUF_SIZE - 1] = '\0';
 	if ((cp = strchr(bprm->buf, '\n')) == NULL)
fs/exec.c +11 -8
@@ -462,7 +462,7 @@
 			if (!err) {
 				file = dentry_open(nd.dentry, nd.mnt, O_RDONLY);
 				if (!IS_ERR(file)) {
-					err = deny_write_access(file);
+					err = deny_write_access(inode);
 					if (err) {
 						fput(file);
 						file = ERR_PTR(err);

@@ -934,9 +934,11 @@
 		struct file * file;
 		unsigned long loader;
 
-		allow_write_access(bprm->file);
-		fput(bprm->file);
-		bprm->file = NULL;
+		if (bprm->file) {
+			allow_write_access(bprm->file->f_dentry->d_inode);
+			fput(bprm->file);
+			bprm->file = NULL;
+		}
 
 	        loader = PAGE_SIZE*MAX_ARG_PAGES-sizeof(void *);
 

@@ -977,10 +979,11 @@
 			retval = fn(bprm, regs);
 			if (retval >= 0) {
 				put_binfmt(fmt);
-				allow_write_access(bprm->file);
-				if (bprm->file)
+				if (bprm->file) {
+					allow_write_access(bprm->file->f_dentry->d_inode);
 					fput(bprm->file);
-				bprm->file = NULL;
+					bprm->file = NULL;
+				}
 				current->did_exec = 1;
 				return retval;
 			}

@@ -1101,7 +1104,7 @@
 
 out_file:
 	if (bprm.file) {
-		allow_write_access(bprm.file);
+		allow_write_access(bprm.file->f_dentry->d_inode);
 		fput(bprm.file);
 	}
 	return retval;
fs/namei.c +4 -3
@@ -251,14 +251,15 @@
 	spin_unlock(&arbitration_lock);
 	return 0;
 }
-int deny_write_access(struct file * file)
+
+int deny_write_access(struct inode * inode)
 {
 	spin_lock(&arbitration_lock);
-	if (atomic_read(&file->f_dentry->d_inode->i_writecount) > 0) {
+	if (atomic_read(&inode->i_writecount) > 0) {
 		spin_unlock(&arbitration_lock);
 		return -ETXTBSY;
 	}
-	atomic_dec(&file->f_dentry->d_inode->i_writecount);
+	atomic_dec(&inode->i_writecount);
 	spin_unlock(&arbitration_lock);
 	return 0;
 }
include/linux/fs.h +3 -4
@@ -1173,15 +1155,14 @@
 extern int permission(struct inode *, int);
 extern int vfs_permission(struct inode *, int);
 extern int get_write_access(struct inode *);
-extern int deny_write_access(struct file *);
+extern int deny_write_access(struct inode *);
 static inline void put_write_access(struct inode * inode)
 {
 	atomic_dec(&inode->i_writecount);
 }
-static inline void allow_write_access(struct file *file)
+static inline void allow_write_access(struct inode *inode)
 {
-	if (file)
-		atomic_inc(&file->f_dentry->d_inode->i_writecount);
+	atomic_inc(&inode->i_writecount);
 }
 extern int do_pipe(int *);
mm/mmap.c +1 -1
@@ -567,7 +567,7 @@
 		if (vm_flags & (VM_GROWSDOWN|VM_GROWSUP))
 			goto free_vma;
 		if (vm_flags & VM_DENYWRITE) {
-			error = deny_write_access(file);
+			error = deny_write_access(file->f_dentry->d_inode);
 			if (error)
 				goto free_vma;
 			correct_wcount = 1;
#1 lore raw
Quoted 4 lines · Christoph Hellwig

Make them take an inode instead of a file, this way we don't need to derefence struct dentry in fs.h and with a few more steps we can get rid of dcache.h in fs.h

This is the point where I say "NO!"

If we're making code uglier because we want to include fewer files, we're doing something WRONG!

It's a lot more important to have readable code than it is to try to avoid a file include. And this patch makes code less readable, just look at it.

Also, I'm going to stop applying the header file cleanups altogether unless the people who send them to me check that things compile afterwards a lot better.

Arnaldo has been helping a lot in fixing stuff up after the cleanups, thanks go to him. The last net.h/fs.h cleanup broke pcm_native.c, for example.

Cleanups are only good if they make things BETTER. Making things worse is not a good thing.

Linus

#2 lore raw
Quoted 7 lines · Linus Torvalds

Also, I'm going to stop applying the header file cleanups altogether unless the people who send them to me check that things compile afterwards a lot better.

Arnaldo has been helping a lot in fixing stuff up after the cleanups, thanks go to him. The last net.h/fs.h cleanup broke pcm_native.c, for example.

I'm quite happy to test that my header cleanups don't break things -- but there are many things which don't currently compile anyway. I try to enable the things I think my changes will break, but I didn't spot the isapnp one my pci_dev changes broke. Could I ask that you update defconfig with your .config again so we can be sure things don't break _your_ build?

Signature · 1 line
Revolutions do not require corporate support.
#3 lore raw
Quoted 7 lines · Matthew Wilcox

I'm quite happy to test that my header cleanups don't break things -- but there are many things which don't currently compile anyway. I try to enable the things I think my changes will break, but I didn't spot the isapnp one my pci_dev changes broke. Could I ask that you update defconfig with your .config again so we can be sure things don't break _your_ build?

Breaking my build is ok, I can trivially fix those. Usually it's a one-liner, and that's not the thing I worry about.

I worry about the fact that I'm aboud to releaser 2.5.48, and then it doesn't compile for some silly reason, even though I've obviously tested it on my configurations and fixed the problems _I_ saw.

Cleaning up header files is fine, but the trivial part is to remove the line that includes another header file. The _real_ work is trying to figure out that nothing else broke as a result, and I think that's what a janitor who does these things should really care about.

The whole "janitor" name to me implies somebody who does the hand and ugly and mostly boring thing that needs to be done to clean stuff up and make sure things work smoothly. A "janitorial" patch that leaves broken pieces in random places of the tree doesn't sound very janitorial to me, if you see what I mean..

Linus

#4 lore raw
In reply to Christoph Hellwig
Quoted 4 lines · Christoph Hellwig

Make them take an inode instead of a file, this way we don't need to derefence struct dentry in fs.h and with a few more steps we can get rid of dcache.h in fs.h

Stop. Quite a few things of the same kind will eventually use both
vfsmount and dentry, so generally such replacements are *not* a good thing.