Remove some BUG_ONs

PATCH In review David Sterba 3 participants 8 messages Feb 20, 2017 – Feb 28, 2017 Btrfs file system

Share & tools

David Sterba

author maintainer

2017-02-20 18:25 UTC #0 lore raw

cover Remove some BUG_ONs

Minor cleanup in the BUG_ONs, where could be either removed or replaced by proper error handling where the call chain is ready for that. For 4.11.

David Sterba (3):
btrfs: remove BUG_ON from __tree_mod_log_insert
btrfs: handle allocation error in update_dev_stat_item
btrfs: do proper error handling in btrfs_insert_xattr_item

 fs/btrfs/ctree.c    | 2 --
 fs/btrfs/dir-item.c | 3 ++-
 fs/btrfs/volumes.c  | 3 ++-
 3 files changed, 4 insertions(+), 4 deletions(-)

David Sterba

author maintainer

2017-02-20 18:25 UTC #1 lore raw

PATCH 1/3 btrfs: remove BUG_ON from __tree_mod_log_insert

All callers dereference the 'tm' parameter before it gets to this function, the NULL check does not make much sense here.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/ctree.c | 2 --
 1 file changed, 2 deletions(-)

fs/btrfs/ctree.c +0 -2

index 1192bc7d2ee7..2c3c943bfcdc 100644
@@ -453,8 +453,6 @@ __tree_mod_log_insert(struct btrfs_fs_info *fs_info, struct tree_mod_elem *tm)
 	struct rb_node *parent = NULL;
 	struct tree_mod_elem *cur;
 
-	BUG_ON(!tm);
-
 	tm->seq = btrfs_inc_tree_mod_seq(fs_info);
 
 	tm_root = &fs_info->tree_mod_log;

Signature · 1 line

2.10.1

Liu Bo

Reviewed-by on v1 1/3

2017-02-22 05:31 UTC #2 lore raw

Re: [PATCH 1/3] btrfs: remove BUG_ON from __tree_mod_log_insert

Quoted 2 lines · David Sterba

All callers dereference the 'tm' parameter before it gets to this function, the NULL check does not make much sense here.

Reviewed-by Liu Bo <[email protected]>

Thanks,

-liubo

Quoted 26 lines

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/ctree.c | 2 --
 1 file changed, 2 deletions(-)

fs/btrfs/ctree.c +0 -2

index 1192bc7d2ee7..2c3c943bfcdc 100644
@@ -453,8 +453,6 @@ __tree_mod_log_insert(struct btrfs_fs_info *fs_info, struct tree_mod_elem *tm)
 	struct rb_node *parent = NULL;
 	struct tree_mod_elem *cur;
 
-	BUG_ON(!tm);
-
 	tm->seq = btrfs_inc_tree_mod_seq(fs_info);
 
 	tm_root = &fs_info->tree_mod_log;

Signature · 6 lines

2.10.1

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

David Sterba

author maintainer

2017-02-20 18:25 UTC #3 lore raw

PATCH 2/3 btrfs: handle allocation error in update_dev_stat_item

In reply to David Sterba

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/volumes.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

fs/btrfs/volumes.c +2 -1

index 1fac98728814..64d6665f6eda 100644
@@ -6954,7 +6954,8 @@ static int update_dev_stat_item(struct btrfs_trans_handle *trans,
 	key.offset = device->devid;
 
 	path = btrfs_alloc_path();
-	BUG_ON(!path);
+	if (!path)
+		return -ENOMEM;
 	ret = btrfs_search_slot(trans, dev_root, &key, path, -1, 1);
 	if (ret < 0) {
 		btrfs_warn_in_rcu(fs_info,

Signature · 1 line

2.10.1

Liu Bo

Reviewed-by on v1 2/3

2017-02-22 05:33 UTC #4 lore raw

Re: [PATCH 2/3] btrfs: handle allocation error in update_dev_stat_item

Quoted 10 lines · David Sterba

Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/volumes.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
fs/btrfs/volumes.c +0 -0
index 1fac98728814..64d6665f6eda 100644

fs/btrfs/volumes.c static int update_dev_stat_item(struct btrfs_trans_handle *trans, · line 6961

Quoted 9 lines

fs/btrfs/volumes.c +2 -1

@@ -6954,7 +6954,8 @@ static int update_dev_stat_item(struct btrfs_trans_handle *trans,
 	key.offset = device->devid;
 
 	path = btrfs_alloc_path();
-	BUG_ON(!path);
+	if (!path)
+		return -ENOMEM;
 	ret = btrfs_search_slot(trans, dev_root, &key, path, -1, 1);
 	if (ret < 0) {
 		btrfs_warn_in_rcu(fs_info,

Reviewed-by Liu Bo <[email protected]>

Thanks,

-liubo

Quoted 7 lines

Signature · 6 lines

2.10.1

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

David Sterba

author maintainer

2017-02-20 18:25 UTC #5 lore raw

PATCH 3/3 btrfs: do proper error handling in btrfs_insert_xattr_item

In reply to David Sterba

The space check in btrfs_insert_xattr_item is duplicated in it's caller (do_setxattr) so we won't hit the BUG_ON. Continuing without any check could be disasterous so turn it to a proper error handling.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/dir-item.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

fs/btrfs/dir-item.c +2 -1

index 724504a2d7ac..640801082533 100644
@@ -80,7 +80,8 @@ int btrfs_insert_xattr_item(struct btrfs_trans_handle *trans,
 	struct extent_buffer *leaf;
 	u32 data_size;
 
-	BUG_ON(name_len + data_len > BTRFS_MAX_XATTR_SIZE(root->fs_info));
+	if (name_len + data_len > BTRFS_MAX_XATTR_SIZE(root->fs_info))
+		return -ENOSPC;
 
 	key.objectid = objectid;
 	key.type = BTRFS_XATTR_ITEM_KEY;

Signature · 1 line

2.10.1

Liu Bo

Reviewed-by on v1 3/3

2017-02-22 05:39 UTC #6 lore raw

Re: [PATCH 3/3] btrfs: do proper error handling in btrfs_insert_xattr_item

Quoted 14 lines · David Sterba

The space check in btrfs_insert_xattr_item is duplicated in it's caller (do_setxattr) so we won't hit the BUG_ON. Continuing without any check could be disasterous so turn it to a proper error handling.
Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/dir-item.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
fs/btrfs/dir-item.c +0 -0
index 724504a2d7ac..640801082533 100644

fs/btrfs/dir-item.c int btrfs_insert_xattr_item(struct btrfs_trans_handle *trans, · line 85

Quoted 7 lines

fs/btrfs/dir-item.c +2 -1

@@ -80,7 +80,8 @@ int btrfs_insert_xattr_item(struct btrfs_trans_handle *trans,
 	struct extent_buffer *leaf;
 	u32 data_size;
 
-	BUG_ON(name_len + data_len > BTRFS_MAX_XATTR_SIZE(root->fs_info));
+	if (name_len + data_len > BTRFS_MAX_XATTR_SIZE(root->fs_info))
+		return -ENOSPC;

Besides making it silent, how about adding a ASSERT to cry out? (Although currently we'd never come into this case.)

Reviewed-by Liu Bo <[email protected]>

Thanks,

-liubo

Quoted 9 lines

fs/btrfs/dir-item.c +0 -0

 	key.objectid = objectid;
 	key.type = BTRFS_XATTR_ITEM_KEY;

Signature · 6 lines

2.10.1

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

David Sterba

2017-02-28 10:44 UTC #7 lore raw

Re: [PATCH 3/3] btrfs: do proper error handling in btrfs_insert_xattr_item

Quoted 18 lines · Liu Bo

Quoted 17 lines · David Sterba
The space check in btrfs_insert_xattr_item is duplicated in it's caller (do_setxattr) so we won't hit the BUG_ON. Continuing without any check could be disasterous so turn it to a proper error handling.
Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/dir-item.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
fs/btrfs/dir-item.c +0 -0
index 724504a2d7ac..640801082533 100644
@@ -80,7 +80,8 @@ int btrfs_insert_xattr_item(struct btrfs_trans_handle *trans,
 	struct extent_buffer *leaf;
 	u32 data_size;
 

Quoted 7 lines

fs/btrfs/dir-item.c int btrfs_insert_xattr_item(struct btrfs_trans_handle *trans, · line 85
Quoted 4 lines
fs/btrfs/dir-item.c +2 -1
-	BUG_ON(name_len + data_len > BTRFS_MAX_XATTR_SIZE(root->fs_info));
+	if (name_len + data_len > BTRFS_MAX_XATTR_SIZE(root->fs_info))
+		return -ENOSPC;
 
Besides making it silent, how about adding a ASSERT to cry out? (Although currently we'd never come into this case.)

I don't think we need the assert, the caller is supposed to handle the error. In this case it's validation of input parameters, that could possibly happen as the function is not static.