Re: [PATCH 1/4] fs: befs: Remove redundant validation from befs_find_brun_direct

[Luis de Bethencourt]

On 02/07/16 14:34, Salah Triki wrote:
> On Sat, Jul 02, 2016 at 12:38:18PM +0100, Luis de Bethencourt wrote:
>> On 02/07/16 09:05, Salah Triki wrote:
>>> The only caller of befs_find_brun_direct is befs_fblock2brun, which
>>> already validates that the block is within the range of direct blocks.
>>> So remove the duplicate validation.
>>>
>>> Signed-off-by: Salah Triki <salah.triki@xxxxxxxxx>
>>> ---
>>>  fs/befs/datastream.c | 8 --------
>>>  1 file changed, 8 deletions(-)
>>>
>>> diff --git a/fs/befs/datastream.c b/fs/befs/datastream.c
>>> index 26cc417..e224b9a 100644
>>> --- a/fs/befs/datastream.c
>>> +++ b/fs/befs/datastream.c
>>> @@ -249,17 +249,9 @@ befs_find_brun_direct(struct super_block *sb, const befs_data_stream *data,
>>>  	int i;
>>>  	const befs_block_run *array = data->direct;
>>>  	befs_blocknr_t sum;
>>> -	befs_blocknr_t max_block =
>>> -	    data->max_direct_range >> BEFS_SB(sb)->block_shift;
>>>  
>>>  	befs_debug(sb, "---> %s, find %lu", __func__, (unsigned long)blockno);
>>>  
>>> -	if (blockno > max_block) {
>>> -		befs_error(sb, "%s passed block outside of direct region",
>>> -			   __func__);
>>> -		return BEFS_ERR;
>>> -	}
>>> -
>>>  	for (i = 0, sum = 0; i < BEFS_NUM_DIRECT_BLOCKS;
>>>  	     sum += array[i].len, i++) {
>>>  		if (blockno >= sum && blockno < sum + (array[i].len)) {
>>>
>>
>> Hi Salah,
>>
>> These aren't the same check though. If we ignore the BEFS_SB(sb)->block_shift just to
>> comparing them, we can consider the checks to be the following.
>>
>> In befs_fblock2brun():
>> if (fblock < data->max_direct_range)
>>
>> In befs_find_brun_direct():
>> if (fblock > data->max_direct_range)
>>
>> Notice how one checks if the block is past the range, and the other checks if it isn't
>> before it.
>>
>> They also looked similar to me the first time I saw them and I had to double-check :)
>>
>> Thanks,
>> Luis
>>
>>
> 
> the two checks could not be evaluated to true at the same time, and since befs_find_brun_direct 
> is called only when the first check is passed, the second check will be always evaluated to false.
> So I think the second check is useless, am I right ? 
> 
> Salah
> 

You are correct!

befs_find_brun_direct() is called when the block position inside the file is inside the direct
range. Rechecking if it is past the range again inside that function, when we know it will always be
false is redundant.

Sorry for misreading it before.

The following comment on befs_find_brun_indirect() should be removed once this patch is merged, or
add the removal to this patch if you like:
-       XXX:
-       Really should check to make sure blockno is inside indirect
-       region.
-

I don't see any other caller of these three functions (_direct, _indirect, _dblindrect) being added.

Acked-by: Luis de Bethencourt <luisbg@xxxxxxxxxxxxxxx>

Thanks :)
Luis