| Message ID | 7b9f87e3ca3368648e9df1d124161a6d4b8e1e9a.1715783315.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | part3 trivial adjustments for return variable coding style | expand |
On Thu, May 16, 2024 at 07:12:15PM +0800, Anand Jain wrote: > The variable err is the actual return value of this function, and the > variable ret is a helper variable for err, which actually is not > needed and can be handled just by err, which is renamed to ret. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > v3: drop ret2 as there is no need for it. > v2: n/a > fs/btrfs/root-tree.c | 32 ++++++++++++++++---------------- > 1 file changed, 16 insertions(+), 16 deletions(-) > > diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c > index 33962671a96c..c11b0bccf513 100644 > --- a/fs/btrfs/root-tree.c > +++ b/fs/btrfs/root-tree.c > @@ -220,8 +220,7 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info) > struct btrfs_path *path; > struct btrfs_key key; > struct btrfs_root *root; > - int err = 0; > - int ret; > + int ret = 0; > > path = btrfs_alloc_path(); > if (!path) > @@ -235,18 +234,19 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info) > u64 root_objectid; > > ret = btrfs_search_slot(NULL, tree_root, &key, path, 0, 0); > - if (ret < 0) { > - err = ret; > + if (ret < 0) > break; > - } > + ret = 0; Should this be handled when ret > 0? This would be unexpected and probably means a corruption but simply overwriting the value does not seem right. > > leaf = path->nodes[0]; > if (path->slots[0] >= btrfs_header_nritems(leaf)) { > ret = btrfs_next_leaf(tree_root, path); > if (ret < 0) > - err = ret; > - if (ret != 0) > break; > + if (ret > 0) { > + ret = 0; > + break; > + } > leaf = path->nodes[0]; > } > > @@ -261,26 +261,26 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info) > key.offset++; > > root = btrfs_get_fs_root(fs_info, root_objectid, false); > - err = PTR_ERR_OR_ZERO(root); > - if (err && err != -ENOENT) { > + ret = PTR_ERR_OR_ZERO(root); > + if (ret && ret != -ENOENT) { > break; > - } else if (err == -ENOENT) { > + } else if (ret == -ENOENT) { > struct btrfs_trans_handle *trans; > > btrfs_release_path(path); > > trans = btrfs_join_transaction(tree_root); > if (IS_ERR(trans)) { > - err = PTR_ERR(trans); > - btrfs_handle_fs_error(fs_info, err, > + ret = PTR_ERR(trans); > + btrfs_handle_fs_error(fs_info, ret, > "Failed to start trans to delete orphan item"); > break; > } > - err = btrfs_del_orphan_item(trans, tree_root, > + ret = btrfs_del_orphan_item(trans, tree_root, > root_objectid); > btrfs_end_transaction(trans); > - if (err) { > - btrfs_handle_fs_error(fs_info, err, > + if (ret) { > + btrfs_handle_fs_error(fs_info, ret, > "Failed to delete root orphan item"); > break; > } > @@ -311,7 +311,7 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info) > } > > btrfs_free_path(path); > - return err; > + return ret; > } > > /* drop the root item for 'key' from the tree root */ > -- > 2.38.1 >
On 5/21/24 23:18, David Sterba wrote: > On Thu, May 16, 2024 at 07:12:15PM +0800, Anand Jain wrote: >> The variable err is the actual return value of this function, and the >> variable ret is a helper variable for err, which actually is not >> needed and can be handled just by err, which is renamed to ret. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> v3: drop ret2 as there is no need for it. >> v2: n/a >> fs/btrfs/root-tree.c | 32 ++++++++++++++++---------------- >> 1 file changed, 16 insertions(+), 16 deletions(-) >> >> diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c >> index 33962671a96c..c11b0bccf513 100644 >> --- a/fs/btrfs/root-tree.c >> +++ b/fs/btrfs/root-tree.c >> @@ -220,8 +220,7 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info) >> struct btrfs_path *path; >> struct btrfs_key key; >> struct btrfs_root *root; >> - int err = 0; >> - int ret; >> + int ret = 0; >> >> path = btrfs_alloc_path(); >> if (!path) >> @@ -235,18 +234,19 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info) >> u64 root_objectid; >> >> ret = btrfs_search_slot(NULL, tree_root, &key, path, 0, 0); >> - if (ret < 0) { >> - err = ret; >> + if (ret < 0) >> break; >> - } >> + ret = 0; > > Should this be handled when ret > 0? This would be unexpected and > probably means a corruption but simply overwriting the value does not > seem right. > Agreed. + if (ret > 0) + ret = 0; is much neater. As in v4. Thanks, Anand >> >> leaf = path->nodes[0]; >> if (path->slots[0] >= btrfs_header_nritems(leaf)) { >> ret = btrfs_next_leaf(tree_root, path); >> if (ret < 0) >> - err = ret; >> - if (ret != 0) >> break; >> + if (ret > 0) { >> + ret = 0; >> + break; >> + } >> leaf = path->nodes[0]; >> } >> >> @@ -261,26 +261,26 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info) >> key.offset++; >> >> root = btrfs_get_fs_root(fs_info, root_objectid, false); >> - err = PTR_ERR_OR_ZERO(root); >> - if (err && err != -ENOENT) { >> + ret = PTR_ERR_OR_ZERO(root); >> + if (ret && ret != -ENOENT) { >> break; >> - } else if (err == -ENOENT) { >> + } else if (ret == -ENOENT) { >> struct btrfs_trans_handle *trans; >> >> btrfs_release_path(path); >> >> trans = btrfs_join_transaction(tree_root); >> if (IS_ERR(trans)) { >> - err = PTR_ERR(trans); >> - btrfs_handle_fs_error(fs_info, err, >> + ret = PTR_ERR(trans); >> + btrfs_handle_fs_error(fs_info, ret, >> "Failed to start trans to delete orphan item"); >> break; >> } >> - err = btrfs_del_orphan_item(trans, tree_root, >> + ret = btrfs_del_orphan_item(trans, tree_root, >> root_objectid); >> btrfs_end_transaction(trans); >> - if (err) { >> - btrfs_handle_fs_error(fs_info, err, >> + if (ret) { >> + btrfs_handle_fs_error(fs_info, ret, >> "Failed to delete root orphan item"); >> break; >> } >> @@ -311,7 +311,7 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info) >> } >> >> btrfs_free_path(path); >> - return err; >> + return ret; >> } >> >> /* drop the root item for 'key' from the tree root */ >> -- >> 2.38.1 >>
On Wed, May 22, 2024 at 01:10:08AM +0800, Anand Jain wrote: > > > On 5/21/24 23:18, David Sterba wrote: > > On Thu, May 16, 2024 at 07:12:15PM +0800, Anand Jain wrote: > >> The variable err is the actual return value of this function, and the > >> variable ret is a helper variable for err, which actually is not > >> needed and can be handled just by err, which is renamed to ret. > >> > >> Signed-off-by: Anand Jain <anand.jain@oracle.com> > >> --- > >> v3: drop ret2 as there is no need for it. > >> v2: n/a > >> fs/btrfs/root-tree.c | 32 ++++++++++++++++---------------- > >> 1 file changed, 16 insertions(+), 16 deletions(-) > >> > >> diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c > >> index 33962671a96c..c11b0bccf513 100644 > >> --- a/fs/btrfs/root-tree.c > >> +++ b/fs/btrfs/root-tree.c > >> @@ -220,8 +220,7 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info) > >> struct btrfs_path *path; > >> struct btrfs_key key; > >> struct btrfs_root *root; > >> - int err = 0; > >> - int ret; > >> + int ret = 0; > >> > >> path = btrfs_alloc_path(); > >> if (!path) > >> @@ -235,18 +234,19 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info) > >> u64 root_objectid; > >> > >> ret = btrfs_search_slot(NULL, tree_root, &key, path, 0, 0); > >> - if (ret < 0) { > >> - err = ret; > >> + if (ret < 0) > >> break; > >> - } > >> + ret = 0; > > > > Should this be handled when ret > 0? This would be unexpected and > > probably means a corruption but simply overwriting the value does not > > seem right. > > > > Agreed. > > + if (ret > 0) > + ret = 0; > > is much neater. That's not what I meant. When btrfs_search_slot() returns 1 then the key was not found and could be inserted, path points to the slot. This is done in many other places, so in the orphan root lookup it should be also handled.
On 22/05/2024 01:59, David Sterba wrote: > On Wed, May 22, 2024 at 01:10:08AM +0800, Anand Jain wrote: >> >> >> On 5/21/24 23:18, David Sterba wrote: >>> On Thu, May 16, 2024 at 07:12:15PM +0800, Anand Jain wrote: >>>> The variable err is the actual return value of this function, and the >>>> variable ret is a helper variable for err, which actually is not >>>> needed and can be handled just by err, which is renamed to ret. >>>> >>>> Signed-off-by: Anand Jain <anand.jain@oracle.com> >>>> --- >>>> v3: drop ret2 as there is no need for it. >>>> v2: n/a >>>> fs/btrfs/root-tree.c | 32 ++++++++++++++++---------------- >>>> 1 file changed, 16 insertions(+), 16 deletions(-) >>>> >>>> diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c >>>> index 33962671a96c..c11b0bccf513 100644 >>>> --- a/fs/btrfs/root-tree.c >>>> +++ b/fs/btrfs/root-tree.c >>>> @@ -220,8 +220,7 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info) >>>> struct btrfs_path *path; >>>> struct btrfs_key key; >>>> struct btrfs_root *root; >>>> - int err = 0; >>>> - int ret; >>>> + int ret = 0; >>>> >>>> path = btrfs_alloc_path(); >>>> if (!path) >>>> @@ -235,18 +234,19 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info) >>>> u64 root_objectid; >>>> >>>> ret = btrfs_search_slot(NULL, tree_root, &key, path, 0, 0); >>>> - if (ret < 0) { >>>> - err = ret; >>>> + if (ret < 0) >>>> break; >>>> - } >>>> + ret = 0; >>> >>> Should this be handled when ret > 0? This would be unexpected and >>> probably means a corruption but simply overwriting the value does not >>> seem right. >>> >> >> Agreed. >> >> + if (ret > 0) >> + ret = 0; >> >> is much neater. > > That's not what I meant. When btrfs_search_slot() returns 1 then the key > was not found and could be inserted, path points to the slot. This is > done in many other places, so in the orphan root lookup it should be > also handled. For the scenario where ret > 0 is good, we generally do varied tasks. However, here we need to reassign ret = 0. Originally, err remained 0 and returned 0. Or, my bad, I didn't understand which usual error handling pattern you are referring to. Thanks, Anand
diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c index 33962671a96c..c11b0bccf513 100644 --- a/fs/btrfs/root-tree.c +++ b/fs/btrfs/root-tree.c @@ -220,8 +220,7 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info) struct btrfs_path *path; struct btrfs_key key; struct btrfs_root *root; - int err = 0; - int ret; + int ret = 0; path = btrfs_alloc_path(); if (!path) @@ -235,18 +234,19 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info) u64 root_objectid; ret = btrfs_search_slot(NULL, tree_root, &key, path, 0, 0); - if (ret < 0) { - err = ret; + if (ret < 0) break; - } + ret = 0; leaf = path->nodes[0]; if (path->slots[0] >= btrfs_header_nritems(leaf)) { ret = btrfs_next_leaf(tree_root, path); if (ret < 0) - err = ret; - if (ret != 0) break; + if (ret > 0) { + ret = 0; + break; + } leaf = path->nodes[0]; } @@ -261,26 +261,26 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info) key.offset++; root = btrfs_get_fs_root(fs_info, root_objectid, false); - err = PTR_ERR_OR_ZERO(root); - if (err && err != -ENOENT) { + ret = PTR_ERR_OR_ZERO(root); + if (ret && ret != -ENOENT) { break; - } else if (err == -ENOENT) { + } else if (ret == -ENOENT) { struct btrfs_trans_handle *trans; btrfs_release_path(path); trans = btrfs_join_transaction(tree_root); if (IS_ERR(trans)) { - err = PTR_ERR(trans); - btrfs_handle_fs_error(fs_info, err, + ret = PTR_ERR(trans); + btrfs_handle_fs_error(fs_info, ret, "Failed to start trans to delete orphan item"); break; } - err = btrfs_del_orphan_item(trans, tree_root, + ret = btrfs_del_orphan_item(trans, tree_root, root_objectid); btrfs_end_transaction(trans); - if (err) { - btrfs_handle_fs_error(fs_info, err, + if (ret) { + btrfs_handle_fs_error(fs_info, ret, "Failed to delete root orphan item"); break; } @@ -311,7 +311,7 @@ int btrfs_find_orphan_roots(struct btrfs_fs_info *fs_info) } btrfs_free_path(path); - return err; + return ret; } /* drop the root item for 'key' from the tree root */
The variable err is the actual return value of this function, and the variable ret is a helper variable for err, which actually is not needed and can be handled just by err, which is renamed to ret. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- v3: drop ret2 as there is no need for it. v2: n/a fs/btrfs/root-tree.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)