From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752328AbcGBOoM (ORCPT ); Sat, 2 Jul 2016 10:44:12 -0400 Received: from ec2-52-27-115-49.us-west-2.compute.amazonaws.com ([52.27.115.49]:45461 "EHLO s-opensource.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751360AbcGBOoL (ORCPT ); Sat, 2 Jul 2016 10:44:11 -0400 Message-ID: <5777D335.2060205@osg.samsung.com> Date: Sat, 02 Jul 2016 15:44:05 +0100 From: Luis de Bethencourt User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.8.0 MIME-Version: 1.0 To: Salah Triki CC: Salah Triki , akpm@linux-foundation.org, viro@zeniv.linux.org.uk, mhocko@suse.com, vdavydov@virtuozzo.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/4] fs: befs: Remove redundant validation from befs_find_brun_direct References: <9859a616787673d2f36ce21d4aa0220637c55001.1467446561.git.salah.triki@gmail.com> <5777A7AA.4020500@osg.samsung.com> <20160702133443.GB3817@pc> In-Reply-To: <20160702133443.GB3817@pc> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>> --- >>> 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 Thanks :) Luis