Re: [Linux-NTFS-Dev] Patch: linux-2.5.30/fs/ntfs BUG_ON(cond1 || cond2) bugs(!) and clean ups

Re: [Linux-NTFS-Dev] Patch: linux-2.5.30/fs/ntfs BUG_ON(cond1 || cond2) bugs(!) and clean ups

[Adam J. Richter]

>Thanks for the patch. Patch accepted and committed to my tree. It will be 
>submitted with the next ntfs point release.

	Wow, thanks for the quick incorporation of my patch.

>I don't feel like this is 
>enough for a release so I will wait to gather some more things before 
>submitting so it may be a week or two, hope you don't mind the delay... If
>nothing comes up, I will just submit it on its own.

	No problem.  I agree that it's not urgent.

Adam J. Richter     __     ______________   575 Oroville Road
adam@yggdrasil.com     \ /                  Milpitas, California 95035
+1 408 309-6081         | g g d r a s i l   United States of America
                         "Free Software For The Rest Of Us."

Re: [Linux-NTFS-Dev] Patch: linux-2.5.30/fs/ntfs BUG_ON(cond1 || cond2) bugs(!) and clean ups

[Pawel Kot]

On Mon, 5 Aug 2002, Adam J. Richter wrote:

> >Thanks for the patch. Patch accepted and committed to my tree. It will be
> >submitted with the next ntfs point release.
>
> 	Wow, thanks for the quick incorporation of my patch.

Just for the info. Along with other updates (few bits from 2.0.23, and
fixed kmap_pte and kmap_proto exports -- issue found by Jörg Prante) it is
also included in upcoming NTFS backport to 2.4.x. Version 2.0.23b should
be available within an hour.

pkot
-- 
mailto:pkot@linuxnews.pl :: mailto:pkot@slackware.pl
http://kt.linuxnews.pl/ :: Kernel Traffic po polsku

Patch: linux-2.5.30/fs/ntfs BUG_ON(cond1 || cond2) bugs(!) and clean ups

[Adam J. Richter]

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

	The following patch replaces all BUG_ON(condition1 || condition2)
statements in fs/ntfs with separate BUG_ON statements, usually like so:

		BUG_ON(condition1);
		BUG_ON(condition2);

	This provides more information if the BUG_ON statement is every
tripped.

	In addition, the fs/ntfs code had some
BUG_ON(...atomic_dec_and_test(...)) statements.  BUG_ON conditions should
not have side effects, because you are supposed to be able to compile out
BUG_ON statements to have the code run faster (which is an important
guarantee for encouraging developers to write BUG_ON statements).  I
have translated the offending cases to statements of the following form:

	if(...atomic_dec_and_test(...))
		BUG();

	I have attached the patch below.  I would like to get these
changes into Linus's 2.5 tree.  Please let me know if you want to take
it from here, if you want me to submit this patch to Linus or if you
want me to do something else.  Thanks for your time, and for maintaining
the NT file system on Linux.

-- 
Adam J. Richter     __     ______________   575 Oroville Road
adam@yggdrasil.com     \ /                  Milpitas, California 95035
+1 408 309-6081         | g g d r a s i l   United States of America
                         "Free Software For The Rest Of Us."

[-- Attachment #2: ntfs.diff --]
[-- Type: text/plain, Size: 3936 bytes --]

diff -u linux-2.5.30/fs/ntfs/attrib.c linux/fs/ntfs/attrib.c
--- linux-2.5.30/fs/ntfs/attrib.c	2002-08-01 14:16:26.000000000 -0700
+++ linux/fs/ntfs/attrib.c	2002-08-05 13:27:51.000000000 -0700
@@ -110,7 +110,8 @@
 static inline BOOL ntfs_are_rl_mergeable(run_list_element *dst,
 		run_list_element *src)
 {
-	BUG_ON(!dst || !src);
+	BUG_ON(!dst);
+	BUG_ON(!src);
 
 	if ((dst->lcn < 0) || (src->lcn < 0))     /* Are we merging holes? */
 		return FALSE;
@@ -192,7 +193,8 @@
 	BOOL right;
 	int magic;
 
-	BUG_ON(!dst || !src);
+	BUG_ON(!dst);
+	BUG_ON(!src);
 
 	/* First, check if the right hand end needs merging. */
 	right = ntfs_are_rl_mergeable(src + ssize - 1, dst + loc + 1);
@@ -258,7 +260,8 @@
 	BOOL hole = FALSE;	/* Following a hole */
 	int magic;
 
-	BUG_ON(!dst || !src);
+	BUG_ON(!dst);
+	BUG_ON(!src);
 
 	/* disc => Discontinuity between the end of @dst and the start of @src.
 	 *         This means we might need to insert a hole.
@@ -362,7 +365,8 @@
 	BOOL right;
 	int magic;
 
-	BUG_ON(!dst || !src);
+	BUG_ON(!dst);
+	BUG_ON(!src);
 
 	/* First, merge the left and right ends, if necessary. */
 	right = ntfs_are_rl_mergeable(src + ssize - 1, dst + loc + 1);
@@ -423,7 +427,8 @@
 static inline run_list_element *ntfs_rl_split(run_list_element *dst, int dsize,
 		run_list_element *src, int ssize, int loc)
 {
-	BUG_ON(!dst || !src);
+	BUG_ON(!dst);
+	BUG_ON(!src);
 
 	/* Space required: @dst size + @src size + one new hole. */
 	dst = ntfs_rl_realloc(dst, dsize, dsize + ssize + 1);
diff -u linux-2.5.30/fs/ntfs/compress.c linux/fs/ntfs/compress.c
--- linux-2.5.30/fs/ntfs/compress.c	2002-08-01 14:16:26.000000000 -0700
+++ linux/fs/ntfs/compress.c	2002-08-05 13:27:51.000000000 -0700
@@ -467,7 +467,8 @@
 	 * Bad things happen if we get here for anything that is not an
 	 * unnamed $DATA attribute.
 	 */
-	BUG_ON(ni->type != AT_DATA || ni->name_len);
+	BUG_ON(ni->type != AT_DATA);
+	BUG_ON(ni->name_len);
 
 	pages = kmalloc(nr_pages * sizeof(struct page *), GFP_NOFS);
 
diff -u linux-2.5.30/fs/ntfs/inode.c linux/fs/ntfs/inode.c
--- linux-2.5.30/fs/ntfs/inode.c	2002-08-01 14:16:19.000000000 -0700
+++ linux/fs/ntfs/inode.c	2002-08-05 13:27:51.000000000 -0700
@@ -278,7 +278,9 @@
 	ntfs_inode *ni = NTFS_I(inode);
 
 	ntfs_debug("Entering.");
-	BUG_ON(atomic_read(&ni->mft_count) || !atomic_dec_and_test(&ni->count));
+	BUG_ON(atomic_read(&ni->mft_count));
+	if (!atomic_dec_and_test(&ni->count))
+		BUG();
 	kmem_cache_free(ntfs_big_inode_cache, NTFS_I(inode));
 }
 
@@ -298,7 +300,11 @@
 void ntfs_destroy_extent_inode(ntfs_inode *ni)
 {
 	ntfs_debug("Entering.");
-	BUG_ON(atomic_read(&ni->mft_count) || !atomic_dec_and_test(&ni->count));
+
+	BUG_ON(atomic_read(&ni->mft_count));
+	if (!atomic_dec_and_test(&ni->count))
+		BUG();
+
 	kmem_cache_free(ntfs_inode_cache, ni);
 }
 
diff -u linux-2.5.30/fs/ntfs/mft.c linux/fs/ntfs/mft.c
--- linux-2.5.30/fs/ntfs/mft.c	2002-08-01 14:16:07.000000000 -0700
+++ linux/fs/ntfs/mft.c	2002-08-05 13:27:51.000000000 -0700
@@ -132,7 +132,9 @@
 	struct page *page;
 	unsigned long index, ofs, end_index;
 
-	BUG_ON(atomic_read(&ni->mft_count) || ni->page);
+	BUG_ON(atomic_read(&ni->mft_count));
+	BUG_ON(ni->page);
+
 	/*
 	 * The index into the page cache and the offset within the page cache
 	 * page of the wanted mft record. FIXME: We need to check for
@@ -190,8 +192,10 @@
  */
 static inline void unmap_mft_record_page(ntfs_inode *ni)
 {
-	BUG_ON(atomic_read(&ni->mft_count) || !ni->page);
-	// TODO: If dirty, blah...
+	BUG_ON(atomic_read(&ni->mft_count));
+	BUG_ON(!ni->page);
+
+	/* TODO: If dirty, blah... */
 	ntfs_unmap_page(ni->page);
 	ni->page = NULL;
 	ni->page_ofs = 0;
@@ -316,7 +320,8 @@
 {
 	struct page *page = ni->page;
 
-	BUG_ON(!atomic_read(&ni->mft_count) || !page);
+	BUG_ON(!atomic_read(&ni->mft_count));
+	BUG_ON(!page);
 
 	ntfs_debug("Entering for mft_no 0x%lx, unmapping from %s.", ni->mft_no,
 			rw == READ ? "READ" : "WRITE");

Re: [Linux-NTFS-Dev] Patch: linux-2.5.30/fs/ntfs BUG_ON(cond1 || cond2) bugs(!) and clean ups

[Anton Altaparmakov]

At 21:41 05/08/02, Adam J. Richter wrote:
>         The following patch replaces all BUG_ON(condition1 || condition2)
>statements in fs/ntfs with separate BUG_ON statements, usually like so:
[snip]
>         I have attached the patch below.  I would like to get these
>changes into Linus's 2.5 tree.  Please let me know if you want to take
>it from here, if you want me to submit this patch to Linus or if you
>want me to do something else.

Thanks for the patch. Patch accepted and committed to my tree. It will be 
submitted with the next ntfs point release. I don't feel like this is 
enough for a release so I will wait to gather some more things before 
submitting so it may be a week or two, hope you don't mind the delay... If 
nothing comes up, I will just submit it on its own.

Note almost none of it applied because it was against 2.5.30 vanilla and 
ntfs has evolved two point releases since then and in fact some of your 
deltas were already there because mft_count has been eliminated. (-: But I 
did the changes by hand so no worries...

>Thanks for your time, and for maintaining the NT file system on Linux.

You are welcome. Thanks for your contribution. (-:

Best regards,

         Anton


-- 
   "I've not lost my mind. It's backed up on tape somewhere." - Unknown
-- 
Anton Altaparmakov <aia21 at cantab.net> (replace at with @)
Linux NTFS Maintainer / IRC: #ntfs on irc.openprojects.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/