| Message ID | 5a96945dcc12befa8fd85f6c3766b52c1b652e41.1661179270.git.fdmanana@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix lost error value deleting root reference | expand |
On 2022/8/22 22:47, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > At btrfs_del_root_ref() we are using two return variables, named 'ret' and > 'err'. This makes it harder to follow and easier to return the wrong value > in case an error happens - the previous patch in the series, which has the > subject "btrfs: fix silent failure when deleting root reference", fixed a > bug due to confusion created by these two variables. > > So change the function to use a single variable for tracking the return > value of the function, using only 'ret', which is consistent with most of > the codebase. > > Signed-off-by: Filipe Manana <fdmanana@suse.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Although one small nitpick inlined below. > --- > fs/btrfs/root-tree.c | 16 +++++++--------- > 1 file changed, 7 insertions(+), 9 deletions(-) > > diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c > index d647cb2938c0..e1f599d7a916 100644 > --- a/fs/btrfs/root-tree.c > +++ b/fs/btrfs/root-tree.c > @@ -337,7 +337,6 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, > struct extent_buffer *leaf; > struct btrfs_key key; > unsigned long ptr; > - int err = 0; > int ret; > > path = btrfs_alloc_path(); > @@ -350,7 +349,6 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, > again: > ret = btrfs_search_slot(trans, tree_root, &key, path, -1, 1); > if (ret < 0) { > - err = ret; > goto out; > } else if (ret == 0) { > leaf = path->nodes[0]; > @@ -360,18 +358,18 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, > if ((btrfs_root_ref_dirid(leaf, ref) != dirid) || > (btrfs_root_ref_name_len(leaf, ref) != name_len) || > memcmp_extent_buffer(leaf, name, ptr, name_len)) { > - err = -ENOENT; > + ret = -ENOENT; > goto out; > } > *sequence = btrfs_root_ref_sequence(leaf, ref); > > ret = btrfs_del_item(trans, tree_root, path); > - if (ret) { > - err = ret; > + if (ret) > goto out; > - } > - } else > - err = -ENOENT; > + } else { > + ret = -ENOENT; > + goto out; > + } > > if (key.type == BTRFS_ROOT_BACKREF_KEY) { > btrfs_release_path(path); To the if () check here can also be a cause of confusion. Can we split it into two dedicated btrfs_search_slot() calls (instead of current goto again with different keys) in a separate patch? I guess that's why the v1 version had some error got overriden, right? Thanks, Qu > @@ -383,7 +381,7 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, > > out: > btrfs_free_path(path); > - return err; > + return ret; > } > > /*
On Tue, Aug 23, 2022 at 07:54:12AM +0800, Qu Wenruo wrote: > > > On 2022/8/22 22:47, fdmanana@kernel.org wrote: > > From: Filipe Manana <fdmanana@suse.com> > > > > At btrfs_del_root_ref() we are using two return variables, named 'ret' and > > 'err'. This makes it harder to follow and easier to return the wrong value > > in case an error happens - the previous patch in the series, which has the > > subject "btrfs: fix silent failure when deleting root reference", fixed a > > bug due to confusion created by these two variables. > > > > So change the function to use a single variable for tracking the return > > value of the function, using only 'ret', which is consistent with most of > > the codebase. > > > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > > Reviewed-by: Qu Wenruo <wqu@suse.com> > > Although one small nitpick inlined below. > > > --- > > fs/btrfs/root-tree.c | 16 +++++++--------- > > 1 file changed, 7 insertions(+), 9 deletions(-) > > > > diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c > > index d647cb2938c0..e1f599d7a916 100644 > > --- a/fs/btrfs/root-tree.c > > +++ b/fs/btrfs/root-tree.c > > @@ -337,7 +337,6 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, > > struct extent_buffer *leaf; > > struct btrfs_key key; > > unsigned long ptr; > > - int err = 0; > > int ret; > > > > path = btrfs_alloc_path(); > > @@ -350,7 +349,6 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, > > again: > > ret = btrfs_search_slot(trans, tree_root, &key, path, -1, 1); > > if (ret < 0) { > > - err = ret; > > goto out; > > } else if (ret == 0) { > > leaf = path->nodes[0]; > > @@ -360,18 +358,18 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, > > if ((btrfs_root_ref_dirid(leaf, ref) != dirid) || > > (btrfs_root_ref_name_len(leaf, ref) != name_len) || > > memcmp_extent_buffer(leaf, name, ptr, name_len)) { > > - err = -ENOENT; > > + ret = -ENOENT; > > goto out; > > } > > *sequence = btrfs_root_ref_sequence(leaf, ref); > > > > ret = btrfs_del_item(trans, tree_root, path); > > - if (ret) { > > - err = ret; > > + if (ret) > > goto out; > > - } > > - } else > > - err = -ENOENT; > > + } else { > > + ret = -ENOENT; > > + goto out; > > + } > > > > if (key.type == BTRFS_ROOT_BACKREF_KEY) { > > btrfs_release_path(path); > > To the if () check here can also be a cause of confusion. > > Can we split it into two dedicated btrfs_search_slot() calls (instead of > current goto again with different keys) in a separate patch? > > I guess that's why the v1 version had some error got overriden, right? The problem in v1 was because I didn't think properly. I was under the wrong idea that we could have either one key type or the other, that it was to deal with some old format - like the v0 backref thing. Then I realized we got all v0 compatibility stuff removed some years ago, and that this is something different - both keys must always exist. In other words, it was not because of the if + goto or because of having two variables for the return value. I'm not sure getting rid of the if + goto logic and duplicating the deletion is better. It would duplicate a lot of code. Either way, my intention of this patch is really just to have a single variable for the return value instead of two. > > Thanks, > Qu > > @@ -383,7 +381,7 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, > > > > out: > > btrfs_free_path(path); > > - return err; > > + return ret; > > } > > > > /*
diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c index d647cb2938c0..e1f599d7a916 100644 --- a/fs/btrfs/root-tree.c +++ b/fs/btrfs/root-tree.c @@ -337,7 +337,6 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, struct extent_buffer *leaf; struct btrfs_key key; unsigned long ptr; - int err = 0; int ret; path = btrfs_alloc_path(); @@ -350,7 +349,6 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, again: ret = btrfs_search_slot(trans, tree_root, &key, path, -1, 1); if (ret < 0) { - err = ret; goto out; } else if (ret == 0) { leaf = path->nodes[0]; @@ -360,18 +358,18 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, if ((btrfs_root_ref_dirid(leaf, ref) != dirid) || (btrfs_root_ref_name_len(leaf, ref) != name_len) || memcmp_extent_buffer(leaf, name, ptr, name_len)) { - err = -ENOENT; + ret = -ENOENT; goto out; } *sequence = btrfs_root_ref_sequence(leaf, ref); ret = btrfs_del_item(trans, tree_root, path); - if (ret) { - err = ret; + if (ret) goto out; - } - } else - err = -ENOENT; + } else { + ret = -ENOENT; + goto out; + } if (key.type == BTRFS_ROOT_BACKREF_KEY) { btrfs_release_path(path); @@ -383,7 +381,7 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, out: btrfs_free_path(path); - return err; + return ret; } /*