[PATCH] qnx4fs: small cleanup

[PATCH] qnx4fs: small cleanup

[Kai Bankett]

[-- Attachment #1: Type: text/plain, Size: 321 bytes --]

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 <chaosman@ontika.net>
---
  inode.c |   27 ---------------------------
  namei.c |    4 ----
  2 files changed, 31 deletions(-)



[-- Attachment #2: patch-3.2.5-qnx4-cleanup1 --]
[-- Type: text/plain, Size: 2109 bytes --]

diff -Nurp linux-3.2.5.orig/fs/qnx4/inode.c linux-3.2.5-qnx4-cleanup/fs/qnx4/inode.c
--- linux-3.2.5.orig/fs/qnx4/inode.c	2012-02-06 18:47:00.000000000 +0100
+++ linux-3.2.5-qnx4-cleanup/fs/qnx4/inode.c	2012-02-13 02:22:39.921199183 +0100
@@ -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
 };
 
diff -Nurp linux-3.2.5.orig/fs/qnx4/namei.c linux-3.2.5-qnx4-cleanup/fs/qnx4/namei.c
--- linux-3.2.5.orig/fs/qnx4/namei.c	2012-02-06 18:47:00.000000000 +0100
+++ linux-3.2.5-qnx4-cleanup/fs/qnx4/namei.c	2012-02-13 02:22:39.921199183 +0100
@@ -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 <chaosman@ontika.net>

Re: [PATCH] qnx4fs: small cleanup

[Al Viro]

On Mon, Feb 13, 2012 at 02:43:41AM +0100, Kai Bankett wrote:
> 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.

Re: [PATCH] qnx4fs: small cleanup

[Kai Bankett]

On 02/14/2012 05:00 AM, Al Viro wrote:
> 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

[PATCH] qnx4fs: small cleanup #2

[Kai Bankett]

[-- Attachment #1: Type: text/plain, Size: 577 bytes --]

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(-)

[-- Attachment #2: patch-3.2.5-qnx4-cleanup2 --]
[-- Type: text/plain, Size: 10005 bytes --]

diff -Nurp linux-3.2.5-qnx4-cleanup1/fs/qnx4/dir.c linux-3.2.5-qnx4-cleanup2/fs/qnx4/dir.c
--- linux-3.2.5-qnx4-cleanup1/fs/qnx4/dir.c	2012-02-06 18:47:00.000000000 +0100
+++ linux-3.2.5-qnx4-cleanup2/fs/qnx4/dir.c	2012-02-15 23:11:33.100947641 +0100
@@ -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;
 					}
diff -Nurp linux-3.2.5-qnx4-cleanup1/fs/qnx4/inode.c linux-3.2.5-qnx4-cleanup2/fs/qnx4/inode.c
--- linux-3.2.5-qnx4-cleanup1/fs/qnx4/inode.c	2012-02-15 22:52:36.166161281 +0100
+++ linux-3.2.5-qnx4-cleanup2/fs/qnx4/inode.c	2012-02-15 23:30:50.410479281 +0100
@@ -57,7 +57,7 @@ static struct buffer_head *qnx4_getblk(s
 {
 	struct buffer_head *result = NULL;
 
-	if ( nr >= 0 )
+	if (nr >= 0)
 		nr = qnx4_block_map( inode, nr );
 	if (nr) {
 		result = sb_getblk(inode->i_sb, nr);
@@ -84,31 +84,33 @@ struct buffer_head *qnx4_bread(struct in
 	return NULL;
 }
 
-static int qnx4_get_block( struct inode *inode, sector_t iblock, struct buffer_head *bh, int create )
+static int qnx4_get_block(struct inode *inode, sector_t iblock,
+			  struct buffer_head *bh, int create)
 {
 	unsigned long phys;
 
-	QNX4DEBUG((KERN_INFO "qnx4: qnx4_get_block inode=[%ld] iblock=[%ld]\n",inode->i_ino,iblock));
+	QNX4DEBUG((KERN_INFO "qnx4: qnx4_get_block inode=[%ld] iblock=[%ld]\n",
+			inode->i_ino, iblock));
 
-	phys = qnx4_block_map( inode, iblock );
-	if ( phys ) {
+	phys = qnx4_block_map(inode, iblock);
+	if (phys) {
 		// logical block is before EOF
 		map_bh(bh, inode->i_sb, phys);
 	}
 	return 0;
 }
 
-unsigned long qnx4_block_map( struct inode *inode, long iblock )
+unsigned long qnx4_block_map(struct inode *inode, unsigned long iblock)
 {
 	int ix;
-	long offset, i_xblk;
+	unsigned long offset, i_xblk;
 	unsigned long block = 0;
 	struct buffer_head *bh = NULL;
 	struct qnx4_xblk *xblk = NULL;
 	struct qnx4_inode_entry *qnx4_inode = qnx4_raw_inode(inode);
 	u16 nxtnt = le16_to_cpu(qnx4_inode->di_num_xtnts);
 
-	if ( iblock < le32_to_cpu(qnx4_inode->di_first_xtnt.xtnt_size) ) {
+	if (iblock < le32_to_cpu(qnx4_inode->di_first_xtnt.xtnt_size)) {
 		// iblock is in the first extent. This is easy.
 		block = le32_to_cpu(qnx4_inode->di_first_xtnt.xtnt_blk) + iblock - 1;
 	} else {
@@ -116,38 +118,39 @@ unsigned long qnx4_block_map( struct ino
 		i_xblk = le32_to_cpu(qnx4_inode->di_xblk);
 		offset = iblock - le32_to_cpu(qnx4_inode->di_first_xtnt.xtnt_size);
 		ix = 0;
-		while ( --nxtnt > 0 ) {
-			if ( ix == 0 ) {
+		while (--nxtnt > 0) {
+			if (!ix) {
 				// read next xtnt block.
 				bh = sb_bread(inode->i_sb, i_xblk - 1);
-				if ( !bh ) {
+				if (!bh) {
 					QNX4DEBUG((KERN_ERR "qnx4: I/O error reading xtnt block [%ld])\n", i_xblk - 1));
 					return -EIO;
 				}
 				xblk = (struct qnx4_xblk*)bh->b_data;
-				if ( memcmp( xblk->xblk_signature, "IamXblk", 7 ) ) {
+				if (memcmp(xblk->xblk_signature, "IamXblk", 7)) {
 					QNX4DEBUG((KERN_ERR "qnx4: block at %ld is not a valid xtnt\n", qnx4_inode->i_xblk));
 					return -EIO;
 				}
 			}
-			if ( offset < le32_to_cpu(xblk->xblk_xtnts[ix].xtnt_size) ) {
+			if (offset < le32_to_cpu(xblk->xblk_xtnts[ix].xtnt_size)) {
 				// got it!
 				block = le32_to_cpu(xblk->xblk_xtnts[ix].xtnt_blk) + offset - 1;
 				break;
 			}
 			offset -= le32_to_cpu(xblk->xblk_xtnts[ix].xtnt_size);
-			if ( ++ix >= xblk->xblk_num_xtnts ) {
+			if (++ix >= xblk->xblk_num_xtnts) {
 				i_xblk = le32_to_cpu(xblk->xblk_next_xblk);
 				ix = 0;
-				brelse( bh );
+				brelse(bh);
 				bh = NULL;
 			}
 		}
-		if ( bh )
-			brelse( bh );
+		if (bh)
+			brelse(bh);
 	}
 
-	QNX4DEBUG((KERN_INFO "qnx4: mapping block %ld of inode %ld = %ld\n",iblock,inode->i_ino,block));
+	QNX4DEBUG((KERN_INFO "qnx4: mapping block %ld of inode %ld = %ld\n",
+			iblock, inode->i_ino, block));
 	return block;
 }
 
@@ -189,12 +192,12 @@ static const char *qnx4_checkroot(struct
 		rl = le32_to_cpu(qnx4_sb(sb)->sb->RootDir.di_first_xtnt.xtnt_size);
 		for (j = 0; j < rl; j++) {
 			bh = sb_bread(sb, rd + j);	/* root dir, first block */
-			if (bh == NULL) {
+			if (!bh) {
 				return "unable to read root entry.";
 			}
 			for (i = 0; i < QNX4_INODES_PER_BLOCK; i++) {
 				rootdir = (struct qnx4_inode_entry *) (bh->b_data + i * QNX4_DIR_ENTRY_SIZE);
-				if (rootdir->di_fname != NULL) {
+				if (!rootdir->di_fname) {
 					QNX4DEBUG((KERN_INFO "rootdir entry found : [%s]\n", rootdir->di_fname));
 					if (!strcmp(rootdir->di_fname,
 						    QNX4_BMNAME)) {
@@ -234,7 +237,10 @@ static int qnx4_fill_super(struct super_
 		return -ENOMEM;
 	s->s_fs_info = qs;
 
-	sb_set_blocksize(s, QNX4_BLOCK_SIZE);
+	if (!sb_set_blocksize(s, QNX4_BLOCK_SIZE)) {
+		printk(KERN_ERR "qnx4: unable to set blocksize\n");
+		goto outnobh;
+	}
 
 	/* Check the superblock signature. Since the qnx4 code is
 	   dangerous, we should leave as quickly as possible
@@ -244,7 +250,7 @@ static int qnx4_fill_super(struct super_
 		printk(KERN_ERR "qnx4: unable to read the superblock\n");
 		goto outnobh;
 	}
-	if ( le32_to_cpup((__le32*) bh->b_data) != QNX4_SUPER_MAGIC ) {
+	if (le32_to_cpup((__le32 *) bh->b_data) != QNX4_SUPER_MAGIC) {
 		if (!silent)
 			printk(KERN_ERR "qnx4: wrong fsid in superblock.\n");
 		goto out;
@@ -253,12 +259,12 @@ static int qnx4_fill_super(struct super_
 	s->s_magic = QNX4_SUPER_MAGIC;
 	s->s_flags |= MS_RDONLY;	/* Yup, read-only yet */
 	qnx4_sb(s)->sb_buf = bh;
-	qnx4_sb(s)->sb = (struct qnx4_super_block *) bh->b_data;
+	qnx4_sb(s)->sb = (struct qnx4_super_block *)bh->b_data;
 
 
  	/* check before allocating dentries, inodes, .. */
 	errmsg = qnx4_checkroot(s);
-	if (errmsg != NULL) {
+	if (errmsg) {
  		if (!silent)
 			printk(KERN_ERR "qnx4: %s\n", errmsg);
 		goto out;
@@ -274,7 +280,7 @@ static int qnx4_fill_super(struct super_
 
 	ret = -ENOMEM;
  	s->s_root = d_alloc_root(root);
- 	if (s->s_root == NULL)
+	if (!s->s_root)
  		goto outi;
 
 	brelse(bh);
@@ -293,8 +299,9 @@ static int qnx4_fill_super(struct super_
 static void qnx4_put_super(struct super_block *sb)
 {
 	struct qnx4_sb_info *qs = qnx4_sb(sb);
-	kfree( qs->BitMap );
-	kfree( qs );
+
+	kfree(qs->BitMap);
+	kfree(qs);
 	sb->s_fs_info = NULL;
 	return;
 }
@@ -366,21 +373,14 @@ struct inode *qnx4_iget(struct super_blo
 	if (S_ISREG(inode->i_mode)) {
 		inode->i_fop = &generic_ro_fops;
 		inode->i_mapping->a_ops = &qnx4_aops;
-		qnx4_i(inode)->mmu_private = inode->i_size;
 	} else if (S_ISDIR(inode->i_mode)) {
 		inode->i_op = &qnx4_dir_inode_operations;
 		inode->i_fop = &qnx4_dir_operations;
 	} else if (S_ISLNK(inode->i_mode)) {
 		inode->i_op = &page_symlink_inode_operations;
 		inode->i_mapping->a_ops = &qnx4_aops;
-		qnx4_i(inode)->mmu_private = inode->i_size;
-	} else {
-		printk(KERN_ERR "qnx4: bad inode %lu on dev %s\n",
-			ino, sb->s_id);
-		iget_failed(inode);
-		brelse(bh);
-		return ERR_PTR(-EIO);
-	}
+	} else
+		init_special_inode(inode, inode->i_mode, 0);
 	brelse(bh);
 	unlock_new_inode(inode);
 	return inode;
diff -Nurp linux-3.2.5-qnx4-cleanup1/fs/qnx4/namei.c linux-3.2.5-qnx4-cleanup2/fs/qnx4/namei.c
--- linux-3.2.5-qnx4-cleanup1/fs/qnx4/namei.c	2012-02-15 22:52:36.166161281 +0100
+++ linux-3.2.5-qnx4-cleanup2/fs/qnx4/namei.c	2012-02-15 22:52:56.037912851 +0100
@@ -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) {
diff -Nurp linux-3.2.5-qnx4-cleanup1/fs/qnx4/qnx4.h linux-3.2.5-qnx4-cleanup2/fs/qnx4/qnx4.h
--- linux-3.2.5-qnx4-cleanup1/fs/qnx4/qnx4.h	2012-02-06 18:47:00.000000000 +0100
+++ linux-3.2.5-qnx4-cleanup2/fs/qnx4/qnx4.h	2012-02-15 22:52:59.458870082 +0100
@@ -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);
 

Re: [PATCH] qnx4fs: small cleanup #2

[Al Viro]

On Wed, Feb 15, 2012 at 11:37:13PM +0100, Kai Bankett wrote:
>  				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...

Re: [PATCH] qnx4fs: small cleanup #2

[Kai Bankett]

> On Wed, Feb 15, 2012 at 11:37:13PM +0100, Kai Bankett wrote:
>>  				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...
>

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