[lustre-devel] [PATCH 0/3] staging: lustre: Deletion of some unnecessary checks

[lustre-devel] [PATCH 0/3] staging: lustre: Deletion of some unnecessary checks

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 5 Nov 2015 13:03:33 +0100

Further update suggestions were taken into account after a patch
was applied from static source code analysis.

Markus Elfring (3):
  Delete unnecessary checks before two function calls
  Delete an unnecessary variable initialisation in class_register_type()
  Less function calls in class_register_type() after error detection

 drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c | 10 +++-----
 drivers/staging/lustre/lustre/lmv/lmv_obd.c     |  5 ++--
 drivers/staging/lustre/lustre/lov/lov_obd.c     |  4 +--
 drivers/staging/lustre/lustre/obdclass/genops.c | 34 ++++++++++++++-----------
 4 files changed, 25 insertions(+), 28 deletions(-)

-- 
2.6.2

[lustre-devel] [PATCH 1/3] staging: lustre: Delete unnecessary checks before two function calls

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 5 Nov 2015 10:18:45 +0100

The functions kobject_put() and kset_unregister() test whether their
argument is NULL and then return immediately.
Thus the tests around their calls are not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c | 10 +++-------
 drivers/staging/lustre/lustre/lmv/lmv_obd.c     |  5 ++---
 drivers/staging/lustre/lustre/lov/lov_obd.c     |  4 +---
 drivers/staging/lustre/lustre/obdclass/genops.c |  6 ++----
 4 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c b/drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c
index ca11511..e67e84b 100644
--- a/drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c
+++ b/drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c
@@ -1062,13 +1062,9 @@ static int ldlm_cleanup(void)
 	if (ldlm_state->ldlm_cb_service != NULL)
 		ptlrpc_unregister_service(ldlm_state->ldlm_cb_service);
 
-	if (ldlm_ns_kset)
-		kset_unregister(ldlm_ns_kset);
-	if (ldlm_svc_kset)
-		kset_unregister(ldlm_svc_kset);
-	if (ldlm_kobj)
-		kobject_put(ldlm_kobj);
-
+	kset_unregister(ldlm_ns_kset);
+	kset_unregister(ldlm_svc_kset);
+	kobject_put(ldlm_kobj);
 	ldlm_debugfs_cleanup();
 
 	kfree(ldlm_state);
diff --git a/drivers/staging/lustre/lustre/lmv/lmv_obd.c b/drivers/staging/lustre/lustre/lmv/lmv_obd.c
index 635a93c..c3c8e8c 100644
--- a/drivers/staging/lustre/lustre/lmv/lmv_obd.c
+++ b/drivers/staging/lustre/lustre/lmv/lmv_obd.c
@@ -240,7 +240,7 @@ static int lmv_connect(const struct lu_env *env,
 	if (data->ocd_connect_flags & OBD_CONNECT_REAL)
 		rc = lmv_check_connect(obd);
 
-	if (rc && lmv->lmv_tgts_kobj)
+	if (rc)
 		kobject_put(lmv->lmv_tgts_kobj);
 
 	return rc;
@@ -646,8 +646,7 @@ static int lmv_disconnect(struct obd_export *exp)
 		lmv_disconnect_mdc(obd, lmv->tgts[i]);
 	}
 
-	if (lmv->lmv_tgts_kobj)
-		kobject_put(lmv->lmv_tgts_kobj);
+	kobject_put(lmv->lmv_tgts_kobj);
 
 out_local:
 	/*
diff --git a/drivers/staging/lustre/lustre/lov/lov_obd.c b/drivers/staging/lustre/lustre/lov/lov_obd.c
index 7abe484..910c62c 100644
--- a/drivers/staging/lustre/lustre/lov/lov_obd.c
+++ b/drivers/staging/lustre/lustre/lov/lov_obd.c
@@ -109,9 +109,7 @@ static void lov_putref(struct obd_device *obd)
 			__lov_del_obd(obd, tgt);
 		}
 
-		if (lov->lov_tgts_kobj)
-			kobject_put(lov->lov_tgts_kobj);
-
+		kobject_put(lov->lov_tgts_kobj);
 	} else {
 		mutex_unlock(&lov->lov_lock);
 	}
diff --git a/drivers/staging/lustre/lustre/obdclass/genops.c b/drivers/staging/lustre/lustre/obdclass/genops.c
index 6477aeb..a1cf8e1 100644
--- a/drivers/staging/lustre/lustre/obdclass/genops.c
+++ b/drivers/staging/lustre/lustre/obdclass/genops.c
@@ -216,8 +216,7 @@ int class_register_type(struct obd_ops *dt_ops, struct md_ops *md_ops,
 	return 0;
 
  failed:
-	if (type->typ_kobj)
-		kobject_put(type->typ_kobj);
+	kobject_put(type->typ_kobj);
 	kfree(type->typ_name);
 	kfree(type->typ_md_ops);
 	kfree(type->typ_dt_ops);
@@ -244,8 +243,7 @@ int class_unregister_type(const char *name)
 		return -EBUSY;
 	}
 
-	if (type->typ_kobj)
-		kobject_put(type->typ_kobj);
+	kobject_put(type->typ_kobj);
 
 	if (!IS_ERR_OR_NULL(type->typ_debugfs_entry))
 		ldebugfs_remove(&type->typ_debugfs_entry);
-- 
2.6.2

[lustre-devel] [PATCH 2/3] staging: lustre: Delete an unnecessary variable initialisation in class_register_type()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 5 Nov 2015 10:55:16 +0100

The variable "rc" will be eventually set to an error return code in the
class_register_type() function.
Thus let us omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/lustre/lustre/obdclass/genops.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/lustre/lustre/obdclass/genops.c b/drivers/staging/lustre/lustre/obdclass/genops.c
index a1cf8e1..acb86f0 100644
--- a/drivers/staging/lustre/lustre/obdclass/genops.c
+++ b/drivers/staging/lustre/lustre/obdclass/genops.c
@@ -155,7 +155,7 @@ int class_register_type(struct obd_ops *dt_ops, struct md_ops *md_ops,
 			struct lu_device_type *ldt)
 {
 	struct obd_type *type;
-	int rc = 0;
+	int rc;
 
 	/* sanity check */
 	LASSERT(strnlen(name, CLASS_MAX_NAME) < CLASS_MAX_NAME);
-- 
2.6.2

[lustre-devel] [PATCH 3/3] staging: lustre: Less function calls in class_register_type() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 5 Nov 2015 12:48:58 +0100

The functions "kfree" and "kobject_put" were called in a few cases by the
function "class_register_type" during error handling even if the passed
variable contained a null pointer.

This implementation detail could be improved by the adjustment of
jump targets.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/lustre/lustre/obdclass/genops.c | 26 +++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/lustre/lustre/obdclass/genops.c b/drivers/staging/lustre/lustre/obdclass/genops.c
index acb86f0..4d99a39 100644
--- a/drivers/staging/lustre/lustre/obdclass/genops.c
+++ b/drivers/staging/lustre/lustre/obdclass/genops.c
@@ -171,13 +171,16 @@ int class_register_type(struct obd_ops *dt_ops, struct md_ops *md_ops,
 		return rc;
 
 	type->typ_dt_ops = kzalloc(sizeof(*type->typ_dt_ops), GFP_NOFS);
+	if (!type->typ_dt_ops)
+		goto free_type;
+
 	type->typ_md_ops = kzalloc(sizeof(*type->typ_md_ops), GFP_NOFS);
-	type->typ_name = kzalloc(strlen(name) + 1, GFP_NOFS);
+	if (!type->typ_md_ops)
+		goto free_dt_ops;
 
-	if (!type->typ_dt_ops ||
-	    !type->typ_md_ops ||
-	    !type->typ_name)
-		goto failed;
+	type->typ_name = kzalloc(strlen(name) + 1, GFP_NOFS);
+	if (!type->typ_name)
+		goto free_md_ops;
 
 	*(type->typ_dt_ops) = *dt_ops;
 	/* md_ops is optional */
@@ -193,20 +196,20 @@ int class_register_type(struct obd_ops *dt_ops, struct md_ops *md_ops,
 		rc = type->typ_debugfs_entry ? PTR_ERR(type->typ_debugfs_entry)
 					     : -ENOMEM;
 		type->typ_debugfs_entry = NULL;
-		goto failed;
+		goto free_name;
 	}
 
 	type->typ_kobj = kobject_create_and_add(type->typ_name, lustre_kobj);
 	if (!type->typ_kobj) {
 		rc = -ENOMEM;
-		goto failed;
+		goto free_name;
 	}
 
 	if (ldt != NULL) {
 		type->typ_lu = ldt;
 		rc = lu_device_type_init(ldt);
 		if (rc != 0)
-			goto failed;
+			goto put_object;
 	}
 
 	spin_lock(&obd_types_lock);
@@ -214,12 +217,15 @@ int class_register_type(struct obd_ops *dt_ops, struct md_ops *md_ops,
 	spin_unlock(&obd_types_lock);
 
 	return 0;
-
- failed:
+put_object:
 	kobject_put(type->typ_kobj);
+free_name:
 	kfree(type->typ_name);
+free_md_ops:
 	kfree(type->typ_md_ops);
+free_dt_ops:
 	kfree(type->typ_dt_ops);
+free_type:
 	kfree(type);
 	return rc;
 }
-- 
2.6.2

[lustre-devel] [PATCH 1/3] staging: lustre: Delete unnecessary checks before two function calls

[Dan Carpenter]

I don't like these patches because relying on hidden sanity checks makes
the code harder to read.

I am CC'd on all these patches because of kernel-janitors and normally I
ignore them but this is drivers/staging so I am invested in this code.
Feel free to send them to other subsystems though.

regards,
dan carpenter

[lustre-devel] [PATCH 2/3] staging: lustre: Delete an unnecessary variable initialisation in class_register_type()

[Dan Carpenter]

Looks good.

regards,
dan carpenter

[lustre-devel] [PATCH 3/3] staging: lustre: Less function calls in class_register_type() after error detection

[Greg Kroah-Hartman]

On Thu, Nov 05, 2015 at 01:20:33PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 5 Nov 2015 12:48:58 +0100
> 
> The functions "kfree" and "kobject_put" were called in a few cases by the
> function "class_register_type" during error handling even if the passed
> variable contained a null pointer.
> 
> This implementation detail could be improved by the adjustment of
> jump targets.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/staging/lustre/lustre/obdclass/genops.c | 26 +++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)

Does not apply to my staging-next branch :(

[lustre-devel] [PATCH 3/3] staging: lustre: Less function calls in class_register_type() after error detection

[SF Markus Elfring]

Am 25.11.2015 um 17:39 schrieb Greg Kroah-Hartman:
> On Thu, Nov 05, 2015 at 01:20:33PM +0100, SF Markus Elfring wrote:
>> From: Markus Elfring <elfring@users.sourceforge.net>
>> Date: Thu, 5 Nov 2015 12:48:58 +0100
>>
>> The functions "kfree" and "kobject_put" were called in a few cases by the
>> function "class_register_type" during error handling even if the passed
>> variable contained a null pointer.
>>
>> This implementation detail could be improved by the adjustment of
>> jump targets.
>>
>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>> ---
>>  drivers/staging/lustre/lustre/obdclass/genops.c | 26 +++++++++++++++----------
>>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> Does not apply to my staging-next branch :(

I get also a result like the following together with the software "Linux next-20151126".   ;-)

elfring at Sonne:~/Projekte/Linux/next-patched> LANG=C git apply ~/Projekte/Bau/Linux/scripts/Coccinelle/deletions1/next/20151102/Flicken/0003-staging-lustre-Less-function-calls-in-class_register.patch
error: patch failed: drivers/staging/lustre/lustre/obdclass/genops.c:214
error: drivers/staging/lustre/lustre/obdclass/genops.c: patch does not apply


Do you try this update suggestion out without integrating the corresponding previous
update suggestion "Delete unnecessary checks before two function calls"
where I proposed to remove extra checks before a few calls of the function "kobject_put"
(which seems to matter for the patch hunk in the shown error message)?
https://lkml.org/lkml/2015/11/5/276
https://www.mail-archive.com/linux-kernel at vger.kernel.org/msg1013635.html

Would you like to reject the first update step from this patch series
so that I need to adapt my approach to your software design decision?

Regards,
Markus

[lustre-devel] [PATCH 3/3] staging: lustre: Less function calls in class_register_type() after error detection

[Greg Kroah-Hartman]

On Thu, Nov 26, 2015 at 08:08:28AM +0100, SF Markus Elfring wrote:
> Am 25.11.2015 um 17:39 schrieb Greg Kroah-Hartman:
> > On Thu, Nov 05, 2015 at 01:20:33PM +0100, SF Markus Elfring wrote:
> >> From: Markus Elfring <elfring@users.sourceforge.net>
> >> Date: Thu, 5 Nov 2015 12:48:58 +0100
> >>
> >> The functions "kfree" and "kobject_put" were called in a few cases by the
> >> function "class_register_type" during error handling even if the passed
> >> variable contained a null pointer.
> >>
> >> This implementation detail could be improved by the adjustment of
> >> jump targets.
> >>
> >> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> >> ---
> >>  drivers/staging/lustre/lustre/obdclass/genops.c | 26 +++++++++++++++----------
> >>  1 file changed, 16 insertions(+), 10 deletions(-)
> > 
> > Does not apply to my staging-next branch :(
> 
> I get also a result like the following together with the software "Linux next-20151126".   ;-)
> 
> elfring at Sonne:~/Projekte/Linux/next-patched> LANG=C git apply ~/Projekte/Bau/Linux/scripts/Coccinelle/deletions1/next/20151102/Flicken/0003-staging-lustre-Less-function-calls-in-class_register.patch
> error: patch failed: drivers/staging/lustre/lustre/obdclass/genops.c:214
> error: drivers/staging/lustre/lustre/obdclass/genops.c: patch does not apply
> 
> 
> Do you try this update suggestion out without integrating the corresponding previous
> update suggestion "Delete unnecessary checks before two function calls"
> where I proposed to remove extra checks before a few calls of the function "kobject_put"
> (which seems to matter for the patch hunk in the shown error message)?
> https://lkml.org/lkml/2015/11/5/276
> https://www.mail-archive.com/linux-kernel at vger.kernel.org/msg1013635.html

I guess so, I don't remember, I don't see any patches from you earlier
in my "todo" mbox.

> Would you like to reject the first update step from this patch series
> so that I need to adapt my approach to your software design decision?

I have no idea what you are talking about.  I have no recolection of
previous patches or conversations about your patches.

greg k-h

[lustre-devel] [PATCH 3/3] staging: lustre: Less function calls in class_register_type() after error detection

[SF Markus Elfring]

>> Do you try this update suggestion out without integrating the corresponding previous
>> update suggestion "Delete unnecessary checks before two function calls"
>> where I proposed to remove extra checks before a few calls of the function "kobject_put"
>> (which seems to matter for the patch hunk in the shown error message)?
>> https://lkml.org/lkml/2015/11/5/276
>> https://www.mail-archive.com/linux-kernel at vger.kernel.org/msg1013635.html
> 
> I guess so, I don't remember, I don't see any patches from you earlier
> in my "todo" mbox.

I am still waiting for further constructive feedback on a bunch
of my update suggestions which are derived from static source code analysis.

It can be the usual challenge to get a bit more attention for them.
Other software improvements will result in bigger effects than the source code
fine-tuning I propose, won't it?

I would like to acknowledge that changes like the following from this patch series
can still be applied together for the software "Linux next-20151126".
* 0001-staging-lustre-Delete-unnecessary-checks-before-two.patch
* 0003-staging-lustre-Less-function-calls-in-class_register.patch


>> Would you like to reject the first update step from this patch series
>> so that I need to adapt my approach to your software design decision?
> 
> I have no idea what you are talking about.  I have no recolection of
> previous patches or conversations about your patches.

* Dan Carpenter expressed his software design concerns around hidden sanity checks
  a few times.
  How do you think about to give the proposed changes another chance?

* Positive feedback is occasionally increasing by specific subsystem supporters
  and maintainers.
  How will our collaboration evolve further?


Do you want that I resend any mails/patches from my "waiting queue"?

Regards,
Markus

[lustre-devel] [PATCH 3/3] staging: lustre: Less function calls in class_register_type() after error detection

[Dan Carpenter]

These patches are often correct in the same way a stopped clock is
correct twice a day, but I reject the motivation/approach/patch
description.  Just because there is a sanity check does not mean we
should use it (ie, do an insane thing).  It hurts readability to hide
the NULL check.

On the other hand, half the time the NULL checks are superflous because
the pointer is never NULL.  The other half of the time the NULL checks
are there because the code uses one err style error handling.  Also the
error handling code should mirror the allocation code so I feel like it
should be:

	if (some_feature)
		foo = allocate();
	ret = frob();
	if (ret)
		goto free_foo;

free_foo:
	if (some_feature)  // <--- as opposed to if (foo) {
		free(foo);

So anyway I often don't like the original code, but the new code is
even worse and I have never heard a good motivation for these patches
besides that they were generated using a cool tool.  It's not a good
reason.  Other maintainers are accepting these patches so you are free
to send them there.

regards,
dan carpenter