Re: [PATCH v3 6/6] btrfs: rename and optimize return variable in btrfs_find_orphan_roots

[Anand Jain <anand.jain@oracle.com>]

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
>>