[PATCH] ext3: Fix sparse -Wbitwise warnings.

[PATCH] ext3: Fix sparse -Wbitwise warnings.

[Alexey Dobriyan]

Signed-off-by: Alexey Dobriyan <adobriyan@mail.ru>
---

 fs/ext3/resize.c        |   20 ++++++++++----------
 fs/ext3/super.c         |    8 ++++----
 include/linux/ext3_fs.h |    2 +-
 3 files changed, 15 insertions(+), 15 deletions(-)

Index: linux-warnings/include/linux/ext3_fs.h
===================================================================
--- linux-warnings/include/linux/ext3_fs.h	(revision 14)
+++ linux-warnings/include/linux/ext3_fs.h	(revision 20)
@@ -450,7 +450,7 @@
 	 */
 	__u8	s_prealloc_blocks;	/* Nr of blocks to try to preallocate*/
 	__u8	s_prealloc_dir_blocks;	/* Nr to preallocate for dirs */
-	__u16	s_reserved_gdt_blocks;	/* Per group desc for online growth */
+	__le16	s_reserved_gdt_blocks;	/* Per group desc for online growth */
 	/*
 	 * Journaling support valid if EXT3_FEATURE_COMPAT_HAS_JOURNAL set.
 	 */
Index: linux-warnings/fs/ext3/super.c
===================================================================
--- linux-warnings/fs/ext3/super.c	(revision 14)
+++ linux-warnings/fs/ext3/super.c	(revision 20)
@@ -2105,13 +2105,13 @@
 
 			ext3_mark_recovery_complete(sb, es);
 		} else {
-			__le32 ret;
-			if ((ret = EXT3_HAS_RO_COMPAT_FEATURE(sb,
-					~EXT3_FEATURE_RO_COMPAT_SUPP))) {
+			int ret;
+			if ((ret = le32_to_cpu(EXT3_HAS_RO_COMPAT_FEATURE(sb,
+					~EXT3_FEATURE_RO_COMPAT_SUPP)))) {
 				printk(KERN_WARNING "EXT3-fs: %s: couldn't "
 				       "remount RDWR because of unsupported "
 				       "optional features (%x).\n",
-				       sb->s_id, le32_to_cpu(ret));
+				       sb->s_id, ret);
 				return -EROFS;
 			}
 			/*
Index: linux-warnings/fs/ext3/resize.c
===================================================================
--- linux-warnings/fs/ext3/resize.c	(revision 14)
+++ linux-warnings/fs/ext3/resize.c	(revision 20)
@@ -328,7 +328,7 @@
 	unsigned five = 5;
 	unsigned seven = 7;
 	unsigned grp;
-	__u32 *p = (__u32 *)primary->b_data;
+	__le32 *p = (__le32 *)primary->b_data;
 	int gdbackups = 0;
 
 	while ((grp = ext3_list_backups(sb, &three, &five, &seven)) < end) {
@@ -371,7 +371,7 @@
 	struct buffer_head *dind;
 	int gdbackups;
 	struct ext3_iloc iloc;
-	__u32 *data;
+	__le32 *data;
 	int err;
 
 	if (test_opt(sb, DEBUG))
@@ -408,7 +408,7 @@
 		goto exit_bh;
 	}
 
-	data = (__u32 *)dind->b_data;
+	data = (__le32 *)dind->b_data;
 	if (le32_to_cpu(data[gdb_num % EXT3_ADDR_PER_BLOCK(sb)]) != gdblock) {
 		ext3_warning(sb, __FUNCTION__,
 			     "new group %u GDT block %lu not reserved\n",
@@ -510,7 +510,7 @@
 	struct buffer_head *dind;
 	struct ext3_iloc iloc;
 	unsigned long blk;
-	__u32 *data, *end;
+	__le32 *data, *end;
 	int gdbackups = 0;
 	int res, i;
 	int err;
@@ -527,15 +527,15 @@
 	}
 
 	blk = EXT3_SB(sb)->s_sbh->b_blocknr + 1 + EXT3_SB(sb)->s_gdb_count;
-	data = (__u32 *)dind->b_data + EXT3_SB(sb)->s_gdb_count;
-	end = (__u32 *)dind->b_data + EXT3_ADDR_PER_BLOCK(sb);
+	data = (__le32 *)dind->b_data + EXT3_SB(sb)->s_gdb_count;
+	end = (__le32 *)dind->b_data + EXT3_ADDR_PER_BLOCK(sb);
 
 	/* Get each reserved primary GDT block and verify it holds backups */
 	for (res = 0; res < reserved_gdb; res++, blk++) {
 		if (le32_to_cpu(*data) != blk) {
 			ext3_warning(sb, __FUNCTION__,
 				     "reserved block %lu not at offset %ld\n",
-				     blk, (long)(data - (__u32 *)dind->b_data));
+				     blk, (long)(data - (__le32 *)dind->b_data));
 			err = -EINVAL;
 			goto exit_bh;
 		}
@@ -550,7 +550,7 @@
 			goto exit_bh;
 		}
 		if (++data >= end)
-			data = (__u32 *)dind->b_data;
+			data = (__le32 *)dind->b_data;
 	}
 
 	for (i = 0; i < reserved_gdb; i++) {
@@ -574,7 +574,7 @@
 	blk = input->group * EXT3_BLOCKS_PER_GROUP(sb);
 	for (i = 0; i < reserved_gdb; i++) {
 		int err2;
-		data = (__u32 *)primary[i]->b_data;
+		data = (__le32 *)primary[i]->b_data;
 		/* printk("reserving backup %lu[%u] = %lu\n",
 		       primary[i]->b_blocknr, gdbackups,
 		       blk + primary[i]->b_blocknr); */
@@ -675,7 +675,7 @@
 			     "can't update backup for group %d (err %d), "
 			     "forcing fsck on next reboot\n", group, err);
 		sbi->s_mount_state &= ~EXT3_VALID_FS;
-		sbi->s_es->s_state &= ~cpu_to_le16(EXT3_VALID_FS);
+		sbi->s_es->s_state &= cpu_to_le16(~EXT3_VALID_FS);
 		mark_buffer_dirty(sbi->s_sbh);
 	}
 }

Re: [PATCH] ext3: Fix sparse -Wbitwise warnings.

[Stephen C. Tweedie]

Hi,

On Tue, 2005-02-15 at 10:46, Alexey Dobriyan wrote:

> -			if ((ret = EXT3_HAS_RO_COMPAT_FEATURE(sb,
> -					~EXT3_FEATURE_RO_COMPAT_SUPP))) {
> +			if ((ret = le32_to_cpu(EXT3_HAS_RO_COMPAT_FEATURE(sb,
> +					~EXT3_FEATURE_RO_COMPAT_SUPP)))) {

NAK.

EXT3_HAS_RO_COMPAT_FEATURE returns a boolean value.  It happens to be
implemented internally as 

#define EXT3_HAS_COMPAT_FEATURE(sb,mask)			\
	( EXT3_SB(sb)->s_es->s_feature_compat & cpu_to_le32(mask) )


so the compiler, looking at the preprocessed code, will reasonably
assume it's a genuine little-endian value.  But it's only used as a
boolean, so we shouldn't be requiring the callers to provide an
le32_to_cpu() conversion.

If we want to fix this, let's fix the macros: for example, convert
EXT3_HAS_COMPAT_FEATURE to be

	( le32_to_cpu(EXT3_SB(sb)->s_es->s_feature_compat) & (mask) )

so that we're doing the tests in native CPU endian-ness.

--Stephen

Re: [PATCH] ext3: Fix sparse -Wbitwise warnings.

[Alexey Dobriyan]

On Tuesday 15 February 2005 16:12, Stephen C. Tweedie wrote:
> On Tue, 2005-02-15 at 10:46, Alexey Dobriyan wrote:
> 
> > -			if ((ret = EXT3_HAS_RO_COMPAT_FEATURE(sb,
> > -					~EXT3_FEATURE_RO_COMPAT_SUPP))) {
> > +			if ((ret = le32_to_cpu(EXT3_HAS_RO_COMPAT_FEATURE(sb,
> > +					~EXT3_FEATURE_RO_COMPAT_SUPP)))) {
> 
> NAK.

Argh... stupid me. super.c part should be just:

--- linux-2.6.11-rc4/fs/ext3/super.c.orig	2005-02-15 20:01:52.000000000 +0200
+++ linux-2.6.11-rc4/fs/ext3/super.c	2005-02-15 20:02:47.000000000 +0200
@@ -2106,6 +2106,7 @@ static int ext3_remount (struct super_bl
 			ext3_mark_recovery_complete(sb, es);
 		} else {
 			__le32 ret;
+			int ret1;
 			if ((ret = EXT3_HAS_RO_COMPAT_FEATURE(sb,
 					~EXT3_FEATURE_RO_COMPAT_SUPP))) {
 				printk(KERN_WARNING "EXT3-fs: %s: couldn't "
@@ -2122,8 +2123,8 @@ static int ext3_remount (struct super_bl
 			 */
 			ext3_clear_journal_err(sb, es);
 			sbi->s_mount_state = le16_to_cpu(es->s_state);
-			if ((ret = ext3_group_extend(sb, es, n_blocks_count)))
-				return ret;
+			if ((ret1 = ext3_group_extend(sb, es, n_blocks_count)))
+				return ret1;
 			if (!ext3_setup_super (sb, es, 0))
 				sb->s_flags &= ~MS_RDONLY;
 		}

Re: [PATCH] ext3: Fix sparse -Wbitwise warnings.

[Stephen C. Tweedie]

Hi,

On Tue, 2005-02-15 at 23:29, Mitchell Blank Jr wrote:

> Of course that's less efficient though since "mask" is probably constant..
> so now the endian conversion changed from compile-time to run-time.
> 
> Would something like
> 
>  	( ( EXT3_SB(sb)->s_es->s_feature_compat & cpu_to_le32(mask) ) != 0)
> 
> be enough to satisfy sparse?

Yes, but it breaks other places: there are a few places where callers
actually want the real return value from the "&", so that (for example)
they can tell the user which feature failed the compatibility test.

--Stephen

Re: [PATCH] ext3: Fix sparse -Wbitwise warnings.

[Mitchell Blank Jr]

Stephen C. Tweedie wrote:
> If we want to fix this, let's fix the macros: for example, convert
> EXT3_HAS_COMPAT_FEATURE to be
> 
> 	( le32_to_cpu(EXT3_SB(sb)->s_es->s_feature_compat) & (mask) )

Of course that's less efficient though since "mask" is probably constant..
so now the endian conversion changed from compile-time to run-time.

Would something like

 	( ( EXT3_SB(sb)->s_es->s_feature_compat & cpu_to_le32(mask) ) != 0)

be enough to satisfy sparse?

-Mitch

Re: [PATCH] ext3: Fix sparse -Wbitwise warnings.

[Andreas Dilger]

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

On Feb 15, 2005  15:29 -0800, Mitchell Blank Jr wrote:
> Stephen C. Tweedie wrote:
> > If we want to fix this, let's fix the macros: for example, convert
> > EXT3_HAS_COMPAT_FEATURE to be
> > 
> > 	( le32_to_cpu(EXT3_SB(sb)->s_es->s_feature_compat) & (mask) )
> 
> Of course that's less efficient though since "mask" is probably constant..
> so now the endian conversion changed from compile-time to run-time.
> 
> Would something like
> 
>  	( ( EXT3_SB(sb)->s_es->s_feature_compat & cpu_to_le32(mask) ) != 0)
> 
> be enough to satisfy sparse?

Or we could cast "mask" to the appropriate type (I'm not sure what sparse
uses to determine this).

Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://members.shaw.ca/adilger/             http://members.shaw.ca/golinux/


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]