| Message ID | f070919ec910b3682dd22742151a60f9e4c95cbf.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(), if btrfs_search_slot() returns an error, we end > up returning from the function with a value of 0 (success). This happens > because the function returns the value stored in the variable 'err', which > is 0, while the error value we got from btrfs_search_slot() is stored in > the 'ret' variable. > > So fix it by setting 'err' with the error value. > > Fixes: 8289ed9f93bef2 ("btrfs: replace the BUG_ON in btrfs_del_root_ref with proper error handling") > Signed-off-by: Filipe Manana <fdmanana@suse.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks for catching this, Qu > --- > fs/btrfs/root-tree.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c > index a64b26b16904..d647cb2938c0 100644 > --- a/fs/btrfs/root-tree.c > +++ b/fs/btrfs/root-tree.c > @@ -349,9 +349,10 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, > key.offset = ref_id; > again: > ret = btrfs_search_slot(trans, tree_root, &key, path, -1, 1); > - if (ret < 0) > + if (ret < 0) { > + err = ret; > goto out; > - if (ret == 0) { Just a small nitpick here, since above if (ret < 0) branch will call "goto out", we don't need the "else" branch. The old "if (ret == 0) {" should be good enough. Thanks, Qu > + } else if (ret == 0) { > leaf = path->nodes[0]; > ref = btrfs_item_ptr(leaf, path->slots[0], > struct btrfs_root_ref);
On Tue, Aug 23, 2022 at 12:47 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: > > > > On 2022/8/22 22:47, fdmanana@kernel.org wrote: > > From: Filipe Manana <fdmanana@suse.com> > > > > At btrfs_del_root_ref(), if btrfs_search_slot() returns an error, we end > > up returning from the function with a value of 0 (success). This happens > > because the function returns the value stored in the variable 'err', which > > is 0, while the error value we got from btrfs_search_slot() is stored in > > the 'ret' variable. > > > > So fix it by setting 'err' with the error value. > > > > Fixes: 8289ed9f93bef2 ("btrfs: replace the BUG_ON in btrfs_del_root_ref with proper error handling") > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > > Reviewed-by: Qu Wenruo <wqu@suse.com> > > Thanks for catching this, > Qu > > --- > > fs/btrfs/root-tree.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c > > index a64b26b16904..d647cb2938c0 100644 > > --- a/fs/btrfs/root-tree.c > > +++ b/fs/btrfs/root-tree.c > > @@ -349,9 +349,10 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, > > key.offset = ref_id; > > again: > > ret = btrfs_search_slot(trans, tree_root, &key, path, -1, 1); > > - if (ret < 0) > > + if (ret < 0) { > > + err = ret; > > goto out; > > - if (ret == 0) { > > Just a small nitpick here, since above if (ret < 0) branch will call > "goto out", we don't need the "else" branch. > The old "if (ret == 0) {" should be good enough. Yes, it's not about correctness. It's just my preferred style. I find it more intuitive to have a single if-else that tests the same variable instead of having it tested by two distinct if statements. > > Thanks, > Qu > > > + } else if (ret == 0) { > > leaf = path->nodes[0]; > > ref = btrfs_item_ptr(leaf, path->slots[0], > > struct btrfs_root_ref);
On Tue, Aug 23, 2022 at 09:11:55AM +0100, Filipe Manana wrote: > On Tue, Aug 23, 2022 at 12:47 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: > > > --- > > > fs/btrfs/root-tree.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c > > > index a64b26b16904..d647cb2938c0 100644 > > > --- a/fs/btrfs/root-tree.c > > > +++ b/fs/btrfs/root-tree.c > > > @@ -349,9 +349,10 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, > > > key.offset = ref_id; > > > again: > > > ret = btrfs_search_slot(trans, tree_root, &key, path, -1, 1); > > > - if (ret < 0) > > > + if (ret < 0) { > > > + err = ret; > > > goto out; > > > - if (ret == 0) { > > > > Just a small nitpick here, since above if (ret < 0) branch will call > > "goto out", we don't need the "else" branch. > > The old "if (ret == 0) {" should be good enough. > > Yes, it's not about correctness. It's just my preferred style. > I find it more intuitive to have a single if-else that tests the same > variable instead > of having it tested by two distinct if statements. Agreed with that style.
diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c index a64b26b16904..d647cb2938c0 100644 --- a/fs/btrfs/root-tree.c +++ b/fs/btrfs/root-tree.c @@ -349,9 +349,10 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans, u64 root_id, key.offset = ref_id; again: ret = btrfs_search_slot(trans, tree_root, &key, path, -1, 1); - if (ret < 0) + if (ret < 0) { + err = ret; goto out; - if (ret == 0) { + } else if (ret == 0) { leaf = path->nodes[0]; ref = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_root_ref);