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

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

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