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

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

[Salah Triki]

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@gmail.com>
---
 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)) {
-- 
1.9.1

[PATCH 2/4] fs: befs: Coding style fix

[Salah Triki]

Constant has to be capitalized.

Signed-off-by: Salah Triki <salah.triki@gmail.com>
---
 fs/befs/btree.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/befs/btree.c b/fs/befs/btree.c
index 307645f9..e59ad20 100644
--- a/fs/befs/btree.c
+++ b/fs/befs/btree.c
@@ -85,7 +85,7 @@ struct befs_btree_node {
 };
 
 /* local constants */
-static const befs_off_t befs_bt_inval = 0xffffffffffffffffULL;
+static const befs_off_t BEFS_BT_INVAL = 0xffffffffffffffffULL;
 
 /* local functions */
 static int befs_btree_seekleaf(struct super_block *sb, const befs_data_stream *ds,
@@ -467,7 +467,7 @@ befs_btree_read(struct super_block *sb, const befs_data_stream *ds,
 	while (key_sum + this_node->head.all_key_count <= key_no) {
 
 		/* no more nodes to look in: key_no is too large */
-		if (this_node->head.right == befs_bt_inval) {
+		if (this_node->head.right == BEFS_BT_INVAL) {
 			*keysize = 0;
 			*value = 0;
 			befs_debug(sb,
@@ -608,7 +608,7 @@ static int
 befs_leafnode(struct befs_btree_node *node)
 {
 	/* all interior nodes (and only interior nodes) have an overflow node */
-	if (node->head.overflow == befs_bt_inval)
+	if (node->head.overflow == BEFS_BT_INVAL)
 		return 1;
 	else
 		return 0;
-- 
1.9.1

Re: [PATCH 2/4] fs: befs: Coding style fix

[Luis de Bethencourt]

On 02/07/16 09:05, Salah Triki wrote:
> Constant has to be capitalized.
> 
> Signed-off-by: Salah Triki <salah.triki@gmail.com>
> ---
>  fs/befs/btree.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/befs/btree.c b/fs/befs/btree.c
> index 307645f9..e59ad20 100644
> --- a/fs/befs/btree.c
> +++ b/fs/befs/btree.c
> @@ -85,7 +85,7 @@ struct befs_btree_node {
>  };
>  
>  /* local constants */
> -static const befs_off_t befs_bt_inval = 0xffffffffffffffffULL;
> +static const befs_off_t BEFS_BT_INVAL = 0xffffffffffffffffULL;
>  
>  /* local functions */
>  static int befs_btree_seekleaf(struct super_block *sb, const befs_data_stream *ds,
> @@ -467,7 +467,7 @@ befs_btree_read(struct super_block *sb, const befs_data_stream *ds,
>  	while (key_sum + this_node->head.all_key_count <= key_no) {
>  
>  		/* no more nodes to look in: key_no is too large */
> -		if (this_node->head.right == befs_bt_inval) {
> +		if (this_node->head.right == BEFS_BT_INVAL) {
>  			*keysize = 0;
>  			*value = 0;
>  			befs_debug(sb,
> @@ -608,7 +608,7 @@ static int
>  befs_leafnode(struct befs_btree_node *node)
>  {
>  	/* all interior nodes (and only interior nodes) have an overflow node */
> -	if (node->head.overflow == befs_bt_inval)
> +	if (node->head.overflow == BEFS_BT_INVAL)
>  		return 1;
>  	else
>  		return 0;
> 

Hi Salah,

What the coding style says is:
"Names of macros defining constants and labels in enums are capitalized."

Even though this isn't a macro, when reading the code it is good to know it is
almost like one. I agree this makes the code more readable.

Acked-by: Luis de Bethencourt <luisbg@osg.samsung.com>

Thanks,
Luis

[PATCH 3/4] fs: befs: Remove useless calls to brelse in befs_find_brun_dblindirect

[Salah Triki]

The calls to brelse are useless since dbl_indir_block and indir_block
are NULL.

Signed-off-by: Salah Triki <salah.triki@gmail.com>
---
 fs/befs/datastream.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/befs/datastream.c b/fs/befs/datastream.c
index e224b9a..b68b6f9 100644
--- a/fs/befs/datastream.c
+++ b/fs/befs/datastream.c
@@ -471,7 +471,6 @@ befs_find_brun_dblindirect(struct super_block *sb,
 			   (unsigned long)
 			   iaddr2blockno(sb, &data->double_indirect) +
 			   dbl_which_block);
-		brelse(dbl_indir_block);
 		return BEFS_ERR;
 	}
 
@@ -496,7 +495,6 @@ befs_find_brun_dblindirect(struct super_block *sb,
 		befs_error(sb, "%s couldn't read the indirect block "
 			   "at blockno %lu", __func__, (unsigned long)
 			   iaddr2blockno(sb, &indir_run) + which_block);
-		brelse(indir_block);
 		return BEFS_ERR;
 	}
 
-- 
1.9.1

Re: [PATCH 3/4] fs: befs: Remove useless calls to brelse in befs_find_brun_dblindirect

[Luis de Bethencourt]

On 02/07/16 09:05, Salah Triki wrote:
> The calls to brelse are useless since dbl_indir_block and indir_block
> are NULL.
> 
> Signed-off-by: Salah Triki <salah.triki@gmail.com>
> ---
>  fs/befs/datastream.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/befs/datastream.c b/fs/befs/datastream.c
> index e224b9a..b68b6f9 100644
> --- a/fs/befs/datastream.c
> +++ b/fs/befs/datastream.c
> @@ -471,7 +471,6 @@ befs_find_brun_dblindirect(struct super_block *sb,
>  			   (unsigned long)
>  			   iaddr2blockno(sb, &data->double_indirect) +
>  			   dbl_which_block);
> -		brelse(dbl_indir_block);
>  		return BEFS_ERR;
>  	}
>  
> @@ -496,7 +495,6 @@ befs_find_brun_dblindirect(struct super_block *sb,
>  		befs_error(sb, "%s couldn't read the indirect block "
>  			   "at blockno %lu", __func__, (unsigned long)
>  			   iaddr2blockno(sb, &indir_run) + which_block);
> -		brelse(indir_block);
>  		return BEFS_ERR;
>  	}
>  
> 

Great catch!

This went unnoticed because brelse() checks if the buffer_head is NULL before
decrementing the reference count. Which in these two cases they clearly are NULL.

Acked-by: Luis de Bethencourt <luisbg@osg.samsung.com>

Thanks!
Luis

[PATCH 4/4] fs: befs: Remove goto from befs_bread_iaddr

[Salah Triki]

Since goto statement merely returns NULL, replace it with return
statement.

Signed-off-by: Salah Triki <salah.triki@gmail.com>
---
 fs/befs/io.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/fs/befs/io.c b/fs/befs/io.c
index 4223b77..af631a6 100644
--- a/fs/befs/io.c
+++ b/fs/befs/io.c
@@ -37,7 +37,7 @@ befs_bread_iaddr(struct super_block *sb, befs_inode_addr iaddr)
 	if (iaddr.allocation_group > befs_sb->num_ags) {
 		befs_error(sb, "BEFS: Invalid allocation group %u, max is %u",
 			   iaddr.allocation_group, befs_sb->num_ags);
-		goto error;
+		return NULL;
 	}
 
 	block = iaddr2blockno(sb, &iaddr);
@@ -49,13 +49,9 @@ befs_bread_iaddr(struct super_block *sb, befs_inode_addr iaddr)
 	if (bh == NULL) {
 		befs_error(sb, "Failed to read block %lu",
 			   (unsigned long)block);
-		goto error;
+		return NULL;
 	}
 
 	befs_debug(sb, "<--- %s", __func__);
 	return bh;
-
-      error:
-	befs_debug(sb, "<--- %s ERROR", __func__);
-	return NULL;
 }
-- 
1.9.1

Re: [PATCH 4/4] fs: befs: Remove goto from befs_bread_iaddr

[Luis de Bethencourt]

On 02/07/16 09:05, Salah Triki wrote:
> Since goto statement merely returns NULL, replace it with return
> statement.
> 
> Signed-off-by: Salah Triki <salah.triki@gmail.com>
> ---
>  fs/befs/io.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/befs/io.c b/fs/befs/io.c
> index 4223b77..af631a6 100644
> --- a/fs/befs/io.c
> +++ b/fs/befs/io.c
> @@ -37,7 +37,7 @@ befs_bread_iaddr(struct super_block *sb, befs_inode_addr iaddr)
>  	if (iaddr.allocation_group > befs_sb->num_ags) {
>  		befs_error(sb, "BEFS: Invalid allocation group %u, max is %u",
>  			   iaddr.allocation_group, befs_sb->num_ags);
> -		goto error;
> +		return NULL;
>  	}
>  
>  	block = iaddr2blockno(sb, &iaddr);
> @@ -49,13 +49,9 @@ befs_bread_iaddr(struct super_block *sb, befs_inode_addr iaddr)
>  	if (bh == NULL) {
>  		befs_error(sb, "Failed to read block %lu",
>  			   (unsigned long)block);
> -		goto error;
> +		return NULL;
>  	}
>  
>  	befs_debug(sb, "<--- %s", __func__);
>  	return bh;
> -
> -      error:
> -	befs_debug(sb, "<--- %s ERROR", __func__);
> -	return NULL;
>  }
> 

Hi Salah,

The goto statement also calls the debug function. I  know it doesn't look pretty
to have "debug; return; label; debug; return;", but I find these debug calls at
the start and end of functions very useful when reading the log to familiarize
myself with the code or test a change.

An alternative in this case would be to add the function name to the error messages
just before those goto errors. But I don't think that would be better than the
current code.

Just as a heads up there are 2 similar cases to this in fs/befs/btree.c
error:
        befs_debug(sb, "<--- %s ERROR", __func__);
        return BEFS_ERR;

Thanks,
Luis

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

[Luis de Bethencourt]

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@gmail.com>
> ---
>  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

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

[Salah Triki]

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@gmail.com>
> > ---
> >  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

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@gmail.com>
>>> ---
>>>  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@osg.samsung.com>

Thanks :)
Luis