Skip to contentSkip to conversation

2.4.28-pre2 and ntfs-2.1.6b ?

PATCH In review O.Sezer 3 participants 5 messages Aug 26, 2004 – Aug 27, 2004 MM core

lore
Share & tools
lore

O.Sezer

author
#0 lore raw

Hi all:

With 2.4.28-pre2, ntfs-2.1.6b from linux-ntfs site started failing to compile at aops.c:

aops.c: In function `ntfs_read_block':
aops.c:315: parse error before "else" -- or in case of gcc3.4 --
aops.c:315: error: syntax error before "else"

This happens with gcc-3.2.2 and gcc-3.4.0 and can be fixed by:

aops.c renamed from aops.c.BAK +3 -2
@@ -310,10 +310,11 @@
 		return 0;
 	}
 	/* No i/o was scheduled on any of the buffers. */
-	if (likely(!PageError(page)))
+	if (likely(!PageError(page))) {
 		SetPageUptodate(page);
-	else /* Signal synchronous i/o error. */
+	} else { /* Signal synchronous i/o error. */
 		nr = -EIO;
+	}
 	unlock_page(page);
 	return nr;
 }

The very same code used to compile fine with 2.4.27 without any changes to it. I can't see where the problem is (it's 23:57 here ;)). Can anyone tell, please?

Regards,
Ozkan Sezer

#1 lore raw

Hi,

Quoted 19 lines · O.Sezer

Hi all:

With 2.4.28-pre2, ntfs-2.1.6b from linux-ntfs site started failing to compile at aops.c:

aops.c: In function `ntfs_read_block':
aops.c:315: parse error before "else"
-- or in case of gcc3.4 --
aops.c:315: error: syntax error before "else"

This happens with gcc-3.2.2 and gcc-3.4.0
and can be fixed by:

aops.c renamed from aops.c.BAK +0 -0
@@ -310,10 +310,11 @@
 		return 0;
 	}
 	/* No i/o was scheduled on any of the buffers. */

aops.c line 320

Quoted 10 lines
aops.c renamed from aops.c.BAK +3 -2
-	if (likely(!PageError(page)))
+	if (likely(!PageError(page))) {
 		SetPageUptodate(page);
-	else /* Signal synchronous i/o error. */
+	} else { /* Signal synchronous i/o error. */
 		nr = -EIO;
+	}
 	unlock_page(page);
 	return nr;
 }

No !
Please don't fix it this way ! The problem lies within the declaration of SetPageUptodate() which it seems is a macro which lacks some braces somewhere. You were very lucky that there was an 'else' to point it out, but imagine what it would do in the following case :

	if (likely(!PageError(page))) {
 		SetPageUptodate(page);
	blah();

It would always execute the second half of SetPageUptodate(), whatever the condition, and nothing will alert you.

What does SetPageUptodate() look like ?

Quoted 2 lines

The very same code used to compile fine with
2.4.27 without any changes to it.

I think that the gcc-3.4 fixes might have hit some sensible parts...

Quoted 3 lines

I can't see
where the problem is (it's 23:57 here ;)).
Can anyone tell, please?

It's even later now, good night :-)

Regards,
Willy

author
#2 lore raw
Quoted 65 lines · Willy Tarreau

Hi,

Quoted 19 lines · O.Sezer

Hi all:

With 2.4.28-pre2, ntfs-2.1.6b from linux-ntfs site started failing to compile at aops.c:

aops.c: In function `ntfs_read_block':
aops.c:315: parse error before "else"
-- or in case of gcc3.4 --
aops.c:315: error: syntax error before "else"

This happens with gcc-3.2.2 and gcc-3.4.0
and can be fixed by:

aops.c renamed from aops.c.BAK +0 -0
@@ -310,10 +310,11 @@
 		return 0;
 	}
 	/* No i/o was scheduled on any of the buffers. */

aops.c line 320

Quoted 10 lines
aops.c renamed from aops.c.BAK +3 -2
-	if (likely(!PageError(page)))
+	if (likely(!PageError(page))) {
 		SetPageUptodate(page);
-	else /* Signal synchronous i/o error. */
+	} else { /* Signal synchronous i/o error. */
 		nr = -EIO;
+	}
 	unlock_page(page);
 	return nr;
 }

No !
Please don't fix it this way ! The problem lies within the declaration of SetPageUptodate() which it seems is a macro which lacks some braces somewhere. You were very lucky that there was an 'else' to point it out, but imagine what it would do in the following case :

	if (likely(!PageError(page))) {
 		SetPageUptodate(page);
	blah();

It would always execute the second half of SetPageUptodate(), whatever the condition, and nothing will alert you.

What does SetPageUptodate() look like ?

Quoted 2 lines

The very same code used to compile fine with
2.4.27 without any changes to it.

I think that the gcc-3.4 fixes might have hit some sensible parts...

Quoted 3 lines

I can't see
where the problem is (it's 23:57 here ;)).
Can anyone tell, please?

It's even later now, good night :-)

Quoted 10 lines

Regards,
Willy

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Hi Willy, and thanks, you're a lifesaver! All the evidence points to the s390 changes in -pre2, specificly cset-1.1514 by schwidefsky which touches include/linux/mm.h this way:

include/linux/mm.h +0 -0
@@ -308,11 +308,9 @@

/* Make it prettier to test the above... */
#define UnlockPage(page) unlock_page(page)
#define Page_Uptodate(page) test_bit(PG_uptodate, &(page)->flags)
-#define SetPageUptodate(page) \
- do { \
- arch_set_page_uptodate(page); \
- set_bit(PG_uptodate, &(page)->flags); \
- } while (0)
+#ifndef SetPageUptodate
+#define SetPageUptodate(page) set_bit(PG_uptodate, &(page)->flags);
+#endif
#define ClearPageUptodate(page) clear_bit(PG_uptodate, &(page)->flags)
#define PageDirty(page) test_bit(PG_dirty, &(page)->flags)
#define SetPageDirty(page) set_bit(PG_dirty, &(page)->flags)

Marcelo, you maybe interested in this.

Cheers,
Ozkan Sezer

author
#3 lore raw

PATCH 1/1 fix typo in mm.h introduced 2.4.28-pre2

Quoted 22 lines

All the evidence points
to the s390 changes in -pre2, specificly cset-1.1514 by schwidefsky which touches include/linux/mm.h this way:

include/linux/mm.h 2004-08-26 15:51:04 -07:00 +3 -5
@@ -308,11 +308,9 @@
 /* Make it prettier to test the above... */
 #define UnlockPage(page)    unlock_page(page)
 #define Page_Uptodate(page)    test_bit(PG_uptodate, &(page)->flags)
-#define SetPageUptodate(page) \
-    do {                                \
-        arch_set_page_uptodate(page);                \
-        set_bit(PG_uptodate, &(page)->flags);            \
-    } while (0)
+#ifndef SetPageUptodate
+#define SetPageUptodate(page)    set_bit(PG_uptodate, &(page)->flags);
+#endif
 #define ClearPageUptodate(page)    clear_bit(PG_uptodate, &(page)->flags)
 #define PageDirty(page)        test_bit(PG_dirty, &(page)->flags)
 #define SetPageDirty(page)    set_bit(PG_dirty, &(page)->flags)
Quoted 5 lines

Marcelo, you maybe interested in this.

Cheers,
Ozkan Sezer

And I beleive this was a typo? Marcelo, please review and apply.

Ozkan Sezer

include/linux/mm.h renamed from include/linux/mm.h.BAK +1 -1
@@ -309,7 +309,7 @@
 #define UnlockPage(page)	unlock_page(page)
 #define Page_Uptodate(page)	test_bit(PG_uptodate, &(page)->flags)
 #ifndef SetPageUptodate
-#define SetPageUptodate(page)	set_bit(PG_uptodate, &(page)->flags);
+#define SetPageUptodate(page)	set_bit(PG_uptodate, &(page)->flags)
 #endif
 #define ClearPageUptodate(page)	clear_bit(PG_uptodate, &(page)->flags)
 #define PageDirty(page)		test_bit(PG_dirty, &(page)->flags)
#4 lore raw

Re: [PATCH 2.4] fix typo in mm.h introduced 2.4.28-pre2

include/linux/mm.h line 312

Quoted 12 lines · O.Sezer

And I beleive this was a typo? Marcelo, please review and apply.

Ozkan Sezer

include/linux/mm.h renamed from include/linux/mm.h.BAK +1 -1
@@ -309,7 +309,7 @@
 #define UnlockPage(page)	unlock_page(page)
 #define Page_Uptodate(page)	test_bit(PG_uptodate, &(page)->flags)
 #ifndef SetPageUptodate
-#define SetPageUptodate(page)	set_bit(PG_uptodate, &(page)->flags);
+#define SetPageUptodate(page)	set_bit(PG_uptodate, &(page)->flags)

ACK. That semi-colon must go.