Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1435738 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/4] fs: befs: Remove redundant validation from befs_find_brun_direct Salah Triki <salah.triki@gmail.com> - 2016-07-02 10:10 +0200
    [PATCH 3/4] fs: befs: Remove useless calls to brelse in befs_find_brun_dblindirect Salah Triki <salah.triki@gmail.com> - 2016-07-02 10:10 +0200
      Re: [PATCH 3/4] fs: befs: Remove useless calls to brelse in befs_find_brun_dblindirect Luis de Bethencourt <luisbg@osg.samsung.com> - 2016-07-02 14:00 +0200
    [PATCH 2/4] fs: befs: Coding style fix Salah Triki <salah.triki@gmail.com> - 2016-07-02 10:10 +0200
      Re: [PATCH 2/4] fs: befs: Coding style fix Luis de Bethencourt <luisbg@osg.samsung.com> - 2016-07-02 13:30 +0200
    [PATCH 4/4] fs: befs: Remove goto from befs_bread_iaddr Salah Triki <salah.triki@gmail.com> - 2016-07-02 10:10 +0200
      Re: [PATCH 4/4] fs: befs: Remove goto from befs_bread_iaddr Luis de Bethencourt <luisbg@osg.samsung.com> - 2016-07-02 14:10 +0200
    Re: [PATCH 1/4] fs: befs: Remove redundant validation from befs_find_brun_direct Luis de Bethencourt <luisbg@osg.samsung.com> - 2016-07-02 13:40 +0200
      Re: [PATCH 1/4] fs: befs: Remove redundant validation from  befs_find_brun_direct Salah Triki <salah.triki@acm.org> - 2016-07-02 16:00 +0200
        Re: [PATCH 1/4] fs: befs: Remove redundant validation from befs_find_brun_direct Luis de Bethencourt <luisbg@osg.samsung.com> - 2016-07-02 16:50 +0200

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1435800

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1435831

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web