* [PATCH 1/5] btrfs: Use common error handling code in tree_mod_log_eb_copy()
2017-08-21 12:35 [PATCH 0/5] BTRFS: Fine-tuning for five function implementations SF Markus Elfring
@ 2017-08-21 12:37 ` SF Markus Elfring
2017-08-21 12:38 ` [PATCH 2/5] btrfs: Use common error handling code in __btrfs_free_extent() SF Markus Elfring
` (4 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: SF Markus Elfring @ 2017-08-21 12:37 UTC (permalink / raw)
To: linux-btrfs, Chris Mason, David Sterba, Josef Bacik; +Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 21 Aug 2017 09:36:48 +0200
Add a jump target so that a bit of exception handling can be better reused
in this function.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
fs/btrfs/ctree.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 6d49db7d86be..d29cf946ebf9 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -823,17 +823,13 @@ tree_mod_log_eb_copy(struct btrfs_fs_info *fs_info, struct extent_buffer *dst,
for (i = 0; i < nr_items; i++) {
tm_list_rem[i] = alloc_tree_mod_elem(src, i + src_offset,
MOD_LOG_KEY_REMOVE, GFP_NOFS);
- if (!tm_list_rem[i]) {
- ret = -ENOMEM;
- goto free_tms;
- }
+ if (!tm_list_rem[i])
+ goto e_nomem;
tm_list_add[i] = alloc_tree_mod_elem(dst, i + dst_offset,
MOD_LOG_KEY_ADD, GFP_NOFS);
- if (!tm_list_add[i]) {
- ret = -ENOMEM;
- goto free_tms;
- }
+ if (!tm_list_add[i])
+ goto e_nomem;
}
if (tree_mod_dont_log(fs_info, NULL))
@@ -854,6 +850,8 @@ tree_mod_log_eb_copy(struct btrfs_fs_info *fs_info, struct extent_buffer *dst,
return 0;
+e_nomem:
+ ret = -ENOMEM;
free_tms:
for (i = 0; i < nr_items * 2; i++) {
if (tm_list[i] && !RB_EMPTY_NODE(&tm_list[i]->node))
--
2.14.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 2/5] btrfs: Use common error handling code in __btrfs_free_extent()
2017-08-21 12:35 [PATCH 0/5] BTRFS: Fine-tuning for five function implementations SF Markus Elfring
2017-08-21 12:37 ` [PATCH 1/5] btrfs: Use common error handling code in tree_mod_log_eb_copy() SF Markus Elfring
@ 2017-08-21 12:38 ` SF Markus Elfring
2017-08-21 13:07 ` Jeff Mahoney
2017-08-21 12:40 ` [PATCH 3/5] btrfs: Use common error handling code in btrfs_update_root() SF Markus Elfring
` (3 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: SF Markus Elfring @ 2017-08-21 12:38 UTC (permalink / raw)
To: linux-btrfs, Chris Mason, David Sterba, Josef Bacik; +Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 21 Aug 2017 10:03:00 +0200
Add a jump target so that a bit of exception handling can be better reused
at the end of this function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
fs/btrfs/extent-tree.c | 69 ++++++++++++++++++++------------------------------
1 file changed, 27 insertions(+), 42 deletions(-)
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 116c5615d6c2..c6b7aca88491 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -6913,10 +6913,9 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
ret = remove_extent_backref(trans, info, path, NULL,
refs_to_drop,
is_data, &last_ref);
- if (ret) {
- btrfs_abort_transaction(trans, ret);
- goto out;
- }
+ if (ret)
+ goto abort_transaction;
+
btrfs_release_path(path);
path->leave_spinning = 1;
@@ -6962,10 +6961,9 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
if (ret > 0)
btrfs_print_leaf(path->nodes[0]);
}
- if (ret < 0) {
- btrfs_abort_transaction(trans, ret);
- goto out;
- }
+ if (ret < 0)
+ goto abort_transaction;
+
extent_slot = path->slots[0];
}
} else if (WARN_ON(ret == -ENOENT)) {
@@ -6974,11 +6972,9 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
"unable to find ref byte nr %llu parent %llu root %llu owner %llu offset %llu",
bytenr, parent, root_objectid, owner_objectid,
owner_offset);
- btrfs_abort_transaction(trans, ret);
- goto out;
+ goto abort_transaction;
} else {
- btrfs_abort_transaction(trans, ret);
- goto out;
+ goto abort_transaction;
}
leaf = path->nodes[0];
@@ -6988,10 +6984,8 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
BUG_ON(found_extent || extent_slot != path->slots[0]);
ret = convert_extent_item_v0(trans, info, path, owner_objectid,
0);
- if (ret < 0) {
- btrfs_abort_transaction(trans, ret);
- goto out;
- }
+ if (ret < 0)
+ goto abort_transaction;
btrfs_release_path(path);
path->leave_spinning = 1;
@@ -7008,10 +7002,8 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
ret, bytenr);
btrfs_print_leaf(path->nodes[0]);
}
- if (ret < 0) {
- btrfs_abort_transaction(trans, ret);
- goto out;
- }
+ if (ret < 0)
+ goto abort_transaction;
extent_slot = path->slots[0];
leaf = path->nodes[0];
@@ -7035,8 +7027,7 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
"trying to drop %d refs but we only have %Lu for bytenr %Lu",
refs_to_drop, refs, bytenr);
ret = -EINVAL;
- btrfs_abort_transaction(trans, ret);
- goto out;
+ goto abort_transaction;
}
refs -= refs_to_drop;
@@ -7057,10 +7048,8 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
ret = remove_extent_backref(trans, info, path,
iref, refs_to_drop,
is_data, &last_ref);
- if (ret) {
- btrfs_abort_transaction(trans, ret);
- goto out;
- }
+ if (ret)
+ goto abort_transaction;
}
} else {
if (found_extent) {
@@ -7078,37 +7067,33 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
last_ref = 1;
ret = btrfs_del_items(trans, extent_root, path, path->slots[0],
num_to_del);
- if (ret) {
- btrfs_abort_transaction(trans, ret);
- goto out;
- }
+ if (ret)
+ goto abort_transaction;
+
btrfs_release_path(path);
if (is_data) {
ret = btrfs_del_csums(trans, info, bytenr, num_bytes);
- if (ret) {
- btrfs_abort_transaction(trans, ret);
- goto out;
- }
+ if (ret)
+ goto abort_transaction;
}
ret = add_to_free_space_tree(trans, info, bytenr, num_bytes);
- if (ret) {
- btrfs_abort_transaction(trans, ret);
- goto out;
- }
+ if (ret)
+ goto abort_transaction;
ret = update_block_group(trans, info, bytenr, num_bytes, 0);
- if (ret) {
- btrfs_abort_transaction(trans, ret);
- goto out;
- }
+ if (ret)
+ goto abort_transaction;
}
btrfs_release_path(path);
out:
btrfs_free_path(path);
return ret;
+abort_transaction:
+ btrfs_abort_transaction(trans, ret);
+ goto out;
}
/*
--
2.14.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 2/5] btrfs: Use common error handling code in __btrfs_free_extent()
2017-08-21 12:38 ` [PATCH 2/5] btrfs: Use common error handling code in __btrfs_free_extent() SF Markus Elfring
@ 2017-08-21 13:07 ` Jeff Mahoney
2017-08-21 13:14 ` Dan Carpenter
0 siblings, 1 reply; 14+ messages in thread
From: Jeff Mahoney @ 2017-08-21 13:07 UTC (permalink / raw)
To: SF Markus Elfring, linux-btrfs, Chris Mason, David Sterba,
Josef Bacik
Cc: LKML, kernel-janitors
[-- Attachment #1.1: Type: text/plain, Size: 4979 bytes --]
On 8/21/17 8:38 AM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 21 Aug 2017 10:03:00 +0200
>
> Add a jump target so that a bit of exception handling can be better reused
> at the end of this function.
>
> This issue was detected by using the Coccinelle software.
btrfs_abort_transaction dumps __FILE__:__LINE__ in the log so this patch
makes the code more difficult to debug.
-Jeff
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> fs/btrfs/extent-tree.c | 69 ++++++++++++++++++++------------------------------
> 1 file changed, 27 insertions(+), 42 deletions(-)
>
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 116c5615d6c2..c6b7aca88491 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -6913,10 +6913,9 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
> ret = remove_extent_backref(trans, info, path, NULL,
> refs_to_drop,
> is_data, &last_ref);
> - if (ret) {
> - btrfs_abort_transaction(trans, ret);
> - goto out;
> - }
> + if (ret)
> + goto abort_transaction;
> +
> btrfs_release_path(path);
> path->leave_spinning = 1;
>
> @@ -6962,10 +6961,9 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
> if (ret > 0)
> btrfs_print_leaf(path->nodes[0]);
> }
> - if (ret < 0) {
> - btrfs_abort_transaction(trans, ret);
> - goto out;
> - }
> + if (ret < 0)
> + goto abort_transaction;
> +
> extent_slot = path->slots[0];
> }
> } else if (WARN_ON(ret == -ENOENT)) {
> @@ -6974,11 +6972,9 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
> "unable to find ref byte nr %llu parent %llu root %llu owner %llu offset %llu",
> bytenr, parent, root_objectid, owner_objectid,
> owner_offset);
> - btrfs_abort_transaction(trans, ret);
> - goto out;
> + goto abort_transaction;
> } else {
> - btrfs_abort_transaction(trans, ret);
> - goto out;
> + goto abort_transaction;
> }
>
> leaf = path->nodes[0];
> @@ -6988,10 +6984,8 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
> BUG_ON(found_extent || extent_slot != path->slots[0]);
> ret = convert_extent_item_v0(trans, info, path, owner_objectid,
> 0);
> - if (ret < 0) {
> - btrfs_abort_transaction(trans, ret);
> - goto out;
> - }
> + if (ret < 0)
> + goto abort_transaction;
>
> btrfs_release_path(path);
> path->leave_spinning = 1;
> @@ -7008,10 +7002,8 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
> ret, bytenr);
> btrfs_print_leaf(path->nodes[0]);
> }
> - if (ret < 0) {
> - btrfs_abort_transaction(trans, ret);
> - goto out;
> - }
> + if (ret < 0)
> + goto abort_transaction;
>
> extent_slot = path->slots[0];
> leaf = path->nodes[0];
> @@ -7035,8 +7027,7 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
> "trying to drop %d refs but we only have %Lu for bytenr %Lu",
> refs_to_drop, refs, bytenr);
> ret = -EINVAL;
> - btrfs_abort_transaction(trans, ret);
> - goto out;
> + goto abort_transaction;
> }
> refs -= refs_to_drop;
>
> @@ -7057,10 +7048,8 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
> ret = remove_extent_backref(trans, info, path,
> iref, refs_to_drop,
> is_data, &last_ref);
> - if (ret) {
> - btrfs_abort_transaction(trans, ret);
> - goto out;
> - }
> + if (ret)
> + goto abort_transaction;
> }
> } else {
> if (found_extent) {
> @@ -7078,37 +7067,33 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
> last_ref = 1;
> ret = btrfs_del_items(trans, extent_root, path, path->slots[0],
> num_to_del);
> - if (ret) {
> - btrfs_abort_transaction(trans, ret);
> - goto out;
> - }
> + if (ret)
> + goto abort_transaction;
> +
> btrfs_release_path(path);
>
> if (is_data) {
> ret = btrfs_del_csums(trans, info, bytenr, num_bytes);
> - if (ret) {
> - btrfs_abort_transaction(trans, ret);
> - goto out;
> - }
> + if (ret)
> + goto abort_transaction;
> }
>
> ret = add_to_free_space_tree(trans, info, bytenr, num_bytes);
> - if (ret) {
> - btrfs_abort_transaction(trans, ret);
> - goto out;
> - }
> + if (ret)
> + goto abort_transaction;
>
> ret = update_block_group(trans, info, bytenr, num_bytes, 0);
> - if (ret) {
> - btrfs_abort_transaction(trans, ret);
> - goto out;
> - }
> + if (ret)
> + goto abort_transaction;
> }
> btrfs_release_path(path);
>
> out:
> btrfs_free_path(path);
> return ret;
> +abort_transaction:
> + btrfs_abort_transaction(trans, ret);
> + goto out;
> }
>
> /*
>
--
Jeff Mahoney
SUSE Labs
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 2/5] btrfs: Use common error handling code in __btrfs_free_extent()
2017-08-21 13:07 ` Jeff Mahoney
@ 2017-08-21 13:14 ` Dan Carpenter
0 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2017-08-21 13:14 UTC (permalink / raw)
To: Jeff Mahoney
Cc: SF Markus Elfring, linux-btrfs, Chris Mason, David Sterba,
Josef Bacik, LKML, kernel-janitors
On Mon, Aug 21, 2017 at 09:07:47AM -0400, Jeff Mahoney wrote:
> On 8/21/17 8:38 AM, SF Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Mon, 21 Aug 2017 10:03:00 +0200
> >
> > Add a jump target so that a bit of exception handling can be better reused
> > at the end of this function.
> >
> > This issue was detected by using the Coccinelle software.
>
> btrfs_abort_transaction dumps __FILE__:__LINE__ in the log so this patch
> makes the code more difficult to debug.
>
I was just reviewing this and I missed that issue. These patches are
just exhausting...
regards,
dan carpenter
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/5] btrfs: Use common error handling code in btrfs_update_root()
2017-08-21 12:35 [PATCH 0/5] BTRFS: Fine-tuning for five function implementations SF Markus Elfring
2017-08-21 12:37 ` [PATCH 1/5] btrfs: Use common error handling code in tree_mod_log_eb_copy() SF Markus Elfring
2017-08-21 12:38 ` [PATCH 2/5] btrfs: Use common error handling code in __btrfs_free_extent() SF Markus Elfring
@ 2017-08-21 12:40 ` SF Markus Elfring
2017-08-21 13:09 ` Jeff Mahoney
2017-08-21 12:41 ` [PATCH 4/5] btrfs: Use common error handling code in update_ref_path() SF Markus Elfring
` (2 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: SF Markus Elfring @ 2017-08-21 12:40 UTC (permalink / raw)
To: linux-btrfs, Chris Mason, David Sterba, Josef Bacik; +Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 21 Aug 2017 13:10:15 +0200
Add a jump target so that a bit of exception handling can be better reused
at the end of this function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
fs/btrfs/root-tree.c | 27 +++++++++++----------------
1 file changed, 11 insertions(+), 16 deletions(-)
diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
index 5b488af6f25e..bc497ba9d9d1 100644
--- a/fs/btrfs/root-tree.c
+++ b/fs/btrfs/root-tree.c
@@ -145,10 +145,8 @@ int btrfs_update_root(struct btrfs_trans_handle *trans, struct btrfs_root
return -ENOMEM;
ret = btrfs_search_slot(trans, root, key, path, 0, 1);
- if (ret < 0) {
- btrfs_abort_transaction(trans, ret);
- goto out;
- }
+ if (ret < 0)
+ goto abort_transaction;
if (ret != 0) {
btrfs_print_leaf(path->nodes[0]);
@@ -171,23 +169,17 @@ int btrfs_update_root(struct btrfs_trans_handle *trans, struct btrfs_root
btrfs_release_path(path);
ret = btrfs_search_slot(trans, root, key, path,
-1, 1);
- if (ret < 0) {
- btrfs_abort_transaction(trans, ret);
- goto out;
- }
+ if (ret < 0)
+ goto abort_transaction;
ret = btrfs_del_item(trans, root, path);
- if (ret < 0) {
- btrfs_abort_transaction(trans, ret);
- goto out;
- }
+ if (ret < 0)
+ goto abort_transaction;
btrfs_release_path(path);
ret = btrfs_insert_empty_item(trans, root, path,
key, sizeof(*item));
- if (ret < 0) {
- btrfs_abort_transaction(trans, ret);
- goto out;
- }
+ if (ret < 0)
+ goto abort_transaction;
l = path->nodes[0];
slot = path->slots[0];
ptr = btrfs_item_ptr_offset(l, slot);
@@ -204,6 +196,9 @@ int btrfs_update_root(struct btrfs_trans_handle *trans, struct btrfs_root
out:
btrfs_free_path(path);
return ret;
+abort_transaction:
+ btrfs_abort_transaction(trans, ret);
+ goto out;
}
int btrfs_insert_root(struct btrfs_trans_handle *trans, struct btrfs_root *root,
--
2.14.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 3/5] btrfs: Use common error handling code in btrfs_update_root()
2017-08-21 12:40 ` [PATCH 3/5] btrfs: Use common error handling code in btrfs_update_root() SF Markus Elfring
@ 2017-08-21 13:09 ` Jeff Mahoney
0 siblings, 0 replies; 14+ messages in thread
From: Jeff Mahoney @ 2017-08-21 13:09 UTC (permalink / raw)
To: SF Markus Elfring, linux-btrfs, Chris Mason, David Sterba,
Josef Bacik
Cc: LKML, kernel-janitors
[-- Attachment #1.1: Type: text/plain, Size: 2441 bytes --]
On 8/21/17 8:40 AM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 21 Aug 2017 13:10:15 +0200
>
> Add a jump target so that a bit of exception handling can be better reused
> at the end of this function.
>
> This issue was detected by using the Coccinelle software.
btrfs_abort_transaction dumps __FILE__:__LINE__ in the log so this patch
makes the code more difficult to debug.
-Jeff
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> fs/btrfs/root-tree.c | 27 +++++++++++----------------
> 1 file changed, 11 insertions(+), 16 deletions(-)
>
> diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
> index 5b488af6f25e..bc497ba9d9d1 100644
> --- a/fs/btrfs/root-tree.c
> +++ b/fs/btrfs/root-tree.c
> @@ -145,10 +145,8 @@ int btrfs_update_root(struct btrfs_trans_handle *trans, struct btrfs_root
> return -ENOMEM;
>
> ret = btrfs_search_slot(trans, root, key, path, 0, 1);
> - if (ret < 0) {
> - btrfs_abort_transaction(trans, ret);
> - goto out;
> - }
> + if (ret < 0)
> + goto abort_transaction;
>
> if (ret != 0) {
> btrfs_print_leaf(path->nodes[0]);
> @@ -171,23 +169,17 @@ int btrfs_update_root(struct btrfs_trans_handle *trans, struct btrfs_root
> btrfs_release_path(path);
> ret = btrfs_search_slot(trans, root, key, path,
> -1, 1);
> - if (ret < 0) {
> - btrfs_abort_transaction(trans, ret);
> - goto out;
> - }
> + if (ret < 0)
> + goto abort_transaction;
>
> ret = btrfs_del_item(trans, root, path);
> - if (ret < 0) {
> - btrfs_abort_transaction(trans, ret);
> - goto out;
> - }
> + if (ret < 0)
> + goto abort_transaction;
> btrfs_release_path(path);
> ret = btrfs_insert_empty_item(trans, root, path,
> key, sizeof(*item));
> - if (ret < 0) {
> - btrfs_abort_transaction(trans, ret);
> - goto out;
> - }
> + if (ret < 0)
> + goto abort_transaction;
> l = path->nodes[0];
> slot = path->slots[0];
> ptr = btrfs_item_ptr_offset(l, slot);
> @@ -204,6 +196,9 @@ int btrfs_update_root(struct btrfs_trans_handle *trans, struct btrfs_root
> out:
> btrfs_free_path(path);
> return ret;
> +abort_transaction:
> + btrfs_abort_transaction(trans, ret);
> + goto out;
> }
>
> int btrfs_insert_root(struct btrfs_trans_handle *trans, struct btrfs_root *root,
>
--
Jeff Mahoney
SUSE Labs
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 4/5] btrfs: Use common error handling code in update_ref_path()
2017-08-21 12:35 [PATCH 0/5] BTRFS: Fine-tuning for five function implementations SF Markus Elfring
` (2 preceding siblings ...)
2017-08-21 12:40 ` [PATCH 3/5] btrfs: Use common error handling code in btrfs_update_root() SF Markus Elfring
@ 2017-08-21 12:41 ` SF Markus Elfring
2017-08-21 13:08 ` Jeff Mahoney
2017-08-21 12:42 ` [PATCH 5/5] btrfs: Use common error handling code in btrfs_mark_extent_written() SF Markus Elfring
2017-08-21 13:24 ` [PATCH 0/5] BTRFS: Fine-tuning for five function implementations David Sterba
5 siblings, 1 reply; 14+ messages in thread
From: SF Markus Elfring @ 2017-08-21 12:41 UTC (permalink / raw)
To: linux-btrfs, Chris Mason, David Sterba, Josef Bacik; +Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 21 Aug 2017 13:34:29 +0200
Add a jump target so that a bit of exception handling can be better reused
in this function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
fs/btrfs/send.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 59fb1ed6ca20..a96edc91a101 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -3697,12 +3697,12 @@ static int update_ref_path(struct send_ctx *sctx, struct recorded_ref *ref)
return -ENOMEM;
ret = get_cur_path(sctx, ref->dir, ref->dir_gen, new_path);
- if (ret < 0) {
- fs_path_free(new_path);
- return ret;
- }
+ if (ret < 0)
+ goto free_path;
+
ret = fs_path_add(new_path, ref->name, ref->name_len);
if (ret < 0) {
+free_path:
fs_path_free(new_path);
return ret;
}
--
2.14.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 4/5] btrfs: Use common error handling code in update_ref_path()
2017-08-21 12:41 ` [PATCH 4/5] btrfs: Use common error handling code in update_ref_path() SF Markus Elfring
@ 2017-08-21 13:08 ` Jeff Mahoney
2017-08-21 13:42 ` Dan Carpenter
0 siblings, 1 reply; 14+ messages in thread
From: Jeff Mahoney @ 2017-08-21 13:08 UTC (permalink / raw)
To: SF Markus Elfring, linux-btrfs, Chris Mason, David Sterba,
Josef Bacik
Cc: LKML, kernel-janitors
[-- Attachment #1.1: Type: text/plain, Size: 1259 bytes --]
On 8/21/17 8:41 AM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 21 Aug 2017 13:34:29 +0200
>
> Add a jump target so that a bit of exception handling can be better reused
> in this function.
>
> This issue was detected by using the Coccinelle software.
Adding a jump label in the middle of a conditional for "common" error
handling makes the code more difficult to understand.
-Jeff
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> fs/btrfs/send.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index 59fb1ed6ca20..a96edc91a101 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -3697,12 +3697,12 @@ static int update_ref_path(struct send_ctx *sctx, struct recorded_ref *ref)
> return -ENOMEM;
>
> ret = get_cur_path(sctx, ref->dir, ref->dir_gen, new_path);
> - if (ret < 0) {
> - fs_path_free(new_path);
> - return ret;
> - }
> + if (ret < 0)
> + goto free_path;
> +
> ret = fs_path_add(new_path, ref->name, ref->name_len);
> if (ret < 0) {
> +free_path:
> fs_path_free(new_path);
> return ret;
> }
>
--
Jeff Mahoney
SUSE Labs
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 4/5] btrfs: Use common error handling code in update_ref_path()
2017-08-21 13:08 ` Jeff Mahoney
@ 2017-08-21 13:42 ` Dan Carpenter
2017-08-21 13:52 ` [PATCH v2] " SF Markus Elfring
0 siblings, 1 reply; 14+ messages in thread
From: Dan Carpenter @ 2017-08-21 13:42 UTC (permalink / raw)
To: Jeff Mahoney
Cc: SF Markus Elfring, linux-btrfs, Chris Mason, David Sterba,
Josef Bacik, LKML, kernel-janitors
On Mon, Aug 21, 2017 at 09:08:04AM -0400, Jeff Mahoney wrote:
> On 8/21/17 8:41 AM, SF Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Mon, 21 Aug 2017 13:34:29 +0200
> >
> > Add a jump target so that a bit of exception handling can be better reused
> > in this function.
> >
> > This issue was detected by using the Coccinelle software.
>
> Adding a jump label in the middle of a conditional for "common" error
> handling makes the code more difficult to understand.
>
I have said that a bunch of times. It's like bashing my face into the
keyboard for all the good it does. On the other hand, some people
accept these oddly placed labels... No one else writes code like this
so far as I know.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2] btrfs: Use common error handling code in update_ref_path()
2017-08-21 13:42 ` Dan Carpenter
@ 2017-08-21 13:52 ` SF Markus Elfring
0 siblings, 0 replies; 14+ messages in thread
From: SF Markus Elfring @ 2017-08-21 13:52 UTC (permalink / raw)
To: linux-btrfs, Chris Mason, Dan Carpenter, David Sterba,
Jeff Mahoney, Josef Bacik
Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 21 Aug 2017 15:45:23 +0200
Add a jump target so that a bit of exception handling can be better reused
at the end of this function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
v2:
Do you find this refactoring acceptable instead?
fs/btrfs/send.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 59fb1ed6ca20..527a9a735664 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -3697,20 +3697,20 @@ static int update_ref_path(struct send_ctx *sctx, struct recorded_ref *ref)
return -ENOMEM;
ret = get_cur_path(sctx, ref->dir, ref->dir_gen, new_path);
- if (ret < 0) {
- fs_path_free(new_path);
- return ret;
- }
+ if (ret < 0)
+ goto free_path;
+
ret = fs_path_add(new_path, ref->name, ref->name_len);
- if (ret < 0) {
- fs_path_free(new_path);
- return ret;
- }
+ if (ret < 0)
+ goto free_path;
fs_path_free(ref->full_path);
set_ref_path(ref, new_path);
return 0;
+free_path:
+ fs_path_free(new_path);
+ return ret;
}
/*
--
2.14.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 5/5] btrfs: Use common error handling code in btrfs_mark_extent_written()
2017-08-21 12:35 [PATCH 0/5] BTRFS: Fine-tuning for five function implementations SF Markus Elfring
` (3 preceding siblings ...)
2017-08-21 12:41 ` [PATCH 4/5] btrfs: Use common error handling code in update_ref_path() SF Markus Elfring
@ 2017-08-21 12:42 ` SF Markus Elfring
2017-08-21 13:07 ` Jeff Mahoney
2017-08-21 13:24 ` [PATCH 0/5] BTRFS: Fine-tuning for five function implementations David Sterba
5 siblings, 1 reply; 14+ messages in thread
From: SF Markus Elfring @ 2017-08-21 12:42 UTC (permalink / raw)
To: linux-btrfs, Chris Mason, David Sterba, Josef Bacik; +Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 21 Aug 2017 14:15:23 +0200
Add jump targets so that a bit of exception handling can be better reused
at the end of this function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
fs/btrfs/file.c | 62 ++++++++++++++++++++++-----------------------------------
1 file changed, 24 insertions(+), 38 deletions(-)
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 74fd7756cff3..675683051cbc 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1122,25 +1122,17 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
leaf = path->nodes[0];
btrfs_item_key_to_cpu(leaf, &key, path->slots[0]);
- if (key.objectid != ino ||
- key.type != BTRFS_EXTENT_DATA_KEY) {
- ret = -EINVAL;
- btrfs_abort_transaction(trans, ret);
- goto out;
- }
+ if (key.objectid != ino || key.type != BTRFS_EXTENT_DATA_KEY)
+ goto e_inval;
+
fi = btrfs_item_ptr(leaf, path->slots[0],
struct btrfs_file_extent_item);
- if (btrfs_file_extent_type(leaf, fi) != BTRFS_FILE_EXTENT_PREALLOC) {
- ret = -EINVAL;
- btrfs_abort_transaction(trans, ret);
- goto out;
- }
+ if (btrfs_file_extent_type(leaf, fi) != BTRFS_FILE_EXTENT_PREALLOC)
+ goto e_inval;
+
extent_end = key.offset + btrfs_file_extent_num_bytes(leaf, fi);
- if (key.offset > start || extent_end < end) {
- ret = -EINVAL;
- btrfs_abort_transaction(trans, ret);
- goto out;
- }
+ if (key.offset > start || extent_end < end)
+ goto e_inval;
bytenr = btrfs_file_extent_disk_bytenr(leaf, fi);
num_bytes = btrfs_file_extent_disk_num_bytes(leaf, fi);
@@ -1213,10 +1205,8 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
btrfs_release_path(path);
goto again;
}
- if (ret < 0) {
- btrfs_abort_transaction(trans, ret);
- goto out;
- }
+ if (ret < 0)
+ goto abort_transaction;
leaf = path->nodes[0];
fi = btrfs_item_ptr(leaf, path->slots[0] - 1,
@@ -1237,18 +1227,15 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
ret = btrfs_inc_extent_ref(trans, fs_info, bytenr, num_bytes,
0, root->root_key.objectid,
ino, orig_offset);
- if (ret) {
- btrfs_abort_transaction(trans, ret);
- goto out;
- }
+ if (ret)
+ goto abort_transaction;
if (split == start) {
key.offset = start;
} else {
if (start != key.offset) {
ret = -EINVAL;
- btrfs_abort_transaction(trans, ret);
- goto out;
+ goto abort_transaction;
}
path->slots[0]--;
extent_end = end;
@@ -1271,10 +1258,8 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
ret = btrfs_free_extent(trans, fs_info, bytenr, num_bytes,
0, root->root_key.objectid,
ino, orig_offset);
- if (ret) {
- btrfs_abort_transaction(trans, ret);
- goto out;
- }
+ if (ret)
+ goto abort_transaction;
}
other_start = 0;
other_end = start;
@@ -1291,10 +1276,8 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
ret = btrfs_free_extent(trans, fs_info, bytenr, num_bytes,
0, root->root_key.objectid,
ino, orig_offset);
- if (ret) {
- btrfs_abort_transaction(trans, ret);
- goto out;
- }
+ if (ret)
+ goto abort_transaction;
}
if (del_nr == 0) {
fi = btrfs_item_ptr(leaf, path->slots[0],
@@ -1314,14 +1297,17 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
btrfs_mark_buffer_dirty(leaf);
ret = btrfs_del_items(trans, root, path, del_slot, del_nr);
- if (ret < 0) {
- btrfs_abort_transaction(trans, ret);
- goto out;
- }
+ if (ret < 0)
+ goto abort_transaction;
}
out:
btrfs_free_path(path);
return 0;
+e_inval:
+ ret = -EINVAL;
+abort_transaction:
+ btrfs_abort_transaction(trans, ret);
+ goto out;
}
/*
--
2.14.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 5/5] btrfs: Use common error handling code in btrfs_mark_extent_written()
2017-08-21 12:42 ` [PATCH 5/5] btrfs: Use common error handling code in btrfs_mark_extent_written() SF Markus Elfring
@ 2017-08-21 13:07 ` Jeff Mahoney
0 siblings, 0 replies; 14+ messages in thread
From: Jeff Mahoney @ 2017-08-21 13:07 UTC (permalink / raw)
To: SF Markus Elfring, linux-btrfs, Chris Mason, David Sterba,
Josef Bacik
Cc: LKML, kernel-janitors
[-- Attachment #1.1: Type: text/plain, Size: 4419 bytes --]
On 8/21/17 8:42 AM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 21 Aug 2017 14:15:23 +0200
>
> Add jump targets so that a bit of exception handling can be better reused
> at the end of this function.
>
> This issue was detected by using the Coccinelle software.
btrfs_abort_transaction dumps __FILE__:__LINE__ in the log so this patch
makes the code more difficult to debug.
-Jeff
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> fs/btrfs/file.c | 62 ++++++++++++++++++++++-----------------------------------
> 1 file changed, 24 insertions(+), 38 deletions(-)
>
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 74fd7756cff3..675683051cbc 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1122,25 +1122,17 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
>
> leaf = path->nodes[0];
> btrfs_item_key_to_cpu(leaf, &key, path->slots[0]);
> - if (key.objectid != ino ||
> - key.type != BTRFS_EXTENT_DATA_KEY) {
> - ret = -EINVAL;
> - btrfs_abort_transaction(trans, ret);
> - goto out;
> - }
> + if (key.objectid != ino || key.type != BTRFS_EXTENT_DATA_KEY)
> + goto e_inval;
> +
> fi = btrfs_item_ptr(leaf, path->slots[0],
> struct btrfs_file_extent_item);
> - if (btrfs_file_extent_type(leaf, fi) != BTRFS_FILE_EXTENT_PREALLOC) {
> - ret = -EINVAL;
> - btrfs_abort_transaction(trans, ret);
> - goto out;
> - }
> + if (btrfs_file_extent_type(leaf, fi) != BTRFS_FILE_EXTENT_PREALLOC)
> + goto e_inval;
> +
> extent_end = key.offset + btrfs_file_extent_num_bytes(leaf, fi);
> - if (key.offset > start || extent_end < end) {
> - ret = -EINVAL;
> - btrfs_abort_transaction(trans, ret);
> - goto out;
> - }
> + if (key.offset > start || extent_end < end)
> + goto e_inval;
>
> bytenr = btrfs_file_extent_disk_bytenr(leaf, fi);
> num_bytes = btrfs_file_extent_disk_num_bytes(leaf, fi);
> @@ -1213,10 +1205,8 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
> btrfs_release_path(path);
> goto again;
> }
> - if (ret < 0) {
> - btrfs_abort_transaction(trans, ret);
> - goto out;
> - }
> + if (ret < 0)
> + goto abort_transaction;
>
> leaf = path->nodes[0];
> fi = btrfs_item_ptr(leaf, path->slots[0] - 1,
> @@ -1237,18 +1227,15 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
> ret = btrfs_inc_extent_ref(trans, fs_info, bytenr, num_bytes,
> 0, root->root_key.objectid,
> ino, orig_offset);
> - if (ret) {
> - btrfs_abort_transaction(trans, ret);
> - goto out;
> - }
> + if (ret)
> + goto abort_transaction;
>
> if (split == start) {
> key.offset = start;
> } else {
> if (start != key.offset) {
> ret = -EINVAL;
> - btrfs_abort_transaction(trans, ret);
> - goto out;
> + goto abort_transaction;
> }
> path->slots[0]--;
> extent_end = end;
> @@ -1271,10 +1258,8 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
> ret = btrfs_free_extent(trans, fs_info, bytenr, num_bytes,
> 0, root->root_key.objectid,
> ino, orig_offset);
> - if (ret) {
> - btrfs_abort_transaction(trans, ret);
> - goto out;
> - }
> + if (ret)
> + goto abort_transaction;
> }
> other_start = 0;
> other_end = start;
> @@ -1291,10 +1276,8 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
> ret = btrfs_free_extent(trans, fs_info, bytenr, num_bytes,
> 0, root->root_key.objectid,
> ino, orig_offset);
> - if (ret) {
> - btrfs_abort_transaction(trans, ret);
> - goto out;
> - }
> + if (ret)
> + goto abort_transaction;
> }
> if (del_nr == 0) {
> fi = btrfs_item_ptr(leaf, path->slots[0],
> @@ -1314,14 +1297,17 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
> btrfs_mark_buffer_dirty(leaf);
>
> ret = btrfs_del_items(trans, root, path, del_slot, del_nr);
> - if (ret < 0) {
> - btrfs_abort_transaction(trans, ret);
> - goto out;
> - }
> + if (ret < 0)
> + goto abort_transaction;
> }
> out:
> btrfs_free_path(path);
> return 0;
> +e_inval:
> + ret = -EINVAL;
> +abort_transaction:
> + btrfs_abort_transaction(trans, ret);
> + goto out;
> }
>
> /*
>
--
Jeff Mahoney
SUSE Labs
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/5] BTRFS: Fine-tuning for five function implementations
2017-08-21 12:35 [PATCH 0/5] BTRFS: Fine-tuning for five function implementations SF Markus Elfring
` (4 preceding siblings ...)
2017-08-21 12:42 ` [PATCH 5/5] btrfs: Use common error handling code in btrfs_mark_extent_written() SF Markus Elfring
@ 2017-08-21 13:24 ` David Sterba
5 siblings, 0 replies; 14+ messages in thread
From: David Sterba @ 2017-08-21 13:24 UTC (permalink / raw)
To: SF Markus Elfring
Cc: Chris Mason, Josef Bacik, linux-btrfs, kernel-janitors, LKML
On Mon, Aug 21, 2017 at 02:35:56PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 21 Aug 2017 14:30:12 +0200
>
> A few update suggestions were taken into account
> from static source code analysis.
All patches make a nice gallery of anti-patterns. None of them will get
applied.
^ permalink raw reply [flat|nested] 14+ messages in thread