diff mbox

[OPW,kernel,v2] fs:btrfs: Use WARN_ON()'s return value in place of WARN_ON(1)

Message ID 20131022052829.GA18020@dshgna
State New, archived
Headers show

Commit Message

Dulshani Gunawardhana Oct. 22, 2013, 5:28 a.m. UTC
Use WARN_ON()'s return value in place of WARN_ON(1) for cleaner source
code that outputs a more descriptive warnings. Also fix the styling
warning of redundant braces that came up as a result of this fix.

Signed-off-by: Dulshani Gunawardhana <dulshani.gunawardhana89@gmail.com>
Reviewed-by: Josh Triplett <josh@joshtriplett.org>
---

Changes since v1:
*Remove redundant if-condition
*Fix WARN_ON() to refer to preceeding call instead of whole block
---
 fs/btrfs/backref.c         |  3 +--
 fs/btrfs/check-integrity.c |  7 ++-----
 fs/btrfs/ctree.c           |  6 ++----
 fs/btrfs/delayed-inode.c   |  3 +--
 fs/btrfs/disk-io.c         |  8 +-------
 fs/btrfs/extent-tree.c     | 11 ++++-------
 fs/btrfs/extent_io.c       |  7 ++-----
 fs/btrfs/inode.c           | 10 +++-------
 8 files changed, 16 insertions(+), 39 deletions(-)

Comments

Zach Brown Oct. 22, 2013, 9:44 p.m. UTC | #1
On Tue, Oct 22, 2013 at 10:58:35AM +0530, Dulshani Gunawardhana wrote:
> Use WARN_ON()'s return value in place of WARN_ON(1) for cleaner source
> code that outputs a more descriptive warnings. Also fix the styling
> warning of redundant braces that came up as a result of this fix.
> 
> Signed-off-by: Dulshani Gunawardhana <dulshani.gunawardhana89@gmail.com>
> Reviewed-by: Josh Triplett <josh@joshtriplett.org>

Looks good!

Reviewed-by: Zach Brown <zab@redhat.com>

The next step is to send this to the linux-btrfs mailing list to see it
we can get it in the btrfs-next branch : linux-btrfs@vger.kernel.org

- z
Dulshani Gunawardhana Oct. 26, 2013, 4:49 a.m. UTC | #2
Zach, Just a small doubt here.

I've noticed that certain style cleanups had prefixes such as Trivial 
Cleanup on the linux-btrfs mailing list. Should I adhere to any such 
titling standard because this is a minor fix?

Thanks :)

>
> The next step is to send this to the linux-btrfs mailing list to see it 
> we can get it in the btrfs-next branch : linux...@vger.kernel.org<javascript:> 
>
> - z 
>
Zach Brown Oct. 29, 2013, 10:26 p.m. UTC | #3
On Fri, Oct 25, 2013 at 09:49:39PM -0700, Dulshani Gunawardhana wrote:
> Zach, Just a small doubt here.
> 
> I've noticed that certain style cleanups had prefixes such as Trivial Cleanup
> on the linux-btrfs mailing list. Should I adhere to any such titling standard
> because this is a minor fix?

In general, no, I wouldn't be too eager to use a prefix like that.  But
it depends on the patch.

In the specific case of this WARN_ON series, I wouldn't use some trivial
prefix because the patch makes real code changes that people should
review.  My first review caught accidental behavioural changes in the
code.  We want others to read it as carefully in case I miss something.

If, however, it were something like removing trailing whitespace or
fixing comment misspelling, yeah, I might describe it as trivial or
minor in the subject.

It's a judgement call :/.

- z
diff mbox

Patch

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index 0552a59..408b7d1 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -323,8 +323,7 @@  static int __resolve_indirect_ref(struct btrfs_fs_info *fs_info,
 
 	eb = path->nodes[level];
 	while (!eb) {
-		if (!level) {
-			WARN_ON(1);
+		if (WARN_ON(!level)) {
 			ret = 1;
 			goto out;
 		}
diff --git a/fs/btrfs/check-integrity.c b/fs/btrfs/check-integrity.c
index 1c47be1..96d1557 100644
--- a/fs/btrfs/check-integrity.c
+++ b/fs/btrfs/check-integrity.c
@@ -2463,10 +2463,8 @@  static int btrfsic_process_written_superblock(
 		}
 	}
 
-	if (-1 == btrfsic_check_all_ref_blocks(state, superblock, 0)) {
-		WARN_ON(1);
+	if (WARN_ON(-1 == btrfsic_check_all_ref_blocks(state, superblock, 0)))
 		btrfsic_dump_tree(state);
-	}
 
 	return 0;
 }
@@ -2906,7 +2904,7 @@  static void btrfsic_cmp_log_and_dev_bytenr(struct btrfsic_state *state,
 		btrfsic_release_block_ctx(&block_ctx);
 	}
 
-	if (!match) {
+	if (WARN_ON(!match)) {
 		printk(KERN_INFO "btrfs: attempt to write M-block which contains logical bytenr that doesn't map to dev+physical bytenr of submit_bio,"
 		       " buffer->log_bytenr=%llu, submit_bio(bdev=%s,"
 		       " phys_bytenr=%llu)!\n",
@@ -2923,7 +2921,6 @@  static void btrfsic_cmp_log_and_dev_bytenr(struct btrfsic_state *state,
 			       bytenr, block_ctx.dev->name,
 			       block_ctx.dev_bytenr, mirror_num);
 		}
-		WARN_ON(1);
 	}
 }
 
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 61b5bcd..0039dde 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -1285,11 +1285,10 @@  get_old_root(struct btrfs_root *root, u64 time_seq)
 		free_extent_buffer(eb_root);
 		blocksize = btrfs_level_size(root, old_root->level);
 		old = read_tree_block(root, logical, blocksize, 0);
-		if (!old || !extent_buffer_uptodate(old)) {
+		if (WARN_ON(!old || !extent_buffer_uptodate(old))) {
 			free_extent_buffer(old);
 			pr_warn("btrfs: failed to read tree block %llu from get_old_root\n",
 				logical);
-			WARN_ON(1);
 		} else {
 			eb = btrfs_clone_extent_buffer(old);
 			free_extent_buffer(old);
@@ -3639,8 +3638,7 @@  static noinline int __push_leaf_left(struct btrfs_trans_handle *trans,
 		ret = 1;
 		goto out;
 	}
-	if (!empty && push_items == btrfs_header_nritems(right))
-		WARN_ON(1);
+	WARN_ON(!empty && push_items == btrfs_header_nritems(right));
 
 	/* push data from right to left */
 	copy_extent_buffer(left, right,
diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index cbd9523..207e2d0 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -649,14 +649,13 @@  static int btrfs_delayed_inode_reserve_metadata(
 			goto out;
 
 		ret = btrfs_block_rsv_migrate(src_rsv, dst_rsv, num_bytes);
-		if (!ret)
+		if (!WARN_ON(ret))
 			goto out;
 
 		/*
 		 * Ok this is a problem, let's just steal from the global rsv
 		 * since this really shouldn't happen that often.
 		 */
-		WARN_ON(1);
 		ret = btrfs_block_rsv_migrate(&root->fs_info->global_block_rsv,
 					      dst_rsv, num_bytes);
 		goto out;
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 62176ad..1979122 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -477,14 +477,8 @@  static int csum_dirty_buffer(struct btrfs_root *root, struct page *page)
 	if (page != eb->pages[0])
 		return 0;
 	found_start = btrfs_header_bytenr(eb);
-	if (found_start != start) {
-		WARN_ON(1);
+	if (WARN_ON(found_start != start || !PageUptodate(page)))
 		return 0;
-	}
-	if (!PageUptodate(page)) {
-		WARN_ON(1);
-		return 0;
-	}
 	csum_tree_block(root, eb, 0);
 	return 0;
 }
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index d58bef1..c135f1c 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -1551,9 +1551,8 @@  again:
 	if (ret && !insert) {
 		err = -ENOENT;
 		goto out;
-	} else if (ret) {
+	} else if (WARN_ON(ret)) {
 		err = -EIO;
-		WARN_ON(1);
 		goto out;
 	}
 
@@ -5718,9 +5717,8 @@  static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
 			}
 			extent_slot = path->slots[0];
 		}
-	} else if (ret == -ENOENT) {
+	} else if (WARN_ON(ret == -ENOENT)) {
 		btrfs_print_leaf(extent_root, path->nodes[0]);
-		WARN_ON(1);
 		btrfs_err(info,
 			"unable to find ref byte nr %llu parent %llu root %llu  owner %llu offset %llu",
 			bytenr, parent, root_objectid, owner_objectid,
@@ -8276,10 +8274,9 @@  int btrfs_free_block_groups(struct btrfs_fs_info *info)
 					struct btrfs_space_info,
 					list);
 		if (btrfs_test_opt(info->tree_root, ENOSPC_DEBUG)) {
-			if (space_info->bytes_pinned > 0 ||
+			if (WARN_ON(space_info->bytes_pinned > 0 ||
 			    space_info->bytes_reserved > 0 ||
-			    space_info->bytes_may_use > 0) {
-				WARN_ON(1);
+			    space_info->bytes_may_use > 0)) {
 				dump_space_info(space_info, 0, 0);
 			}
 		}
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 51731b7..a4592f4 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -1740,10 +1740,8 @@  u64 count_range_bits(struct extent_io_tree *tree,
 	u64 last = 0;
 	int found = 0;
 
-	if (search_end <= cur_start) {
-		WARN_ON(1);
+	if (WARN_ON(search_end <= cur_start))
 		return 0;
-	}
 
 	spin_lock(&tree->lock);
 	if (cur_start == 0 && bits == EXTENT_DIRTY) {
@@ -3569,9 +3567,8 @@  retry:
 			 * but no sense in crashing the users box for something
 			 * we can survive anyway.
 			 */
-			if (!eb) {
+			if (WARN_ON(!eb)) {
 				spin_unlock(&mapping->private_lock);
-				WARN_ON(1);
 				continue;
 			}
 
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 51e3afa..c3fa038 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2041,10 +2041,8 @@  static noinline int record_one_backref(u64 inum, u64 offset, u64 root_id,
 		key.offset = offset;
 
 	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
-	if (ret < 0) {
-		WARN_ON(1);
+	if (WARN_ON(ret < 0))
 		return ret;
-	}
 	ret = 0;
 
 	while (1) {
@@ -3172,8 +3170,7 @@  int btrfs_orphan_cleanup(struct btrfs_root *root)
 
 		/* if we have links, this was a truncate, lets do that */
 		if (inode->i_nlink) {
-			if (!S_ISREG(inode->i_mode)) {
-				WARN_ON(1);
+			if (WARN_ON(!S_ISREG(inode->i_mode))) {
 				iput(inode);
 				continue;
 			}
@@ -7995,8 +7992,7 @@  static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 		if (ret == -EEXIST) {
 			/* we shouldn't get
 			 * eexist without a new_inode */
-			if (!new_inode) {
-				WARN_ON(1);
+			if (WARN_ON(!new_inode)) {
 				return ret;
 			}
 		} else {