[2.6.14-rc2-git] NTFS: Even more bug fixes!

[2.6.14-rc2-git] NTFS: Even more bug fixes!

[Anton Altaparmakov]

Hi Linus, please pull from

rsync://rsync.kernel.org/pub/scm/linux/kernel/git/aia21/ntfs-2.6.git/HEAD

 fs/ntfs/ChangeLog  |   26 +++++++++++++++++---------
 fs/ntfs/Makefile   |    4 ++--
 fs/ntfs/layout.h   |    9 +++++----
 fs/ntfs/lcnalloc.c |   31 +++++++++++++------------------
 fs/ntfs/lcnalloc.h |   27 +++++++++++++--------------
 fs/ntfs/logfile.c  |   30 +++++++++++++++++++++++++-----
 fs/ntfs/logfile.h  |    2 +-
 fs/ntfs/mft.c      |    2 +-
 8 files changed, 77 insertions(+), 54 deletions(-)

This contains more bugfixes for NTFS that need to go in before 2.6.14
is released.  -  Please apply.  Thanks!  -  There should really not be any 
more for 2.6.14!  Famous last, last words...

Diff style patches generated with git format-patch follow as replies to 
this email.

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/

[PATCH 1/4] NTFS: Fix sparse warnings that have crept in over time.

[Anton Altaparmakov]

NTFS: Fix sparse warnings that have crept in over time.

Signed-off-by: Anton Altaparmakov <aia21@cantab.net>

---

 fs/ntfs/ChangeLog |    4 ++++
 fs/ntfs/layout.h  |    7 ++++---
 fs/ntfs/logfile.h |    2 +-
 3 files changed, 9 insertions(+), 4 deletions(-)

91fbc6edfa7086b5fcdb74ea82ab747104541f1f
diff --git a/fs/ntfs/ChangeLog b/fs/ntfs/ChangeLog
--- a/fs/ntfs/ChangeLog
+++ b/fs/ntfs/ChangeLog
@@ -22,6 +22,10 @@ ToDo/Notes:
 	- Enable the code for setting the NT4 compatibility flag when we start
 	  making NTFS 1.2 specific modifications.
 
+2.1.25-WIP
+
+	- Fix sparse warnings that have crept in over time.
+
 2.1.24 - Lots of bug fixes and support more clean journal states.
 
 	- Support journals ($LogFile) which have been modified by chkdsk.  This
diff --git a/fs/ntfs/layout.h b/fs/ntfs/layout.h
--- a/fs/ntfs/layout.h
+++ b/fs/ntfs/layout.h
@@ -317,12 +317,13 @@ typedef u64 MFT_REF;
 typedef le64 leMFT_REF;
 
 #define MK_MREF(m, s)	((MFT_REF)(((MFT_REF)(s) << 48) |		\
-					((MFT_REF)(m) & MFT_REF_MASK_CPU)))
+					((MFT_REF)(m) & (u64)MFT_REF_MASK_CPU)))
 #define MK_LE_MREF(m, s) cpu_to_le64(MK_MREF(m, s))
 
-#define MREF(x)		((unsigned long)((x) & MFT_REF_MASK_CPU))
+#define MREF(x)		((unsigned long)((x) & (u64)MFT_REF_MASK_CPU))
 #define MSEQNO(x)	((u16)(((x) >> 48) & 0xffff))
-#define MREF_LE(x)	((unsigned long)(le64_to_cpu(x) & MFT_REF_MASK_CPU))
+#define MREF_LE(x)	((unsigned long)(le64_to_cpu(x) &		\
+					(u64)MFT_REF_MASK_CPU))
 #define MSEQNO_LE(x)	((u16)((le64_to_cpu(x) >> 48) & 0xffff))
 
 #define IS_ERR_MREF(x)	(((x) & 0x0000800000000000ULL) ? 1 : 0)
diff --git a/fs/ntfs/logfile.h b/fs/ntfs/logfile.h
--- a/fs/ntfs/logfile.h
+++ b/fs/ntfs/logfile.h
@@ -113,7 +113,7 @@ typedef struct {
  */
 enum {
 	RESTART_VOLUME_IS_CLEAN	= const_cpu_to_le16(0x0002),
-	RESTART_SPACE_FILLER	= 0xffff, /* gcc: Force enum bit width to 16. */
+	RESTART_SPACE_FILLER	= const_cpu_to_le16(0xffff), /* gcc: Force enum bit width to 16. */
 } __attribute__ ((__packed__));
 
 typedef le16 RESTART_AREA_FLAGS;

[PATCH 2/4] NTFS: Change ntfs_cluster_free() to require a write locked runlist on entry

[Anton Altaparmakov]

NTFS: Change ntfs_cluster_free() to require a write locked runlist on entry
      since we otherwise get into a lock reversal deadlock if a read locked
      runlist is passed in. In the process also change it to take an ntfs
      inode instead of a vfs inode as parameter.

Signed-off-by: Anton Altaparmakov <aia21@cantab.net>

---

 fs/ntfs/ChangeLog  |    4 ++++
 fs/ntfs/Makefile   |    2 +-
 fs/ntfs/lcnalloc.c |   31 +++++++++++++------------------
 fs/ntfs/lcnalloc.h |   27 +++++++++++++--------------
 fs/ntfs/mft.c      |    2 +-
 5 files changed, 32 insertions(+), 34 deletions(-)

715dc636b64b57aee7aee7e8b5bf4f5267a6df48
diff --git a/fs/ntfs/ChangeLog b/fs/ntfs/ChangeLog
--- a/fs/ntfs/ChangeLog
+++ b/fs/ntfs/ChangeLog
@@ -25,6 +25,10 @@ ToDo/Notes:
 2.1.25-WIP
 
 	- Fix sparse warnings that have crept in over time.
+	- Change ntfs_cluster_free() to require a write locked runlist on entry
+	  since we otherwise get into a lock reversal deadlock if a read locked
+	  runlist is passed in. In the process also change it to take an ntfs
+	  inode instead of a vfs inode as parameter.
 
 2.1.24 - Lots of bug fixes and support more clean journal states.
 
diff --git a/fs/ntfs/Makefile b/fs/ntfs/Makefile
--- a/fs/ntfs/Makefile
+++ b/fs/ntfs/Makefile
@@ -6,7 +6,7 @@ ntfs-objs := aops.o attrib.o collate.o c
 	     index.o inode.o mft.o mst.o namei.o runlist.o super.o sysctl.o \
 	     unistr.o upcase.o
 
-EXTRA_CFLAGS = -DNTFS_VERSION=\"2.1.24\"
+EXTRA_CFLAGS = -DNTFS_VERSION=\"2.1.25-WIP\"
 
 ifeq ($(CONFIG_NTFS_DEBUG),y)
 EXTRA_CFLAGS += -DDEBUG
diff --git a/fs/ntfs/lcnalloc.c b/fs/ntfs/lcnalloc.c
--- a/fs/ntfs/lcnalloc.c
+++ b/fs/ntfs/lcnalloc.c
@@ -779,14 +779,13 @@ out:
 
 /**
  * __ntfs_cluster_free - free clusters on an ntfs volume
- * @vi:		vfs inode whose runlist describes the clusters to free
- * @start_vcn:	vcn in the runlist of @vi at which to start freeing clusters
+ * @ni:		ntfs inode whose runlist describes the clusters to free
+ * @start_vcn:	vcn in the runlist of @ni at which to start freeing clusters
  * @count:	number of clusters to free or -1 for all clusters
- * @write_locked:	true if the runlist is locked for writing
  * @is_rollback:	true if this is a rollback operation
  *
  * Free @count clusters starting at the cluster @start_vcn in the runlist
- * described by the vfs inode @vi.
+ * described by the vfs inode @ni.
  *
  * If @count is -1, all clusters from @start_vcn to the end of the runlist are
  * deallocated.  Thus, to completely free all clusters in a runlist, use
@@ -801,31 +800,28 @@ out:
  * Return the number of deallocated clusters (not counting sparse ones) on
  * success and -errno on error.
  *
- * Locking: - The runlist described by @vi must be locked on entry and is
- *	      locked on return.  Note if the runlist is locked for reading the
- *	      lock may be dropped and reacquired.  Note the runlist may be
- *	      modified when needed runlist fragments need to be mapped.
+ * Locking: - The runlist described by @ni must be locked for writing on entry
+ *	      and is locked on return.  Note the runlist may be modified when
+ *	      needed runlist fragments need to be mapped.
  *	    - The volume lcn bitmap must be unlocked on entry and is unlocked
  *	      on return.
  *	    - This function takes the volume lcn bitmap lock for writing and
  *	      modifies the bitmap contents.
  */
-s64 __ntfs_cluster_free(struct inode *vi, const VCN start_vcn, s64 count,
-		const BOOL write_locked, const BOOL is_rollback)
+s64 __ntfs_cluster_free(ntfs_inode *ni, const VCN start_vcn, s64 count,
+		const BOOL is_rollback)
 {
 	s64 delta, to_free, total_freed, real_freed;
-	ntfs_inode *ni;
 	ntfs_volume *vol;
 	struct inode *lcnbmp_vi;
 	runlist_element *rl;
 	int err;
 
-	BUG_ON(!vi);
+	BUG_ON(!ni);
 	ntfs_debug("Entering for i_ino 0x%lx, start_vcn 0x%llx, count "
-			"0x%llx.%s", vi->i_ino, (unsigned long long)start_vcn,
+			"0x%llx.%s", ni->mft_no, (unsigned long long)start_vcn,
 			(unsigned long long)count,
 			is_rollback ? " (rollback)" : "");
-	ni = NTFS_I(vi);
 	vol = ni->vol;
 	lcnbmp_vi = vol->lcnbmp_ino;
 	BUG_ON(!lcnbmp_vi);
@@ -843,7 +839,7 @@ s64 __ntfs_cluster_free(struct inode *vi
 
 	total_freed = real_freed = 0;
 
-	rl = ntfs_attr_find_vcn_nolock(ni, start_vcn, write_locked);
+	rl = ntfs_attr_find_vcn_nolock(ni, start_vcn, TRUE);
 	if (IS_ERR(rl)) {
 		if (!is_rollback)
 			ntfs_error(vol->sb, "Failed to find first runlist "
@@ -897,7 +893,7 @@ s64 __ntfs_cluster_free(struct inode *vi
 
 			/* Attempt to map runlist. */
 			vcn = rl->vcn;
-			rl = ntfs_attr_find_vcn_nolock(ni, vcn, write_locked);
+			rl = ntfs_attr_find_vcn_nolock(ni, vcn, TRUE);
 			if (IS_ERR(rl)) {
 				err = PTR_ERR(rl);
 				if (!is_rollback)
@@ -965,8 +961,7 @@ err_out:
 	 * If rollback fails, set the volume errors flag, emit an error
 	 * message, and return the error code.
 	 */
-	delta = __ntfs_cluster_free(vi, start_vcn, total_freed, write_locked,
-			TRUE);
+	delta = __ntfs_cluster_free(ni, start_vcn, total_freed, TRUE);
 	if (delta < 0) {
 		ntfs_error(vol->sb, "Failed to rollback (error %i).  Leaving "
 				"inconsistent metadata!  Unmount and run "
diff --git a/fs/ntfs/lcnalloc.h b/fs/ntfs/lcnalloc.h
--- a/fs/ntfs/lcnalloc.h
+++ b/fs/ntfs/lcnalloc.h
@@ -2,7 +2,7 @@
  * lcnalloc.h - Exports for NTFS kernel cluster (de)allocation.  Part of the
  *		Linux-NTFS project.
  *
- * Copyright (c) 2004 Anton Altaparmakov
+ * Copyright (c) 2004-2005 Anton Altaparmakov
  *
  * This program/include file is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License as published
@@ -28,6 +28,7 @@
 #include <linux/fs.h>
 
 #include "types.h"
+#include "inode.h"
 #include "runlist.h"
 #include "volume.h"
 
@@ -42,18 +43,17 @@ extern runlist_element *ntfs_cluster_all
 		const VCN start_vcn, const s64 count, const LCN start_lcn,
 		const NTFS_CLUSTER_ALLOCATION_ZONES zone);
 
-extern s64 __ntfs_cluster_free(struct inode *vi, const VCN start_vcn,
-		s64 count, const BOOL write_locked, const BOOL is_rollback);
+extern s64 __ntfs_cluster_free(ntfs_inode *ni, const VCN start_vcn,
+		s64 count, const BOOL is_rollback);
 
 /**
  * ntfs_cluster_free - free clusters on an ntfs volume
- * @vi:		vfs inode whose runlist describes the clusters to free
- * @start_vcn:	vcn in the runlist of @vi at which to start freeing clusters
+ * @ni:		ntfs inode whose runlist describes the clusters to free
+ * @start_vcn:	vcn in the runlist of @ni at which to start freeing clusters
  * @count:	number of clusters to free or -1 for all clusters
- * @write_locked:	true if the runlist is locked for writing
  *
  * Free @count clusters starting at the cluster @start_vcn in the runlist
- * described by the vfs inode @vi.
+ * described by the ntfs inode @ni.
  *
  * If @count is -1, all clusters from @start_vcn to the end of the runlist are
  * deallocated.  Thus, to completely free all clusters in a runlist, use
@@ -65,19 +65,18 @@ extern s64 __ntfs_cluster_free(struct in
  * Return the number of deallocated clusters (not counting sparse ones) on
  * success and -errno on error.
  *
- * Locking: - The runlist described by @vi must be locked on entry and is
- *	      locked on return.  Note if the runlist is locked for reading the
- *	      lock may be dropped and reacquired.  Note the runlist may be
- *	      modified when needed runlist fragments need to be mapped.
+ * Locking: - The runlist described by @ni must be locked for writing on entry
+ *	      and is locked on return.  Note the runlist may be modified when
+ *	      needed runlist fragments need to be mapped.
  *	    - The volume lcn bitmap must be unlocked on entry and is unlocked
  *	      on return.
  *	    - This function takes the volume lcn bitmap lock for writing and
  *	      modifies the bitmap contents.
  */
-static inline s64 ntfs_cluster_free(struct inode *vi, const VCN start_vcn,
-		s64 count, const BOOL write_locked)
+static inline s64 ntfs_cluster_free(ntfs_inode *ni, const VCN start_vcn,
+		s64 count)
 {
-	return __ntfs_cluster_free(vi, start_vcn, count, write_locked, FALSE);
+	return __ntfs_cluster_free(ni, start_vcn, count, FALSE);
 }
 
 extern int ntfs_cluster_free_from_rl_nolock(ntfs_volume *vol,
diff --git a/fs/ntfs/mft.c b/fs/ntfs/mft.c
--- a/fs/ntfs/mft.c
+++ b/fs/ntfs/mft.c
@@ -1953,7 +1953,7 @@ restore_undo_alloc:
 	a = ctx->attr;
 	a->data.non_resident.highest_vcn = cpu_to_sle64(old_last_vcn - 1);
 undo_alloc:
-	if (ntfs_cluster_free(vol->mft_ino, old_last_vcn, -1, TRUE) < 0) {
+	if (ntfs_cluster_free(mft_ni, old_last_vcn, -1) < 0) {
 		ntfs_error(vol->sb, "Failed to free clusters from mft data "
 				"attribute.%s", es);
 		NVolSetErrors(vol);

[PATCH 3/4] NTFS: Fix the definition of the CHKD ntfs record magic.

[Anton Altaparmakov]

NTFS: Fix the definition of the CHKD ntfs record magic.  It had an off by
      two error causing it to be CHKB instead of CHKD.

Signed-off-by: Anton Altaparmakov <aia21@cantab.net>

---

 fs/ntfs/layout.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

838bf9675a3d1ede01408aa105357b9ab43faf1b
diff --git a/fs/ntfs/layout.h b/fs/ntfs/layout.h
--- a/fs/ntfs/layout.h
+++ b/fs/ntfs/layout.h
@@ -123,7 +123,7 @@ enum {
 	magic_RCRD = const_cpu_to_le32(0x44524352), /* Log record page. */
 
 	/* Found in $LogFile/$DATA.  (May be found in $MFT/$DATA, also?) */
-	magic_CHKD = const_cpu_to_le32(0x424b4843), /* Modified by chkdsk. */
+	magic_CHKD = const_cpu_to_le32(0x444b4843), /* Modified by chkdsk. */
 
 	/* Found in all ntfs record containing records. */
 	magic_BAAD = const_cpu_to_le32(0x44414142), /* Failed multi sector

[PATCH 4/4] NTFS: More $LogFile handling fixes: when chkdsk has been run, it can leave the

[Anton Altaparmakov]

NTFS: More $LogFile handling fixes: when chkdsk has been run, it can leave the
      restart pages in the journal without multi sector transfer protection
      fixups (i.e. the update sequence array is empty and in fact does not
      exist).

Signed-off-by: Anton Altaparmakov <aia21@cantab.net>

---

 fs/ntfs/ChangeLog |   18 +++++++++---------
 fs/ntfs/Makefile  |    2 +-
 fs/ntfs/logfile.c |   30 +++++++++++++++++++++++++-----
 3 files changed, 35 insertions(+), 15 deletions(-)

5a8c0cc32bb6e029cd9c36f655c6b0955b0d9967
diff --git a/fs/ntfs/ChangeLog b/fs/ntfs/ChangeLog
--- a/fs/ntfs/ChangeLog
+++ b/fs/ntfs/ChangeLog
@@ -22,14 +22,6 @@ ToDo/Notes:
 	- Enable the code for setting the NT4 compatibility flag when we start
 	  making NTFS 1.2 specific modifications.
 
-2.1.25-WIP
-
-	- Fix sparse warnings that have crept in over time.
-	- Change ntfs_cluster_free() to require a write locked runlist on entry
-	  since we otherwise get into a lock reversal deadlock if a read locked
-	  runlist is passed in. In the process also change it to take an ntfs
-	  inode instead of a vfs inode as parameter.
-
 2.1.24 - Lots of bug fixes and support more clean journal states.
 
 	- Support journals ($LogFile) which have been modified by chkdsk.  This
@@ -37,7 +29,8 @@ ToDo/Notes:
 	  The Windows boot will run chkdsk and then reboot.  The user can then
 	  immediately boot into Linux rather than having to do a full Windows
 	  boot first before rebooting into Linux and we will recognize such a
-	  journal and empty it as it is clean by definition.
+	  journal and empty it as it is clean by definition.  Note, this only
+	  works if chkdsk left the journal in an obviously clean state.
 	- Support journals ($LogFile) with only one restart page as well as
 	  journals with two different restart pages.  We sanity check both and
 	  either use the only sane one or the more recent one of the two in the
@@ -102,6 +95,13 @@ ToDo/Notes:
 	  my ways.
 	- Fix various bugs in the runlist merging code.  (Based on libntfs
 	  changes by Richard Russon.)
+	- Fix sparse warnings that have crept in over time.
+	- Change ntfs_cluster_free() to require a write locked runlist on entry
+	  since we otherwise get into a lock reversal deadlock if a read locked
+	  runlist is passed in. In the process also change it to take an ntfs
+	  inode instead of a vfs inode as parameter.
+	- Fix the definition of the CHKD ntfs record magic.  It had an off by
+	  two error causing it to be CHKB instead of CHKD.
 
 2.1.23 - Implement extension of resident files and make writing safe as well as
 	 many bug fixes, cleanups, and enhancements...
diff --git a/fs/ntfs/Makefile b/fs/ntfs/Makefile
--- a/fs/ntfs/Makefile
+++ b/fs/ntfs/Makefile
@@ -6,7 +6,7 @@ ntfs-objs := aops.o attrib.o collate.o c
 	     index.o inode.o mft.o mst.o namei.o runlist.o super.o sysctl.o \
 	     unistr.o upcase.o
 
-EXTRA_CFLAGS = -DNTFS_VERSION=\"2.1.25-WIP\"
+EXTRA_CFLAGS = -DNTFS_VERSION=\"2.1.24\"
 
 ifeq ($(CONFIG_NTFS_DEBUG),y)
 EXTRA_CFLAGS += -DDEBUG
diff --git a/fs/ntfs/logfile.c b/fs/ntfs/logfile.c
--- a/fs/ntfs/logfile.c
+++ b/fs/ntfs/logfile.c
@@ -51,7 +51,8 @@ static BOOL ntfs_check_restart_page_head
 		RESTART_PAGE_HEADER *rp, s64 pos)
 {
 	u32 logfile_system_page_size, logfile_log_page_size;
-	u16 usa_count, usa_ofs, usa_end, ra_ofs;
+	u16 ra_ofs, usa_count, usa_ofs, usa_end = 0;
+	BOOL have_usa = TRUE;
 
 	ntfs_debug("Entering.");
 	/*
@@ -86,6 +87,14 @@ static BOOL ntfs_check_restart_page_head
 				(int)sle16_to_cpu(rp->minor_ver));
 		return FALSE;
 	}
+	/*
+	 * If chkdsk has been run the restart page may not be protected by an
+	 * update sequence array.
+	 */
+	if (ntfs_is_chkd_record(rp->magic) && !le16_to_cpu(rp->usa_count)) {
+		have_usa = FALSE;
+		goto skip_usa_checks;
+	}
 	/* Verify the size of the update sequence array. */
 	usa_count = 1 + (logfile_system_page_size >> NTFS_BLOCK_SIZE_BITS);
 	if (usa_count != le16_to_cpu(rp->usa_count)) {
@@ -102,6 +111,7 @@ static BOOL ntfs_check_restart_page_head
 				"inconsistent update sequence array offset.");
 		return FALSE;
 	}
+skip_usa_checks:
 	/*
 	 * Verify the position of the restart area.  It must be:
 	 *	- aligned to 8-byte boundary,
@@ -109,7 +119,8 @@ static BOOL ntfs_check_restart_page_head
 	 *	- within the system page size.
 	 */
 	ra_ofs = le16_to_cpu(rp->restart_area_offset);
-	if (ra_ofs & 7 || ra_ofs < usa_end ||
+	if (ra_ofs & 7 || (have_usa ? ra_ofs < usa_end :
+			ra_ofs < sizeof(RESTART_PAGE_HEADER)) ||
 			ra_ofs > logfile_system_page_size) {
 		ntfs_error(vi->i_sb, "$LogFile restart page specifies "
 				"inconsistent restart area offset.");
@@ -402,8 +413,12 @@ static int ntfs_check_and_load_restart_p
 			idx++;
 		} while (to_read > 0);
 	}
-	/* Perform the multi sector transfer deprotection on the buffer. */
-	if (post_read_mst_fixup((NTFS_RECORD*)trp,
+	/*
+	 * Perform the multi sector transfer deprotection on the buffer if the
+	 * restart page is protected.
+	 */
+	if ((!ntfs_is_chkd_record(trp->magic) || le16_to_cpu(trp->usa_count))
+			&& post_read_mst_fixup((NTFS_RECORD*)trp,
 			le32_to_cpu(rp->system_page_size))) {
 		/*
 		 * A multi sector tranfer error was detected.  We only need to
@@ -615,11 +630,16 @@ is_empty:
 		 * Otherwise just throw it away.
 		 */
 		if (rstr2_lsn > rstr1_lsn) {
+			ntfs_debug("Using second restart page as it is more "
+					"recent.");
 			ntfs_free(rstr1_ph);
 			rstr1_ph = rstr2_ph;
 			/* rstr1_lsn = rstr2_lsn; */
-		} else
+		} else {
+			ntfs_debug("Using first restart page as it is more "
+					"recent.");
 			ntfs_free(rstr2_ph);
+		}
 		rstr2_ph = NULL;
 	}
 	/* All consistency checks passed. */

Re: [PATCH 1/4] NTFS: Fix sparse warnings that have crept in over time.

[Linus Torvalds]

On Mon, 26 Sep 2005, Anton Altaparmakov wrote:
>
> NTFS: Fix sparse warnings that have crept in over time.

I think this is wrong.

What was the warning that caused this (and the two other things that look 
the same)?

>  #define MK_MREF(m, s)	((MFT_REF)(((MFT_REF)(s) << 48) |		\
> -					((MFT_REF)(m) & MFT_REF_MASK_CPU)))
> +					((MFT_REF)(m) & (u64)MFT_REF_MASK_CPU)))

Also, side note: how you defined "MFT_REF_MASK_CPU" is pretty debatable in 
the first place:

	typedef enum {
	        MFT_REF_MASK_CPU        = 0x0000ffffffffffffULL,
	        MFT_REF_MASK_LE         = const_cpu_to_le64(0x0000ffffffffffffULL),
	} MFT_REF_CONSTS;

and this just _happens_ to work with gcc, but it's not real C.

The issue? "enum" is really an integer type. As in "int". Trying to put a 
larger value than one that fits in "int" is not guaranteed to work.

There's another issue, namely that the type of the snum is not only of 
undefined size (is it the same size as an "int"? Is it an "unsigned long 
long"?) but the "endianness" of it is also now totally undefined. You have 
two different endiannesses inside the _same_ enum. What is the type of the 
enum?

You _really_ probably should use just

	#define MFT_REF_MASK_CPU 0x0000ffffffffffffULL
	#define MFT_REF_MASK_LE const_cpu_to_le64(MFT_REF_MASK_CPU)

instead. That way the type of that thing is well-defined.

Depending on what warning you're trying to shut up, that may well fix it 
too. Because now "MFT_REF_MASK_CPU" is clearly a regular constant, while 
MFT_REF_MASK_LE is clearly a little-endian constant. Before, they were of 
the same enum type, which made it very unclear what the hell they were.

		Linus

Re: [PATCH 1/4] NTFS: Fix sparse warnings that have crept in over time.

[Anton Altaparmakov]

On Mon, 26 Sep 2005, Linus Torvalds wrote:
> On Mon, 26 Sep 2005, Anton Altaparmakov wrote:
> >
> > NTFS: Fix sparse warnings that have crept in over time.
> 
> I think this is wrong.
> 
> What was the warning that caused this (and the two other things that look 
> the same)?

fs/ntfs/mft.c:2577:24: warning: incompatible types for operation (&)
fs/ntfs/mft.c:2577:24:    left side has type unsigned long long [unsigned] 
[usertype] <noident>
fs/ntfs/mft.c:2577:24:    right side has type bad type enum MFT_REF_CONSTS 
[toplevel] MFT_REF_MASK_CPU
fs/ntfs/mft.c:2577:24: warning: cast from unknown type

> >  #define MK_MREF(m, s)	((MFT_REF)(((MFT_REF)(s) << 48) |		\
> > -					((MFT_REF)(m) & MFT_REF_MASK_CPU)))
> > +					((MFT_REF)(m) & (u64)MFT_REF_MASK_CPU)))
> 
> Also, side note: how you defined "MFT_REF_MASK_CPU" is pretty debatable in 
> the first place:
> 
> 	typedef enum {
> 	        MFT_REF_MASK_CPU        = 0x0000ffffffffffffULL,
> 	        MFT_REF_MASK_LE         = const_cpu_to_le64(0x0000ffffffffffffULL),
> 	} MFT_REF_CONSTS;
> 
> and this just _happens_ to work with gcc, but it's not real C.
> 
> The issue? "enum" is really an integer type. As in "int". Trying to put a 
> larger value than one that fits in "int" is not guaranteed to work.

Yes, that is true but as you say it does work with gcc.

> There's another issue, namely that the type of the snum is not only of 
> undefined size (is it the same size as an "int"? Is it an "unsigned long 
> long"?) but the "endianness" of it is also now totally undefined. You have 
> two different endiannesses inside the _same_ enum. What is the type of the 
> enum?

Good question.  "confused"?  (-;

> You _really_ probably should use just
> 
> 	#define MFT_REF_MASK_CPU 0x0000ffffffffffffULL
> 	#define MFT_REF_MASK_LE const_cpu_to_le64(MFT_REF_MASK_CPU)
> 
> instead. That way the type of that thing is well-defined.

Yes, that is probably better given it is not possible to have them both be 
the same type (by definition).

> Depending on what warning you're trying to shut up, that may well fix it 
> too. Because now "MFT_REF_MASK_CPU" is clearly a regular constant, while 
> MFT_REF_MASK_LE is clearly a little-endian constant. Before, they were of 
> the same enum type, which made it very unclear what the hell they were.

Yes, I would expect it to fix it.

In fact I just did it and yes, it fixes it, too.  The repository I asked 
you to pull from is now updated with the below patch added which reverts 
the wrong layout.h parts and changes the enum to two defines as per your 
suggestion.

rsync://rsync.kernel.org/pub/scm/linux/kernel/git/aia21/ntfs-2.6.git/HEAD

Thanks for the comments.  (-:

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/

---

NTFS: Re-fix sparse warnings in a more correct way, i.e. don't use an enum with
      different types in it but #define the two constants instead.

Signed-off-by: Anton Altaparmakov <aia21@cantab.net>

---

 fs/ntfs/layout.h |   13 +++++--------
 1 files changed, 5 insertions(+), 8 deletions(-)

e2fcc61ef0d654887b651bd99ffcb52f7344b836
diff --git a/fs/ntfs/layout.h b/fs/ntfs/layout.h
--- a/fs/ntfs/layout.h
+++ b/fs/ntfs/layout.h
@@ -308,22 +308,19 @@ typedef le16 MFT_RECORD_FLAGS;
  * The _LE versions are to be applied on little endian MFT_REFs.
  * Note: The _LE versions will return a CPU endian formatted value!
  */
-typedef enum {
-	MFT_REF_MASK_CPU	= 0x0000ffffffffffffULL,
-	MFT_REF_MASK_LE		= const_cpu_to_le64(0x0000ffffffffffffULL),
-} MFT_REF_CONSTS;
+#define MFT_REF_MASK_CPU 0x0000ffffffffffffULL
+#define MFT_REF_MASK_LE const_cpu_to_le64(0x0000ffffffffffffULL)
 
 typedef u64 MFT_REF;
 typedef le64 leMFT_REF;
 
 #define MK_MREF(m, s)	((MFT_REF)(((MFT_REF)(s) << 48) |		\
-					((MFT_REF)(m) & (u64)MFT_REF_MASK_CPU)))
+					((MFT_REF)(m) & MFT_REF_MASK_CPU)))
 #define MK_LE_MREF(m, s) cpu_to_le64(MK_MREF(m, s))
 
-#define MREF(x)		((unsigned long)((x) & (u64)MFT_REF_MASK_CPU))
+#define MREF(x)		((unsigned long)((x) & MFT_REF_MASK_CPU))
 #define MSEQNO(x)	((u16)(((x) >> 48) & 0xffff))
-#define MREF_LE(x)	((unsigned long)(le64_to_cpu(x) &		\
-					(u64)MFT_REF_MASK_CPU))
+#define MREF_LE(x)	((unsigned long)(le64_to_cpu(x) & MFT_REF_MASK_CPU))
 #define MSEQNO_LE(x)	((u16)((le64_to_cpu(x) >> 48) & 0xffff))
 
 #define IS_ERR_MREF(x)	(((x) & 0x0000800000000000ULL) ? 1 : 0)

Re: [PATCH 1/4] NTFS: Fix sparse warnings that have crept in over time.

[Linus Torvalds]

On Mon, 26 Sep 2005, Anton Altaparmakov wrote:
> > 
> > What was the warning that caused this (and the two other things that look 
> > the same)?
> 
> fs/ntfs/mft.c:2577:24: warning: incompatible types for operation (&)
> fs/ntfs/mft.c:2577:24:    left side has type unsigned long long [unsigned] [usertype] <noident>
> fs/ntfs/mft.c:2577:24:    right side has type bad type enum MFT_REF_CONSTS [toplevel] MFT_REF_MASK_CPU
> fs/ntfs/mft.c:2577:24: warning: cast from unknown type

Ok, not the most wonderful of error messages, I do have to admit ;)

That's sparse being very verbose and not saying a lot, but what happens is
that the "enum" doesn't have a well-defined type, since the different 
constants in the enum don't have compatible types.

So it's type ends up being a "bad type enum MFT_REF_CONSTS"

(It also prints out the name of the symbol with that type, which is why
you also see the MFT_REF_MASK_CPU - the "[toplevel]" is just an internal 
sparse bit saying that it was declared outside of any block scope).

I'm actually a bit surprised that the cast even shut sparse up. It
probably shouldn't have, and it should have complained about casting an
unknown type even _with_ your added cast (ie I think it should have cut
your four lines of warning down to one).

Did it?

> > The issue? "enum" is really an integer type. As in "int". Trying to put a 
> > larger value than one that fits in "int" is not guaranteed to work.
> 
> Yes, that is true but as you say it does work with gcc.

Yes, and sparse will actually conform to gcc behaviour. I think we had a 
warning about it, but it's sadly quite common in the kernel ;p

So if the size of the constants was the only problem, sparse wouldn't 
actually have complained.

The reason it ended up complaining was that it couldn't promote the 
different enum values to the same type. 

I suspect it might be more readable had it complained at enum declaration
time instead, since at that point it would have been able to describe
_why_ it didn't like that enum a bit better. But the problem with that 
approach is that then it complains whether the thing is used or not (and a 
lot of things are bad only at usage time, so sparse tends to try to 
delay any complaints as long as computerly possible).

> > There's another issue, namely that the type of the snum is not only of 
> > undefined size (is it the same size as an "int"? Is it an "unsigned long 
> > long"?) but the "endianness" of it is also now totally undefined. You have 
> > two different endiannesses inside the _same_ enum. What is the type of the 
> > enum?
> 
> Good question.  "confused"?  (-;

Well, in gcc it's clear: it's "unsigned long long".  Because gcc doesn't 
know about little-endian vs big-endian.

In sparse, it's not actually confused either, it's "enum of type
bad_ctype". But the error message isn't very helpful unless you understand
how sparse does that internally (that's why sparse says "right side has
type bad type enum ..." - that "bad type enum" is the magic code-word)

		Linus

Re: [PATCH 1/4] NTFS: Fix sparse warnings that have crept in over time.

[Al Viro]

On Mon, Sep 26, 2005 at 07:57:09AM -0700, Linus Torvalds wrote:
> 	#define MFT_REF_MASK_CPU 0x0000ffffffffffffULL
> 	#define MFT_REF_MASK_LE const_cpu_to_le64(MFT_REF_MASK_CPU)
> 
> instead. That way the type of that thing is well-defined.

Or just make those two different enums.  Basically, gcc is hopelessly
b0rken in version-dependent way when it comes to multi-element enums
that get outside of int range.  Single-element ones at least have
kinda-sorta consistent semantics; anything beyond that and you are
walking into very nasty areas.

Re: [PATCH 1/4] NTFS: Fix sparse warnings that have crept in over time.

[Al Viro]

On Mon, Sep 26, 2005 at 09:41:14AM -0700, Linus Torvalds wrote:
> Yes, and sparse will actually conform to gcc behaviour. I think we had a 
> warning about it, but it's sadly quite common in the kernel ;p
 
No, they are not.  unsigned _is_ common (mostly from 1U<<31).  Going
beyond 32 bits is done in a handful of cases, NTFS ones being at least
half of the entire pile.

Re: [PATCH 1/4] NTFS: Fix sparse warnings that have crept in over time.

[Anton Altaparmakov]

On Mon, 26 Sep 2005, Linus Torvalds wrote:
> On Mon, 26 Sep 2005, Anton Altaparmakov wrote:
> > > What was the warning that caused this (and the two other things that look 
> > > the same)?
> > 
> > fs/ntfs/mft.c:2577:24: warning: incompatible types for operation (&)
> > fs/ntfs/mft.c:2577:24:    left side has type unsigned long long [unsigned] [usertype] <noident>
> > fs/ntfs/mft.c:2577:24:    right side has type bad type enum MFT_REF_CONSTS [toplevel] MFT_REF_MASK_CPU
> > fs/ntfs/mft.c:2577:24: warning: cast from unknown type
> 
> Ok, not the most wonderful of error messages, I do have to admit ;)
> 
> That's sparse being very verbose and not saying a lot, but what happens is
> that the "enum" doesn't have a well-defined type, since the different 
> constants in the enum don't have compatible types.
> 
> So it's type ends up being a "bad type enum MFT_REF_CONSTS"
> 
> (It also prints out the name of the symbol with that type, which is why
> you also see the MFT_REF_MASK_CPU - the "[toplevel]" is just an internal 
> sparse bit saying that it was declared outside of any block scope).
> 
> I'm actually a bit surprised that the cast even shut sparse up. It
> probably shouldn't have, and it should have complained about casting an
> unknown type even _with_ your added cast (ie I think it should have cut
> your four lines of warning down to one).
> 
> Did it?

No, the warnings completely disappeared with the cast.

This is using: make CHECKFLAGS=-Wbitwise C=2 modules

> > > The issue? "enum" is really an integer type. As in "int". Trying to put a 
> > > larger value than one that fits in "int" is not guaranteed to work.
> > 
> > Yes, that is true but as you say it does work with gcc.
> 
> Yes, and sparse will actually conform to gcc behaviour. I think we had a 
> warning about it, but it's sadly quite common in the kernel ;p
> 
> So if the size of the constants was the only problem, sparse wouldn't 
> actually have complained.
> 
> The reason it ended up complaining was that it couldn't promote the 
> different enum values to the same type. 
> 
> I suspect it might be more readable had it complained at enum declaration
> time instead, since at that point it would have been able to describe
> _why_ it didn't like that enum a bit better. But the problem with that 
> approach is that then it complains whether the thing is used or not (and a 
> lot of things are bad only at usage time, so sparse tends to try to 
> delay any complaints as long as computerly possible).
> 
> > > There's another issue, namely that the type of the snum is not only of 
> > > undefined size (is it the same size as an "int"? Is it an "unsigned long 
> > > long"?) but the "endianness" of it is also now totally undefined. You have 
> > > two different endiannesses inside the _same_ enum. What is the type of the 
> > > enum?
> > 
> > Good question.  "confused"?  (-;
> 
> Well, in gcc it's clear: it's "unsigned long long".  Because gcc doesn't 
> know about little-endian vs big-endian.
> 
> In sparse, it's not actually confused either, it's "enum of type
> bad_ctype". But the error message isn't very helpful unless you understand
> how sparse does that internally (that's why sparse says "right side has
> type bad type enum ..." - that "bad type enum" is the magic code-word)

Thanks for the explanation.

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/

Re: [PATCH 1/4] NTFS: Fix sparse warnings that have crept in over time.

[Linus Torvalds]

On Mon, 26 Sep 2005, Anton Altaparmakov wrote:
> > 
> > I'm actually a bit surprised that the cast even shut sparse up. It
> > probably shouldn't have, and it should have complained about casting an
> > unknown type even _with_ your added cast (ie I think it should have cut
> > your four lines of warning down to one).
> > 
> > Did it?
> 
> No, the warnings completely disappeared with the cast.

Ok, it turns out that a bad enum type ends up being seen as just its 
integer type, and anything that doesn't care about anything else than its 
size (namely, a cast) won't even complain.

That's very arguably a bug. Oh, well. Not a big one.

		Linus