Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <075d601a-1d79-acbd-2d03-92a1a73cf9c7@paragon-software.com>
Date:   Mon, 30 May 2022 20:11:24 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.0
Subject: Re: [PATCH 1/3] fs/ntfs3: Refactoring of indx_find function
Content-Language: en-US
To:     Joe Perches <joe@perches.com>, <ntfs3@lists.linux.dev>
CC:     <linux-kernel@vger.kernel.org>, <linux-fsdevel@vger.kernel.org>
References: <75a1215a-eda2-d0dc-b962-0334356eef7c@paragon-software.com>
 <0f9648cc-66af-077c-88e6-8650fd78f44c@paragon-software.com>
 <94dd870e498e89e0998dee4dd0dbaaa4b4497929.camel@perches.com>
From:   Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
In-Reply-To: <94dd870e498e89e0998dee4dd0dbaaa4b4497929.camel@perches.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hello.

Thanks for your input.
It's nice to have a clear typical return value, so I've updated patch.


On 5/27/22 19:07, Joe Perches wrote:
> On Fri, 2022-05-27 at 17:21 +0300, Almaz Alexandrovich wrote:
>> This commit makes function a bit more readable
> 
> trivia:
> 
>> diff --git a/fs/ntfs3/index.c b/fs/ntfs3/index.c
> []
>> @@ -1042,19 +1042,16 @@ int indx_find(struct ntfs_index *indx, struct ntfs_inode *ni,
>>    {
>>    	int err;
>>    	struct NTFS_DE *e;
>> -	const struct INDEX_HDR *hdr;
>>    	struct indx_node *node;
>>    
>>    	if (!root)
>>    		root = indx_get_root(&ni->dir, ni, NULL, NULL);
>>    
>>    	if (!root) {
>> -		err = -EINVAL;
>> -		goto out;
>> +		/* Should not happed. */
>> +		return -EINVAL;
> 
> s/happed/happen/
> 
>>    	for (;;) {
>>    		node = NULL;
>>    		if (*diff >= 0 || !de_has_vcn_ex(e)) {
>>    			*entry = e;
>> -			goto out;
>> +			return 0;
>>    		}
> 
> might be nicer with a break; or a while like
> 
> 	while (*diff < 0 && de_has_vcn_ex(e)) {
> 		node = NULL;
> 
> 
>>    		/* Read next level. */
>>    		err = indx_read(indx, ni, de_get_vbn(e), &node);
>>    		if (err)
>> -			goto out;
>> +			return err;
>>    
>>    		/* Lookup entry that is <= to the search value. */
>>    		e = hdr_find_e(indx, &node->index->ihdr, key, key_len, ctx,
>>    			       diff);
>>    		if (!e) {
>> -			err = -EINVAL;
>>    			put_indx_node(node);
>> -			goto out;
>> +			return -EINVAL;
>>    		}
>>    
>>    		fnd_push(fnd, node, e);
>>    	}
>> -
>> -out:
>> -	return err;
> 
> and a return 0;
> 
> or
> 	*entry = e;
> 	return 0;
> 
> so it appears that the function has a typical return value.
>