[patch] ocfs2: cleanup error handling in o2hb_alloc_hb_set()

[patch] ocfs2: cleanup error handling in o2hb_alloc_hb_set()

[Dan Carpenter]

If "ret" is NULL, then "hs" is also NULL, so there is no need to free
it.  config_group_init_type_name() can't fail if the name ("heartbeat"
in this case) is less than CONFIGFS_ITEM_NAME_LEN (20) characters long
so we can just remove this error handling code.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/fs/ocfs2/cluster/heartbeat.c b/fs/ocfs2/cluster/heartbeat.c
index f3f2d95..0bca51b 100644
--- a/fs/ocfs2/cluster/heartbeat.c
+++ b/fs/ocfs2/cluster/heartbeat.c
@@ -2306,20 +2306,14 @@ static struct config_item_type o2hb_heartbeat_group_type = {
 struct config_group *o2hb_alloc_hb_set(void)
 {
 	struct o2hb_heartbeat_group *hs = NULL;
-	struct config_group *ret = NULL;
 
 	hs = kzalloc(sizeof(struct o2hb_heartbeat_group), GFP_KERNEL);
 	if (hs = NULL)
-		goto out;
+		return NULL;
 
 	config_group_init_type_name(&hs->hs_group, "heartbeat",
 				    &o2hb_heartbeat_group_type);
-
-	ret = &hs->hs_group;
-out:
-	if (ret = NULL)
-		kfree(hs);
-	return ret;
+	return &hs->hs_group;
 }
 
 void o2hb_free_hb_set(struct config_group *group)

Re: [patch] ocfs2: cleanup error handling in o2hb_alloc_hb_set()

[Sunil Mushran]

hmm... I would say NAK because config_group_item_type_name() could
change in the future. And there is nothing wrong with the current
code.

On 02/13/2012 05:50 AM, Dan Carpenter wrote:
> If "ret" is NULL, then "hs" is also NULL, so there is no need to free
> it.  config_group_init_type_name() can't fail if the name ("heartbeat"
> in this case) is less than CONFIGFS_ITEM_NAME_LEN (20) characters long
> so we can just remove this error handling code.
>
> Signed-off-by: Dan Carpenter<dan.carpenter@oracle.com>
>
> diff --git a/fs/ocfs2/cluster/heartbeat.c b/fs/ocfs2/cluster/heartbeat.c
> index f3f2d95..0bca51b 100644
> --- a/fs/ocfs2/cluster/heartbeat.c
> +++ b/fs/ocfs2/cluster/heartbeat.c
> @@ -2306,20 +2306,14 @@ static struct config_item_type o2hb_heartbeat_group_type = {
>   struct config_group *o2hb_alloc_hb_set(void)
>   {
>   	struct o2hb_heartbeat_group *hs = NULL;
> -	struct config_group *ret = NULL;
>
>   	hs = kzalloc(sizeof(struct o2hb_heartbeat_group), GFP_KERNEL);
>   	if (hs = NULL)
> -		goto out;
> +		return NULL;
>
>   	config_group_init_type_name(&hs->hs_group, "heartbeat",
>   				&o2hb_heartbeat_group_type);
> -
> -	ret =&hs->hs_group;
> -out:
> -	if (ret = NULL)
> -		kfree(hs);
> -	return ret;
> +	return&hs->hs_group;
>   }
>
>   void o2hb_free_hb_set(struct config_group *group)

Re: [patch] ocfs2: cleanup error handling in o2hb_alloc_hb_set()

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 715 bytes --]

On Mon, Feb 13, 2012 at 11:39:27AM -0800, Sunil Mushran wrote:
> hmm... I would say NAK because config_group_item_type_name() could
> change in the future. And there is nothing wrong with the current
> code.

The error handling isn't correct because checking "&hs->hs_group"
for NULL doesn't work.  "hs" and "&hs->hs_group" are the same
address and we checked "hs" already.  If we wanted to check for
allocation errors, then we would need to change the check to:

        if (!hs->hs_group.cg_item.ci_name)
		kfree(hs);

But that's not how the function is supposed to be used.  The example
code in Documentation/filesystems/configfs/configfs_example_explicit.c
doesn't have error handling.

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [patch] ocfs2: cleanup error handling in o2hb_alloc_hb_set()

[Sunil Mushran]

On 02/13/2012 12:08 PM, Dan Carpenter wrote:
> On Mon, Feb 13, 2012 at 11:39:27AM -0800, Sunil Mushran wrote:
>> hmm... I would say NAK because config_group_item_type_name() could
>> change in the future. And there is nothing wrong with the current
>> code.
>
> The error handling isn't correct because checking "&hs->hs_group"
> for NULL doesn't work.  "hs" and "&hs->hs_group" are the same
> address and we checked "hs" already.  If we wanted to check for
> allocation errors, then we would need to change the check to:
>
>          if (!hs->hs_group.cg_item.ci_name)
> 		kfree(hs);
>
> But that's not how the function is supposed to be used.  The example
> code in Documentation/filesystems/configfs/configfs_example_explicit.c
> doesn't have error handling.


True. The error handling is not ideal but only so because it
has not been specified. A better fix would be to first clarify that
in config_group_item_type_name().

Re: [patch] ocfs2: cleanup error handling in o2hb_alloc_hb_set()

[Joel Becker]

On Mon, Feb 13, 2012 at 04:50:47PM +0300, Dan Carpenter wrote:
> If "ret" is NULL, then "hs" is also NULL, so there is no need to free
> it.  config_group_init_type_name() can't fail if the name ("heartbeat"
> in this case) is less than CONFIGFS_ITEM_NAME_LEN (20) characters long
> so we can just remove this error handling code.

	Is there a problem we're fixing here?  We can make all sorts of
arguments about single-exit functions vs immediate returns, but if there
isn't a problem, I'm not sure what we gain by code churn.

Joel

> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/fs/ocfs2/cluster/heartbeat.c b/fs/ocfs2/cluster/heartbeat.c
> index f3f2d95..0bca51b 100644
> --- a/fs/ocfs2/cluster/heartbeat.c
> +++ b/fs/ocfs2/cluster/heartbeat.c
> @@ -2306,20 +2306,14 @@ static struct config_item_type o2hb_heartbeat_group_type = {
>  struct config_group *o2hb_alloc_hb_set(void)
>  {
>  	struct o2hb_heartbeat_group *hs = NULL;
> -	struct config_group *ret = NULL;
>  
>  	hs = kzalloc(sizeof(struct o2hb_heartbeat_group), GFP_KERNEL);
>  	if (hs = NULL)
> -		goto out;
> +		return NULL;
>  
>  	config_group_init_type_name(&hs->hs_group, "heartbeat",
>  				    &o2hb_heartbeat_group_type);
> -
> -	ret = &hs->hs_group;
> -out:
> -	if (ret = NULL)
> -		kfree(hs);
> -	return ret;
> +	return &hs->hs_group;
>  }
>  
>  void o2hb_free_hb_set(struct config_group *group)

-- 

"Copy from one, it's plagiarism; copy from two, it's research."
        - Wilson Mizner

			http://www.jlbec.org/
			jlbec@evilplan.org

Re: [patch] ocfs2: cleanup error handling in o2hb_alloc_hb_set()

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 977 bytes --]

On Mon, Feb 13, 2012 at 12:04:09PM -0800, Joel Becker wrote:
> On Mon, Feb 13, 2012 at 04:50:47PM +0300, Dan Carpenter wrote:
> > If "ret" is NULL, then "hs" is also NULL, so there is no need to free
> > it.  config_group_init_type_name() can't fail if the name ("heartbeat"
> > in this case) is less than CONFIGFS_ITEM_NAME_LEN (20) characters long
> > so we can just remove this error handling code.
> 
> 	Is there a problem we're fixing here?  We can make all sorts of
> arguments about single-exit functions vs immediate returns, but if there
> isn't a problem, I'm not sure what we gain by code churn.

It's a static checker thing.  It's either an unneeded NULL check or
a check on the wrong variable depending on how you look at it.
Recently, I've been sending a lot of patches to remove unneeded NULL
checks for static checkers.  I try to fix the mistakes that are
harmless so that the real bugs aren't drowned out in noise.

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [patch] ocfs2: cleanup error handling in o2hb_alloc_hb_set()

[Sunil Mushran]

On 02/13/2012 12:29 PM, Dan Carpenter wrote:
> On Mon, Feb 13, 2012 at 12:04:09PM -0800, Joel Becker wrote:
>> On Mon, Feb 13, 2012 at 04:50:47PM +0300, Dan Carpenter wrote:
>>> If "ret" is NULL, then "hs" is also NULL, so there is no need to free
>>> it.  config_group_init_type_name() can't fail if the name ("heartbeat"
>>> in this case) is less than CONFIGFS_ITEM_NAME_LEN (20) characters long
>>> so we can just remove this error handling code.
>>
>> 	Is there a problem we're fixing here?  We can make all sorts of
>> arguments about single-exit functions vs immediate returns, but if there
>> isn't a problem, I'm not sure what we gain by code churn.
>
> It's a static checker thing.  It's either an unneeded NULL check or
> a check on the wrong variable depending on how you look at it.
> Recently, I've been sending a lot of patches to remove unneeded NULL
> checks for static checkers.  I try to fix the mistakes that are
> harmless so that the real bugs aren't drowned out in noise.

In that case, ACK.