[PATCH 0/4] ISOFS: Adjustments for three function implementations

[PATCH 0/4] ISOFS: Adjustments for three function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 19 Aug 2017 08:40:12 +0200

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (4):
  Delete an error message for a failed memory allocation in isofs_read_inode()
  Adjust four checks for null pointers
  One check less in isofs_read_inode() after error detection
  Delete an unnecessary variable initialisation in isofs_read_inode()

 fs/isofs/inode.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

-- 
2.14.0

[PATCH 0/4] ISOFS: Adjustments for three function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 19 Aug 2017 08:40:12 +0200

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (4):
  Delete an error message for a failed memory allocation in isofs_read_inode()
  Adjust four checks for null pointers
  One check less in isofs_read_inode() after error detection
  Delete an unnecessary variable initialisation in isofs_read_inode()

 fs/isofs/inode.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

-- 
2.14.0

[PATCH 1/4] isofs: Delete an error message for a failed memory allocation in isofs_read_inode()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 18 Aug 2017 21:41:24 +0200

Omit an extra message for a memory allocation failure in this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 fs/isofs/inode.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
index 217a5e7815da..2d3f090944df 100644
--- a/fs/isofs/inode.c
+++ b/fs/isofs/inode.c
@@ -1324,4 +1324,3 @@ static int isofs_read_inode(struct inode *inode, int relocated)
-			printk(KERN_INFO "%s: out of memory\n", __func__);
 			ret = -ENOMEM;
 			goto fail;
 		}
-- 
2.14.0

[PATCH 1/4] isofs: Delete an error message for a failed memory allocation in isofs_read_inode()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 18 Aug 2017 21:41:24 +0200

Omit an extra message for a memory allocation failure in this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 fs/isofs/inode.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
index 217a5e7815da..2d3f090944df 100644
--- a/fs/isofs/inode.c
+++ b/fs/isofs/inode.c
@@ -1324,4 +1324,3 @@ static int isofs_read_inode(struct inode *inode, int relocated)
-			printk(KERN_INFO "%s: out of memory\n", __func__);
 			ret = -ENOMEM;
 			goto fail;
 		}
-- 
2.14.0

[PATCH 2/4] isofs: Adjust four checks for null pointers

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 18 Aug 2017 21:47:14 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The script “checkpatch.pl” pointed information out like the following.

Comparison to NULL could be written !…

Thus fix the affected source code places.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 fs/isofs/inode.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
index 2d3f090944df..db43f52f2268 100644
--- a/fs/isofs/inode.c
+++ b/fs/isofs/inode.c
@@ -96,7 +96,7 @@ static int __init init_inodecache(void)
 					0, (SLAB_RECLAIM_ACCOUNT|
 					SLAB_MEM_SPREAD|SLAB_ACCOUNT),
 					init_once);
-	if (isofs_inode_cachep == NULL)
+	if (!isofs_inode_cachep)
 		return -ENOMEM;
 	return 0;
 }
@@ -678,7 +678,7 @@ static int isofs_fill_super(struct super_block *s, void *data, int silent)
 			if (isonum_711(vdp->type) == ISO_VD_END)
 				break;
 			if (isonum_711(vdp->type) == ISO_VD_PRIMARY) {
-				if (pri == NULL) {
+				if (!pri) {
 					pri = (struct iso_primary_descriptor *)vdp;
 					/* Save the buffer in case we need it ... */
 					pri_bh = bh;
@@ -742,7 +742,7 @@ static int isofs_fill_super(struct super_block *s, void *data, int silent)
 		goto out_freebh;
 	}
 
-	if (joliet_level && (pri == NULL || !opt.rock)) {
+	if (joliet_level && (!pri || !opt.rock)) {
 		/* This is the case of Joliet with the norock mount flag.
 		 * A disc with both Joliet and Rock Ridge is handled later
 		 */
@@ -1320,5 +1320,5 @@ static int isofs_read_inode(struct inode *inode, int relocated)
 		int frag1 = bufsize - offset;
 
 		tmpde = kmalloc(de_len, GFP_KERNEL);
-		if (tmpde == NULL) {
+		if (!tmpde) {
 			ret = -ENOMEM;
-- 
2.14.0

[PATCH 2/4] isofs: Adjust four checks for null pointers

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 18 Aug 2017 21:47:14 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The script “checkpatch.pl” pointed information out like the following.

Comparison to NULL could be written !…

Thus fix the affected source code places.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 fs/isofs/inode.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
index 2d3f090944df..db43f52f2268 100644
--- a/fs/isofs/inode.c
+++ b/fs/isofs/inode.c
@@ -96,7 +96,7 @@ static int __init init_inodecache(void)
 					0, (SLAB_RECLAIM_ACCOUNT|
 					SLAB_MEM_SPREAD|SLAB_ACCOUNT),
 					init_once);
-	if (isofs_inode_cachep = NULL)
+	if (!isofs_inode_cachep)
 		return -ENOMEM;
 	return 0;
 }
@@ -678,7 +678,7 @@ static int isofs_fill_super(struct super_block *s, void *data, int silent)
 			if (isonum_711(vdp->type) = ISO_VD_END)
 				break;
 			if (isonum_711(vdp->type) = ISO_VD_PRIMARY) {
-				if (pri = NULL) {
+				if (!pri) {
 					pri = (struct iso_primary_descriptor *)vdp;
 					/* Save the buffer in case we need it ... */
 					pri_bh = bh;
@@ -742,7 +742,7 @@ static int isofs_fill_super(struct super_block *s, void *data, int silent)
 		goto out_freebh;
 	}
 
-	if (joliet_level && (pri = NULL || !opt.rock)) {
+	if (joliet_level && (!pri || !opt.rock)) {
 		/* This is the case of Joliet with the norock mount flag.
 		 * A disc with both Joliet and Rock Ridge is handled later
 		 */
@@ -1320,5 +1320,5 @@ static int isofs_read_inode(struct inode *inode, int relocated)
 		int frag1 = bufsize - offset;
 
 		tmpde = kmalloc(de_len, GFP_KERNEL);
-		if (tmpde = NULL) {
+		if (!tmpde) {
 			ret = -ENOMEM;
-- 
2.14.0

[PATCH 3/4] isofs: One check less in isofs_read_inode() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 19 Aug 2017 08:10:40 +0200

* Adjust jump targets.

* Avoid a repeated check for the local variable "bh" after
  a memory allocation failure in this function.

* Delete an initialisation for the local variable "ret"
  which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 fs/isofs/inode.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
index db43f52f2268..f6d6c2ca8723 100644
--- a/fs/isofs/inode.c
+++ b/fs/isofs/inode.c
@@ -1304,7 +1304,7 @@ static int isofs_read_inode(struct inode *inode, int relocated)
 	unsigned int de_len;
 	unsigned long offset;
 	struct iso_inode_info *ei = ISOFS_I(inode);
-	int ret = -EIO;
+	int ret;
 
 	block = ei->i_iget5_block;
 	bh = sb_bread(inode->i_sb, block);
@@ -1322,7 +1322,7 @@ static int isofs_read_inode(struct inode *inode, int relocated)
 		tmpde = kmalloc(de_len, GFP_KERNEL);
 		if (!tmpde) {
 			ret = -ENOMEM;
-			goto fail;
+			goto release_bh;
 		}
 		memcpy(tmpde, bh->b_data + offset, frag1);
 		brelse(bh);
@@ -1377,7 +1377,7 @@ static int isofs_read_inode(struct inode *inode, int relocated)
 	if (de->flags[-high_sierra] & 0x80) {
 		ret = isofs_read_level3_size(inode);
 		if (ret < 0)
-			goto fail;
+			goto free_tmp;
 		ret = -EIO;
 	} else {
 		ei->i_next_section_block = 0;
@@ -1475,16 +1475,17 @@ static int isofs_read_inode(struct inode *inode, int relocated)
 		init_special_inode(inode, inode->i_mode, inode->i_rdev);
 
 	ret = 0;
-out:
+free_tmp:
 	kfree(tmpde);
 	if (bh)
+release_bh:
 		brelse(bh);
 	return ret;
 
 out_badread:
 	printk(KERN_WARNING "ISOFS: unable to read i-node block\n");
-fail:
-	goto out;
+	ret = -EIO;
+	goto free_tmp;
 }
 
 struct isofs_iget5_callback_data {
-- 
2.14.0

[PATCH 3/4] isofs: One check less in isofs_read_inode() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 19 Aug 2017 08:10:40 +0200

* Adjust jump targets.

* Avoid a repeated check for the local variable "bh" after
  a memory allocation failure in this function.

* Delete an initialisation for the local variable "ret"
  which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 fs/isofs/inode.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
index db43f52f2268..f6d6c2ca8723 100644
--- a/fs/isofs/inode.c
+++ b/fs/isofs/inode.c
@@ -1304,7 +1304,7 @@ static int isofs_read_inode(struct inode *inode, int relocated)
 	unsigned int de_len;
 	unsigned long offset;
 	struct iso_inode_info *ei = ISOFS_I(inode);
-	int ret = -EIO;
+	int ret;
 
 	block = ei->i_iget5_block;
 	bh = sb_bread(inode->i_sb, block);
@@ -1322,7 +1322,7 @@ static int isofs_read_inode(struct inode *inode, int relocated)
 		tmpde = kmalloc(de_len, GFP_KERNEL);
 		if (!tmpde) {
 			ret = -ENOMEM;
-			goto fail;
+			goto release_bh;
 		}
 		memcpy(tmpde, bh->b_data + offset, frag1);
 		brelse(bh);
@@ -1377,7 +1377,7 @@ static int isofs_read_inode(struct inode *inode, int relocated)
 	if (de->flags[-high_sierra] & 0x80) {
 		ret = isofs_read_level3_size(inode);
 		if (ret < 0)
-			goto fail;
+			goto free_tmp;
 		ret = -EIO;
 	} else {
 		ei->i_next_section_block = 0;
@@ -1475,16 +1475,17 @@ static int isofs_read_inode(struct inode *inode, int relocated)
 		init_special_inode(inode, inode->i_mode, inode->i_rdev);
 
 	ret = 0;
-out:
+free_tmp:
 	kfree(tmpde);
 	if (bh)
+release_bh:
 		brelse(bh);
 	return ret;
 
 out_badread:
 	printk(KERN_WARNING "ISOFS: unable to read i-node block\n");
-fail:
-	goto out;
+	ret = -EIO;
+	goto free_tmp;
 }
 
 struct isofs_iget5_callback_data {
-- 
2.14.0

[PATCH 4/4] isofs: Delete an unnecessary variable initialisation in isofs_read_inode()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 19 Aug 2017 08:13:46 +0200

The local variable "bh" will be set to an appropriate pointer a bit later.
Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 fs/isofs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
index f6d6c2ca8723..3a33b3db712a 100644
--- a/fs/isofs/inode.c
+++ b/fs/isofs/inode.c
@@ -1298,7 +1298,7 @@ static int isofs_read_inode(struct inode *inode, int relocated)
 	unsigned long bufsize = ISOFS_BUFFER_SIZE(inode);
 	unsigned long block;
 	int high_sierra = sbi->s_high_sierra;
-	struct buffer_head *bh = NULL;
+	struct buffer_head *bh;
 	struct iso_directory_record *de;
 	struct iso_directory_record *tmpde = NULL;
 	unsigned int de_len;
-- 
2.14.0

[PATCH 4/4] isofs: Delete an unnecessary variable initialisation in isofs_read_inode()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 19 Aug 2017 08:13:46 +0200

The local variable "bh" will be set to an appropriate pointer a bit later.
Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 fs/isofs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
index f6d6c2ca8723..3a33b3db712a 100644
--- a/fs/isofs/inode.c
+++ b/fs/isofs/inode.c
@@ -1298,7 +1298,7 @@ static int isofs_read_inode(struct inode *inode, int relocated)
 	unsigned long bufsize = ISOFS_BUFFER_SIZE(inode);
 	unsigned long block;
 	int high_sierra = sbi->s_high_sierra;
-	struct buffer_head *bh = NULL;
+	struct buffer_head *bh;
 	struct iso_directory_record *de;
 	struct iso_directory_record *tmpde = NULL;
 	unsigned int de_len;
-- 
2.14.0

Re: [PATCH 1/4] isofs: Delete an error message for a failed memory allocation in isofs_read_inode()

[Jan Kara]

On Sat 19-08-17 09:15:57, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 18 Aug 2017 21:41:24 +0200
> 
> Omit an extra message for a memory allocation failure in this function.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Thanks. Applied to my tree.

								Honza

> ---
>  fs/isofs/inode.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
> index 217a5e7815da..2d3f090944df 100644
> --- a/fs/isofs/inode.c
> +++ b/fs/isofs/inode.c
> @@ -1324,4 +1324,3 @@ static int isofs_read_inode(struct inode *inode, int relocated)
> -			printk(KERN_INFO "%s: out of memory\n", __func__);
>  			ret = -ENOMEM;
>  			goto fail;
>  		}
> -- 
> 2.14.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/4] isofs: Delete an error message for a failed memory allocation in isofs_read_inode()

[Jan Kara]

On Sat 19-08-17 09:15:57, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 18 Aug 2017 21:41:24 +0200
> 
> Omit an extra message for a memory allocation failure in this function.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Thanks. Applied to my tree.

								Honza

> ---
>  fs/isofs/inode.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
> index 217a5e7815da..2d3f090944df 100644
> --- a/fs/isofs/inode.c
> +++ b/fs/isofs/inode.c
> @@ -1324,4 +1324,3 @@ static int isofs_read_inode(struct inode *inode, int relocated)
> -			printk(KERN_INFO "%s: out of memory\n", __func__);
>  			ret = -ENOMEM;
>  			goto fail;
>  		}
> -- 
> 2.14.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/4] isofs: Adjust four checks for null pointers

[Jan Kara]

On Sat 19-08-17 09:17:28, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 18 Aug 2017 21:47:14 +0200
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> The script “checkpatch.pl” pointed information out like the following.
> 
> Comparison to NULL could be written !…
> 
> Thus fix the affected source code places.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Thanks. Applied to my tree.

								Honza


> ---
>  fs/isofs/inode.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
> index 2d3f090944df..db43f52f2268 100644
> --- a/fs/isofs/inode.c
> +++ b/fs/isofs/inode.c
> @@ -96,7 +96,7 @@ static int __init init_inodecache(void)
>  					0, (SLAB_RECLAIM_ACCOUNT|
>  					SLAB_MEM_SPREAD|SLAB_ACCOUNT),
>  					init_once);
> -	if (isofs_inode_cachep == NULL)
> +	if (!isofs_inode_cachep)
>  		return -ENOMEM;
>  	return 0;
>  }
> @@ -678,7 +678,7 @@ static int isofs_fill_super(struct super_block *s, void *data, int silent)
>  			if (isonum_711(vdp->type) == ISO_VD_END)
>  				break;
>  			if (isonum_711(vdp->type) == ISO_VD_PRIMARY) {
> -				if (pri == NULL) {
> +				if (!pri) {
>  					pri = (struct iso_primary_descriptor *)vdp;
>  					/* Save the buffer in case we need it ... */
>  					pri_bh = bh;
> @@ -742,7 +742,7 @@ static int isofs_fill_super(struct super_block *s, void *data, int silent)
>  		goto out_freebh;
>  	}
>  
> -	if (joliet_level && (pri == NULL || !opt.rock)) {
> +	if (joliet_level && (!pri || !opt.rock)) {
>  		/* This is the case of Joliet with the norock mount flag.
>  		 * A disc with both Joliet and Rock Ridge is handled later
>  		 */
> @@ -1320,5 +1320,5 @@ static int isofs_read_inode(struct inode *inode, int relocated)
>  		int frag1 = bufsize - offset;
>  
>  		tmpde = kmalloc(de_len, GFP_KERNEL);
> -		if (tmpde == NULL) {
> +		if (!tmpde) {
>  			ret = -ENOMEM;
> -- 
> 2.14.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/4] isofs: Adjust four checks for null pointers

[Jan Kara]

On Sat 19-08-17 09:17:28, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 18 Aug 2017 21:47:14 +0200
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> The script “checkpatch.pl” pointed information out like the following.
> 
> Comparison to NULL could be written !…
> 
> Thus fix the affected source code places.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Thanks. Applied to my tree.

								Honza


> ---
>  fs/isofs/inode.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
> index 2d3f090944df..db43f52f2268 100644
> --- a/fs/isofs/inode.c
> +++ b/fs/isofs/inode.c
> @@ -96,7 +96,7 @@ static int __init init_inodecache(void)
>  					0, (SLAB_RECLAIM_ACCOUNT|
>  					SLAB_MEM_SPREAD|SLAB_ACCOUNT),
>  					init_once);
> -	if (isofs_inode_cachep = NULL)
> +	if (!isofs_inode_cachep)
>  		return -ENOMEM;
>  	return 0;
>  }
> @@ -678,7 +678,7 @@ static int isofs_fill_super(struct super_block *s, void *data, int silent)
>  			if (isonum_711(vdp->type) = ISO_VD_END)
>  				break;
>  			if (isonum_711(vdp->type) = ISO_VD_PRIMARY) {
> -				if (pri = NULL) {
> +				if (!pri) {
>  					pri = (struct iso_primary_descriptor *)vdp;
>  					/* Save the buffer in case we need it ... */
>  					pri_bh = bh;
> @@ -742,7 +742,7 @@ static int isofs_fill_super(struct super_block *s, void *data, int silent)
>  		goto out_freebh;
>  	}
>  
> -	if (joliet_level && (pri = NULL || !opt.rock)) {
> +	if (joliet_level && (!pri || !opt.rock)) {
>  		/* This is the case of Joliet with the norock mount flag.
>  		 * A disc with both Joliet and Rock Ridge is handled later
>  		 */
> @@ -1320,5 +1320,5 @@ static int isofs_read_inode(struct inode *inode, int relocated)
>  		int frag1 = bufsize - offset;
>  
>  		tmpde = kmalloc(de_len, GFP_KERNEL);
> -		if (tmpde = NULL) {
> +		if (!tmpde) {
>  			ret = -ENOMEM;
> -- 
> 2.14.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 3/4] isofs: One check less in isofs_read_inode() after error detection

[Jan Kara]

On Sat 19-08-17 09:18:42, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 19 Aug 2017 08:10:40 +0200
> 
> * Adjust jump targets.
> 
> * Avoid a repeated check for the local variable "bh" after
>   a memory allocation failure in this function.
> 
> * Delete an initialisation for the local variable "ret"
>   which became unnecessary with this refactoring.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

I agree that

fail:
        goto out;

in the original code is awkward and we can get rid of it. However renaming
labels is IMO pointless and

>  	if (bh)
> +release_bh:
>  		brelse(bh);
>  	return ret;

is just disgusting.

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 3/4] isofs: One check less in isofs_read_inode() after error detection

[Jan Kara]

On Sat 19-08-17 09:18:42, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 19 Aug 2017 08:10:40 +0200
> 
> * Adjust jump targets.
> 
> * Avoid a repeated check for the local variable "bh" after
>   a memory allocation failure in this function.
> 
> * Delete an initialisation for the local variable "ret"
>   which became unnecessary with this refactoring.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

I agree that

fail:
        goto out;

in the original code is awkward and we can get rid of it. However renaming
labels is IMO pointless and

>  	if (bh)
> +release_bh:
>  		brelse(bh);
>  	return ret;

is just disgusting.

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 4/4] isofs: Delete an unnecessary variable initialisation in isofs_read_inode()

[Jan Kara]

On Sat 19-08-17 09:19:34, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 19 Aug 2017 08:13:46 +0200
> 
> The local variable "bh" will be set to an appropriate pointer a bit later.
> Thus omit the explicit initialisation at the beginning.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Applied to my tree. Thanks.

								Honza
> ---
>  fs/isofs/inode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
> index f6d6c2ca8723..3a33b3db712a 100644
> --- a/fs/isofs/inode.c
> +++ b/fs/isofs/inode.c
> @@ -1298,7 +1298,7 @@ static int isofs_read_inode(struct inode *inode, int relocated)
>  	unsigned long bufsize = ISOFS_BUFFER_SIZE(inode);
>  	unsigned long block;
>  	int high_sierra = sbi->s_high_sierra;
> -	struct buffer_head *bh = NULL;
> +	struct buffer_head *bh;
>  	struct iso_directory_record *de;
>  	struct iso_directory_record *tmpde = NULL;
>  	unsigned int de_len;
> -- 
> 2.14.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 4/4] isofs: Delete an unnecessary variable initialisation in isofs_read_inode()

[Jan Kara]

On Sat 19-08-17 09:19:34, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 19 Aug 2017 08:13:46 +0200
> 
> The local variable "bh" will be set to an appropriate pointer a bit later.
> Thus omit the explicit initialisation at the beginning.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Applied to my tree. Thanks.

								Honza
> ---
>  fs/isofs/inode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
> index f6d6c2ca8723..3a33b3db712a 100644
> --- a/fs/isofs/inode.c
> +++ b/fs/isofs/inode.c
> @@ -1298,7 +1298,7 @@ static int isofs_read_inode(struct inode *inode, int relocated)
>  	unsigned long bufsize = ISOFS_BUFFER_SIZE(inode);
>  	unsigned long block;
>  	int high_sierra = sbi->s_high_sierra;
> -	struct buffer_head *bh = NULL;
> +	struct buffer_head *bh;
>  	struct iso_directory_record *de;
>  	struct iso_directory_record *tmpde = NULL;
>  	unsigned int de_len;
> -- 
> 2.14.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: isofs: One check less in isofs_read_inode() after error detection

[SF Markus Elfring]

> I agree that
> 
> fail:
>         goto out;
> 
> in the original code is awkward and we can get rid of it.

How would you like to change this place instead?


> However renaming labels is IMO pointless and
> 
>>  	if (bh)
>> +release_bh:
>>  		brelse(bh);
>>  	return ret;
> 
> is just disgusting.

I know that it can be occasionally harder to achieve the desired consensus.

Regards,
Markus

Re: isofs: One check less in isofs_read_inode() after error detection

[SF Markus Elfring]

> I agree that
> 
> fail:
>         goto out;
> 
> in the original code is awkward and we can get rid of it.

How would you like to change this place instead?


> However renaming labels is IMO pointless and
> 
>>  	if (bh)
>> +release_bh:
>>  		brelse(bh);
>>  	return ret;
> 
> is just disgusting.

I know that it can be occasionally harder to achieve the desired consensus.

Regards,
Markus

Re: isofs: One check less in isofs_read_inode() after error detection

[Jan Kara]

On Mon 21-08-17 16:18:14, SF Markus Elfring wrote:
> > I agree that
> > 
> > fail:
> >         goto out;
> > 
> > in the original code is awkward and we can get rid of it.
> 
> How would you like to change this place instead?

Just do 'goto out' directly.

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: isofs: One check less in isofs_read_inode() after error detection

[Jan Kara]

On Mon 21-08-17 16:18:14, SF Markus Elfring wrote:
> > I agree that
> > 
> > fail:
> >         goto out;
> > 
> > in the original code is awkward and we can get rid of it.
> 
> How would you like to change this place instead?

Just do 'goto out' directly.

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR