Skip to contentSkip to conversation

qnx4fs: small cleanup

PATCH Applied Kai Bankett 2 participants 6 messages Feb 13, 2012 – Feb 16, 2012

Subsystems (2)
VFS QNX4 filesystem
lore
Share & tools
lore

Kai Bankett

author
#0 lore raw

PATCH 1/1 qnx4fs: small cleanup

Small qnx4 cleanup patch.
- removes .writepage, .write_begin and .write_end (+callback functions)
- removes '.' path checking in namei.c (handled on upper layers)

Signed-off-by Kai Bankett <[email protected]>
---
 inode.c |   27 ---------------------------
 namei.c |    4 ----
 2 files changed, 31 deletions(-)
fs/qnx4/inode.c +0 -27
diff -Nurp linux-3.2.5.orig/fs/qnx4/inode.c linux-3.2.5-qnx4-cleanup/fs/qnx4/inode.c
@@ -299,44 +299,17 @@ static void qnx4_put_super(struct super_
 	return;
 }
 
-static int qnx4_writepage(struct page *page, struct writeback_control *wbc)
-{
-	return block_write_full_page(page,qnx4_get_block, wbc);
-}
-
 static int qnx4_readpage(struct file *file, struct page *page)
 {
 	return block_read_full_page(page,qnx4_get_block);
 }
 
-static int qnx4_write_begin(struct file *file, struct address_space *mapping,
-			loff_t pos, unsigned len, unsigned flags,
-			struct page **pagep, void **fsdata)
-{
-	struct qnx4_inode_info *qnx4_inode = qnx4_i(mapping->host);
-	int ret;
-
-	*pagep = NULL;
-	ret = cont_write_begin(file, mapping, pos, len, flags, pagep, fsdata,
-				qnx4_get_block,
-				&qnx4_inode->mmu_private);
-	if (unlikely(ret)) {
-		loff_t isize = mapping->host->i_size;
-		if (pos + len > isize)
-			vmtruncate(mapping->host, isize);
-	}
-
-	return ret;
-}
 static sector_t qnx4_bmap(struct address_space *mapping, sector_t block)
 {
 	return generic_block_bmap(mapping,block,qnx4_get_block);
 }
 static const struct address_space_operations qnx4_aops = {
 	.readpage	= qnx4_readpage,
-	.writepage	= qnx4_writepage,
-	.write_begin	= qnx4_write_begin,
-	.write_end	= generic_write_end,
 	.bmap		= qnx4_bmap
 };
fs/qnx4/namei.c +0 -4
diff -Nurp linux-3.2.5.orig/fs/qnx4/namei.c linux-3.2.5-qnx4-cleanup/fs/qnx4/namei.c
@@ -39,10 +39,6 @@ static int qnx4_match(int len, const cha
 	} else {
 		namelen = QNX4_SHORT_NAME_MAX;
 	}
-	/* "" means "." ---> so paths like "/usr/lib//libc.a" work */
-	if (!len && (de->di_fname[0] == '.') && (de->di_fname[1] == '\0')) {
-		return 1;
-	}
 	thislen = strlen( de->di_fname );
 	if ( thislen > namelen )
 		thislen = namelen;
Signed-off-by Kai Bankett <[email protected]>
maintainer Applied
#1 lore raw
Quoted 3 lines · Kai Bankett

Small qnx4 cleanup patch.
- removes .writepage, .write_begin and .write_end (+callback functions)
- removes '.' path checking in namei.c (handled on upper layers)

Applied. FWIW, after looking at fs/qnx4... *OUCH*

* qnx4_bread()/qnx4_getblk() is completely pointless - the sole caller is going to call qnx4_block_map() *anyway*, so we might as well just feed that value to sb_bread().

* extent blocks handling is fishy - e.g. if you have an extent block with 0 extents in it, the damn thing will treat that as if it contained 1 extent. If it's really invalid (and it probably is), that might be a sane mitigation strategy, but otherwise it's a bug; in any case, that shouldn't be silent. It also leaks references to buffer_head if it runs into extent block with invalid signature. It definitely could've used a bit of caching - storing the last matched extent in the in-core inode and skipping the entire "walk the whole extent chain" thing would be a clear win. It also has a problem with error reporting - it returns (unsigned long)-EIO and callers have no idea what to do with that (they do understand 0 as "no such block", but that's it; you'll _probably_ end up with sb_bread() unhappy with huge block number, so it'll end up with screaming block layer)

* readdir and lookup handling relies on running into NULs in directories. Neither bothers to check for combination of "used" and "link" bits being invalid. BTW, neither handles long names (present in qnx4 filesystems on recent enough versions).

Overall, the code could use quite a bit of cleanup. I don't know if they have evaluation images that could run under qemu and if the license allows experimenting with fs images anyway; if anyone knows (or can run the tests on their box), please yell - I'd rather not play with cleanup patches without testing, but the codebase isn't large or complex enough to make writing the patches to test particulary hard.

author
#2 lore raw
Quoted 6 lines · Al Viro

Overall, the code could use quite a bit of cleanup. I don't know if they have evaluation images that could run under qemu and if the license allows experimenting with fs images anyway; if anyone knows (or can run the tests on their box), please yell - I'd rather not play with cleanup patches without testing, but the codebase isn't large or complex enough to make writing the patches to test particulary hard.

Yes, fully agree. Not sure if the source is still actively maintained?

However, I could offer to take a look at it as soon as I made sufficient progress with the qnx6fs stuff. I could then test it by mounting a qnx4fs and properly test it + send some patches.
At least I am "somehow" in the qnx4fs source already ...

Kai

author
#3 lore raw

PATCH 1/1 qnx4fs: small cleanup #2

In reply to Al Viro

Here is the 2nd cleanup patch.

* set_blocksize return checked
* uses init_special_node() in iget()
* dir.c use strnlen() instead of strlen() - should at least improve it a bit
* removed mmu_private - was not used anymore after cleanup #1
* small signedness correction qnx4_block_map()
* code style corrections

Tested on a qnx4 filesystem. Still working as before.

 dir.c   |   20 +++++++++--------
 inode.c |   72 
++++++++++++++++++++++++++++++++--------------------------------
 namei.c |    2 -
 qnx4.h  |    3 --
 4 files changed, 49 insertions(+), 48 deletions(-)
fs/qnx4/dir.c +11 -9
diff -Nurp linux-3.2.5-qnx4-cleanup1/fs/qnx4/dir.c linux-3.2.5-qnx4-cleanup2/fs/qnx4/dir.c
@@ -29,30 +29,32 @@ static int qnx4_readdir(struct file *fil
 	QNX4DEBUG((KERN_INFO "filp->f_pos         = %ld\n", (long) filp->f_pos));
 
 	while (filp->f_pos < inode->i_size) {
-		blknum = qnx4_block_map( inode, filp->f_pos >> QNX4_BLOCK_SIZE_BITS );
+		blknum = qnx4_block_map(inode,
+					filp->f_pos >> QNX4_BLOCK_SIZE_BITS);
 		bh = sb_bread(inode->i_sb, blknum);
-		if(bh==NULL) {
-			printk(KERN_ERR "qnx4_readdir: bread failed (%ld)\n", blknum);
+		if (!bh) {
+			printk(KERN_ERR "qnx4_readdir: bread failed (%ld)\n",
+					blknum);
 			break;
 		}
 		ix = (int)(filp->f_pos >> QNX4_DIR_ENTRY_SIZE_BITS) % QNX4_INODES_PER_BLOCK;
 		while (ix < QNX4_INODES_PER_BLOCK) {
 			offset = ix * QNX4_DIR_ENTRY_SIZE;
-			de = (struct qnx4_inode_entry *) (bh->b_data + offset);
+			de = (struct qnx4_inode_entry *)(bh->b_data + offset);
 			size = strlen(de->di_fname);
 			if (size) {
-				if ( !( de->di_status & QNX4_FILE_LINK ) && size > QNX4_SHORT_NAME_MAX )
+				if (!(de->di_status & QNX4_FILE_LINK) && size > QNX4_SHORT_NAME_MAX)
 					size = QNX4_SHORT_NAME_MAX;
-				else if ( size > QNX4_NAME_MAX )
+				else if (size > QNX4_NAME_MAX)
 					size = QNX4_NAME_MAX;
 
-				if ( ( de->di_status & (QNX4_FILE_USED|QNX4_FILE_LINK) ) != 0 ) {
+				if ((de->di_status & (QNX4_FILE_USED|QNX4_FILE_LINK)) != 0) {
 					QNX4DEBUG((KERN_INFO "qnx4_readdir:%.*s\n", size, de->di_fname));
-					if ( ( de->di_status & QNX4_FILE_LINK ) == 0 )
+					if ((de->di_status & QNX4_FILE_LINK) == 0)
 						ino = blknum * QNX4_INODES_PER_BLOCK + ix - 1;
 					else {
 						le  = (struct qnx4_link_info*)de;
-						ino = ( le32_to_cpu(le->dl_inode_blk) - 1 ) *
+						ino = (le32_to_cpu(le->dl_inode_blk) - 1) *
 							QNX4_INODES_PER_BLOCK +
 							le->dl_inode_ndx;
 					}
fs/qnx4/inode.c +36 -36

186 lines not on this page · raw message

fs/qnx4/namei.c +1 -1
diff -Nurp linux-3.2.5-qnx4-cleanup1/fs/qnx4/namei.c linux-3.2.5-qnx4-cleanup2/fs/qnx4/namei.c
@@ -39,7 +39,7 @@ static int qnx4_match(int len, const cha
 	} else {
 		namelen = QNX4_SHORT_NAME_MAX;
 	}
-	thislen = strlen( de->di_fname );
+	thislen = strnlen(de->di_fname, namelen);
 	if ( thislen > namelen )
 		thislen = namelen;
 	if (len != thislen) {
fs/qnx4/qnx4.h +1 -2
diff -Nurp linux-3.2.5-qnx4-cleanup1/fs/qnx4/qnx4.h linux-3.2.5-qnx4-cleanup2/fs/qnx4/qnx4.h
@@ -18,14 +18,13 @@ struct qnx4_sb_info {
 
 struct qnx4_inode_info {
 	struct qnx4_inode_entry raw;
-	loff_t mmu_private;
 	struct inode vfs_inode;
 };
 
 extern struct inode *qnx4_iget(struct super_block *, unsigned long);
 extern struct dentry *qnx4_lookup(struct inode *dir, struct dentry *dentry, struct nameidata *nd);
 extern unsigned long qnx4_count_free_blocks(struct super_block *sb);
-extern unsigned long qnx4_block_map(struct inode *inode, long iblock);
+extern unsigned long qnx4_block_map(struct inode *inode, unsigned long iblock);
 
 extern struct buffer_head *qnx4_bread(struct inode *, int, int);
maintainer
#4 lore raw

Re: [PATCH] qnx4fs: small cleanup #2

fs/qnx4/inode.c static const char *qnx4_checkroot(struct · line 200

Quoted 3 lines · Kai Bankett
fs/qnx4/inode.c +1 -1
 				rootdir = (struct qnx4_inode_entry *) (bh->b_data + i * QNX4_DIR_ENTRY_SIZE);
-				if (rootdir->di_fname != NULL) {
+				if (!rootdir->di_fname) {

Huh???

	a) this actually inverts the test
	b) ->di_fname is actually an array; the test made no

sense even before that change; it's _never_ NULL. See commit 1aab323ea5cd67d2d2572a1f2794978583ff8545 in -rc1...

author
#5 lore raw

Re: [PATCH] qnx4fs: small cleanup #2

Quoted 7 lines · Kai Bankett

fs/qnx4/inode.c static const char *qnx4_checkroot(struct · line 200

Quoted 4 lines

rootdir = (struct qnx4_inode_entry *) (bh->b_data + i *
QNX4_DIR_ENTRY_SIZE);

fs/qnx4/inode.c +1 -1
-				if (rootdir->di_fname != NULL) {
+				if (!rootdir->di_fname) {

Huh???

Quoted 5 lines
	a) this actually inverts the test
	b) ->di_fname is actually an array; the test made no

sense even before that change; it's _never_ NULL. See commit 1aab323ea5cd67d2d2572a1f2794978583ff8545 in -rc1...

Ack.
Never, ever again do even small, quick style fixes after testing. (that's what I've learned from it ...)