ocfs2: cleanup error handling in o2hb_alloc_hb_set()

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<[email protected]>

fs/ocfs2/cluster/heartbeat.c +2 -8

index f3f2d95..0bca51b 100644
@@ -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)

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.

Quoted 3 lines · 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.

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.

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 <[email protected]>

fs/ocfs2/cluster/heartbeat.c +2 -8

index f3f2d95..0bca51b 100644
@@ -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)

Quoted 4 lines · 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.

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.

Quoted 9 lines · Joel Becker

Quoted 4 lines · 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.

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.