[lustre-devel] [PATCH 0/7] staging-Lustre: Fine-tuning for some function implementations

[lustre-devel] [PATCH 0/7] staging-Lustre: Fine-tuning for some function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 13 Dec 2015 14:40:14 +0100

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

Markus Elfring (7):
  Delete unnecessary goto statements in six functions
  Rename a jump label for ptlrpc_req_finished() calls
  Rename a jump label for a kfree(key) call
  Delete an unnecessary variable initialisation in mgc_process_recover_log()
  Less checks in mgc_process_recover_log() after error detection
  A few checks less in mgc_process_recover_log() after error detection
  Rename a jump label for module_put() calls

 drivers/staging/lustre/lustre/llite/file.c         |  26 ++---
 drivers/staging/lustre/lustre/llite/lloop.c        |   8 +-
 drivers/staging/lustre/lustre/llite/namei.c        |  13 +--
 drivers/staging/lustre/lustre/llite/xattr.c        |  20 ++--
 drivers/staging/lustre/lustre/mdc/mdc_request.c    | 124 ++++++++++-----------
 drivers/staging/lustre/lustre/mgc/mgc_request.c    |  53 ++++-----
 drivers/staging/lustre/lustre/osc/osc_request.c    |  52 ++++-----
 drivers/staging/lustre/lustre/ptlrpc/llog_client.c |  22 ++--
 8 files changed, 152 insertions(+), 166 deletions(-)

-- 
2.6.3

[lustre-devel] [PATCH 1/7] staging: lustre: Delete unnecessary goto statements in six functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 13 Dec 2015 09:30:47 +0100

Six goto statements referred to a source code position directly behind them.
Thus omit such unnecessary jumps.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/lustre/lustre/llite/namei.c     | 1 -
 drivers/staging/lustre/lustre/mdc/mdc_request.c | 7 -------
 2 files changed, 8 deletions(-)

diff --git a/drivers/staging/lustre/lustre/llite/namei.c b/drivers/staging/lustre/lustre/llite/namei.c
index 64db5e8..2113dd4 100644
--- a/drivers/staging/lustre/lustre/llite/namei.c
+++ b/drivers/staging/lustre/lustre/llite/namei.c
@@ -554,7 +554,6 @@ static struct dentry *ll_lookup_it(struct inode *parent, struct dentry *dentry,
 		retval = NULL;
 	else
 		retval = dentry;
-	goto out;
  out:
 	if (req)
 		ptlrpc_req_finished(req);
diff --git a/drivers/staging/lustre/lustre/mdc/mdc_request.c b/drivers/staging/lustre/lustre/mdc/mdc_request.c
index 294c050..920b1e9 100644
--- a/drivers/staging/lustre/lustre/mdc/mdc_request.c
+++ b/drivers/staging/lustre/lustre/mdc/mdc_request.c
@@ -1181,7 +1181,6 @@ static int mdc_ioc_hsm_progress(struct obd_export *exp,
 	ptlrpc_request_set_replen(req);
 
 	rc = mdc_queue_wait(req);
-	goto out;
 out:
 	ptlrpc_req_finished(req);
 	return rc;
@@ -1216,7 +1215,6 @@ static int mdc_ioc_hsm_ct_register(struct obd_import *imp, __u32 archives)
 	ptlrpc_request_set_replen(req);
 
 	rc = mdc_queue_wait(req);
-	goto out;
 out:
 	ptlrpc_req_finished(req);
 	return rc;
@@ -1282,7 +1280,6 @@ static int mdc_ioc_hsm_ct_unregister(struct obd_import *imp)
 	ptlrpc_request_set_replen(req);
 
 	rc = mdc_queue_wait(req);
-	goto out;
 out:
 	ptlrpc_req_finished(req);
 	return rc;
@@ -1362,8 +1359,6 @@ static int mdc_ioc_hsm_state_set(struct obd_export *exp,
 	ptlrpc_request_set_replen(req);
 
 	rc = mdc_queue_wait(req);
-	goto out;
-
 out:
 	ptlrpc_req_finished(req);
 	return rc;
@@ -1427,8 +1422,6 @@ static int mdc_ioc_hsm_request(struct obd_export *exp,
 	ptlrpc_request_set_replen(req);
 
 	rc = mdc_queue_wait(req);
-	goto out;
-
 out:
 	ptlrpc_req_finished(req);
 	return rc;
-- 
2.6.3

[lustre-devel] [PATCH 1/7] staging: lustre: Delete unnecessary goto statements in six functions

[Joe Perches]

On Sun, 2015-12-13 at 14:52 +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 13 Dec 2015 09:30:47 +0100
> 
> Six goto statements referred to a source code position directly behind them.
> Thus omit such unnecessary jumps.

I suggest you leave a blank line instead
of deleting the goto.

> diff --git a/drivers/staging/lustre/lustre/llite/namei.c b/drivers/staging/lustre/lustre/llite/namei.c
[]
> @@ -554,7 +554,6 @@ static struct dentry *ll_lookup_it(struct inode *parent, struct dentry *dentry,
> ?		retval = NULL;
> ?	else
> ?		retval = dentry;
> -	goto out;
> ? out:
> ?	if (req)
> ?		ptlrpc_req_finished(req);

etc.,,

[lustre-devel] [PATCH 1/7] staging: lustre: Delete unnecessary goto statements in six functions

[Dan Carpenter]

On Tue, Dec 15, 2015 at 06:27:56AM -0800, Joe Perches wrote:
> On Sun, 2015-12-13 at 14:52 +0100, SF Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Sun, 13 Dec 2015 09:30:47 +0100
> > 
> > Six goto statements referred to a source code position directly behind them.
> > Thus omit such unnecessary jumps.
> 
> I suggest you leave a blank line instead
> of deleting the goto.
> 

What is the point of the little bunny hop?

regards,
dan carpenter

[lustre-devel] [PATCH 1/7] staging: lustre: Delete unnecessary goto statements in six functions

[Joe Perches]

On Tue, 2015-12-15 at 17:41 +0300, Dan Carpenter wrote:
> On Tue, Dec 15, 2015 at 06:27:56AM -0800, Joe Perches wrote:
> > On Sun, 2015-12-13 at 14:52 +0100, SF Markus Elfring wrote:
> > > From: Markus Elfring <elfring@users.sourceforge.net>
> > > Date: Sun, 13 Dec 2015 09:30:47 +0100
> > > 
> > > Six goto statements referred to a source code position directly behind them.
> > > Thus omit such unnecessary jumps.
> > 
> > I suggest you leave a blank line instead
> > of deleting the goto.
> > 
> 
> What is the point of the little bunny hop?
> 
> regards,
> dan carpenter
> 

-ENOPARSE little bunny hop
(though I could have said "just leave a blank line)

I think that code blocks are more obvious to read.

This is the original code:

	result = foo();
	if (result)
		goto label;

	result = bar();
	if (result)
		goto label;

	result = baz();
	if (result)
		goto label;

label:
	go on...

He proposes:

	result = foo();
	if (result)
		goto label;

	result = bar();
	if (result)
		goto label;

	result = baz();
label:
	go on...


I don't find the test->goto label; label: use offensive,
but if he does, I think keeping a blank line in place of
the test->goto might be better.

	result = foo();
	if (result)
		goto label;

	result = bar();
	if (result)
		goto label;

	result = baz();

label:
	go on...

[lustre-devel] [PATCH 1/7] staging: lustre: Delete unnecessary goto statements in six functions

[Dan Carpenter]

On Tue, Dec 15, 2015 at 07:02:31AM -0800, Joe Perches wrote:
> This is the original code:
> 
> 	result = foo();
> 	if (result)
> 		goto label;
> 
> 	result = bar();
> 	if (result)
> 		goto label;
>
> 	result = baz();
> 	if (result)
> 		goto label;
> 
> label:
> 	go on...
> 

No.  There is no test.  The original code looks like:

	result = foo();
	if (result)
		goto out;
	result = baz();
	goto out;
out:
	go on..

regards,
dan carpenter

[lustre-devel] [PATCH 1/7] staging: lustre: Delete unnecessary goto statements in six functions

[Joe Perches]

On Tue, 2015-12-15 at 20:48 +0300, Dan Carpenter wrote:
> On Tue, Dec 15, 2015 at 07:02:31AM -0800, Joe Perches wrote:
> > This is the original code:
> > 
> > 	result = foo();
> > 	if (result)
> > 		goto label;
> > 
> > 	result = bar();
> > 	if (result)
> > 		goto label;
> > 
> > 	result = baz();
> > 	if (result)
> > 		goto label;
> > 
> > label:
> > 	go on...
> > 
> 
> No.??There is no test.??The original code looks like:
> 
> 	result = foo();
> 	if (result)
> 		goto out;
> 	result = baz();
> 	goto out;
> out:
> 	go on..
> 
> regards,
> dan carpenter

Here is the original code:
---------------------
	/* Copy hsm_progress struct */
	req_hpk = req_capsule_client_get(&req->rq_pill, &RMF_MDS_HSM_PROGRESS);
	if (req_hpk == NULL) {
		rc = -EPROTO;
		goto out;
	}

	*req_hpk = *hpk;
	req_hpk->hpk_errval = lustre_errno_hton(hpk->hpk_errval);

	ptlrpc_request_set_replen(req);

	rc = mdc_queue_wait(req);
	goto out;
out:
	ptlrpc_req_finished(req);
	return rc;
}
---------------------

I think if the last goto out; is to be removed,
then it should be replaced by a blank line.

It separates the last operation block from the return.

cheers, Joe

[lustre-devel] staging: lustre: Delete unnecessary goto statements in six functions

[SF Markus Elfring]

> 	rc = mdc_queue_wait(req);
> 	goto out;
> out:
> 	ptlrpc_req_finished(req);
> 	return rc;
> }
> ---------------------
>
> I think if the last goto out; is to be removed,
> then it should be replaced by a blank line.
>
> It separates the last operation block from the return.

Would you like to point a very specific coding style issue out?
How often should jump labels preceded with blank lines?

Regards,
Markus

[lustre-devel] staging: lustre: Delete unnecessary goto statements in six functions

[Joe Perches]

On Tue, 2015-12-15 at 19:26 +0100, SF Markus Elfring wrote:
> > 	rc = mdc_queue_wait(req);
> > 	goto out;
> > out:
> > 	ptlrpc_req_finished(req);
> > 	return rc;
> > }
> > ---------------------
> > 
> > I think if the last goto out; is to be removed,
> > then it should be replaced by a blank line.
> > 
> > It separates the last operation block from the return.
> 
> Would you like to point a very specific coding style issue out?

Other than using vertical separation can help readability?

I think there should _not_ be a hardened rule.
Style is just a guide. ?Do what you think appropriate.

> How often should jump labels preceded with blank lines?

When other nearby blocks are also separated by blank lines.

Localized consistency can be useful.

Inconsistency can make code harder to follow/predict.

[lustre-devel] staging: lustre: Delete unnecessary goto statements in six functions

[SF Markus Elfring]

> I think there should _not_ be a hardened rule.

I guess that it can become hard to achieve consensus on a precise rule.


> Style is just a guide.

Generally nice ?


>   Do what you think appropriate.

I'm sorry for my evolving understanding. - But I imagine that your feedback
can cause further software development troubles if the acceptance for
this update suggestion will really depend on the number of blank lines
before a jump label.

Regards,
Markus

[lustre-devel] staging: lustre: Delete unnecessary goto statements in six functions

[Joe Perches]

On Tue, 2015-12-15 at 19:49 +0100, SF Markus Elfring wrote:
> > I think there should _not_ be a hardened rule.
> I guess that it can become hard to achieve consensus on a precise rule.

Consensus isn't unanimity.

> I imagine that your feedback
> can cause further software development troubles if the acceptance for
> this update suggestion will really depend on the number of blank lines
> before a jump label.

<shrug>

The author of any particular bit of code can do
whatever they want.

Localized consistency is probably more valuable than
global consistency.

[lustre-devel] staging: lustre: Delete unnecessary goto statements in six functions

[SF Markus Elfring]

> This is the original code:
Really ??
> 	result = baz();
> 	if (result)
> 		goto label;
>
> label:
> 	go on...

I do not see such a source code structure
at the six places I propose to clean-up.


> I don't find the test->goto label; label: use offensive,
> but if he does, I think keeping a blank line in place of
> the test->goto might be better.

I find this an interesting view on source code layout.
Are there any more opinions around such implementation details?

Regards,
Markus

[lustre-devel] staging: lustre: Delete unnecessary goto statements in six functions

[Joe Perches]

On Tue, 2015-12-15 at 19:02 +0100, SF Markus Elfring wrote:
> > This is the original code:
> Really ??
> > 	result = baz();
> > 	if (result)
> > 		goto label;
> > 
> > label:
> > 	go on...
> 
> I do not see such a source code structure
> at the six places I propose to clean-up.
> 
> 
> > I don't find the test->goto label; label: use offensive,
> > but if he does, I think keeping a blank line in place of
> > the test->goto might be better.
> 
> I find this an interesting view on source code layout.
> Are there any more opinions around such implementation details?

Or to put it another way, use a blank line before the
first or only label in an error/out block.

I don't find it different then commonly written blocks like:

void foo(void)
{
	...;

	wind1();

	val = func1(...);
	if (val) {
		printk(...);
		goto err_type;
	}

	wind2();

	val = func2(...);
	if (val) {
		printk(...);
		goto err_type2;
	}

	...

	return 0;

err_type2:
	unwind2();
err_type:
	unwind1();
	return -ERR;
}

Yes, you can elide all the blank lines, but using them can
help readability.

[lustre-devel] [PATCH 2/7] staging: lustre: Rename a jump label for ptlrpc_req_finished() calls

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 13 Dec 2015 10:33:38 +0100

This issue was detected by using the Coccinelle software.

Choose a jump label according to the current Linux coding style convention.
I suggest to improve this implementation detail by the reuse of a script
like the following for the semantic patch language.

@rename_jump_label exists@
identifier work;
type return_type;
@@
 return_type work(...)
 {
 ... when any
 goto
-out
+finish_request
 ;
 ... when any
-out
+finish_request
 :
 ptlrpc_req_finished(...);
 ... when any
 }

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/lustre/lustre/llite/file.c         | 26 +++++------
 drivers/staging/lustre/lustre/llite/namei.c        | 12 ++---
 drivers/staging/lustre/lustre/llite/xattr.c        | 20 ++++----
 drivers/staging/lustre/lustre/mdc/mdc_request.c    | 54 +++++++++++-----------
 drivers/staging/lustre/lustre/osc/osc_request.c    | 28 +++++------
 drivers/staging/lustre/lustre/ptlrpc/llog_client.c | 22 ++++-----
 6 files changed, 81 insertions(+), 81 deletions(-)

diff --git a/drivers/staging/lustre/lustre/llite/file.c b/drivers/staging/lustre/lustre/llite/file.c
index 31cd6b3..b94df54 100644
--- a/drivers/staging/lustre/lustre/llite/file.c
+++ b/drivers/staging/lustre/lustre/llite/file.c
@@ -427,27 +427,27 @@ static int ll_intent_file_open(struct dentry *dentry, void *lmm,
 		*/
 		if (!it_disposition(itp, DISP_OPEN_OPEN) ||
 		     it_open_error(DISP_OPEN_OPEN, itp))
-			goto out;
+			goto finish_request;
 		ll_release_openhandle(inode, itp);
-		goto out;
+		goto finish_request;
 	}
 
 	if (it_disposition(itp, DISP_LOOKUP_NEG)) {
 		rc = -ENOENT;
-		goto out;
+		goto finish_request;
 	}
 
 	if (rc != 0 || it_open_error(DISP_OPEN_OPEN, itp)) {
 		rc = rc ? rc : it_open_error(DISP_OPEN_OPEN, itp);
 		CDEBUG(D_VFSTRACE, "lock enqueue: err: %d\n", rc);
-		goto out;
+		goto finish_request;
 	}
 
 	rc = ll_prep_inode(&inode, req, NULL, itp);
 	if (!rc && itp->d.lustre.it_lock_mode)
 		ll_set_lock_data(sbi->ll_md_exp, inode, itp, NULL);
 
-out:
+finish_request:
 	ptlrpc_req_finished(req);
 	ll_intent_drop_lock(itp);
 
@@ -2900,13 +2900,13 @@ static int __ll_inode_revalidate(struct dentry *dentry, __u64 ibits)
 		oit.it_create_mode &= ~M_CHECK_STALE;
 		if (rc < 0) {
 			rc = ll_inode_revalidate_fini(inode, rc);
-			goto out;
+			goto finish_request;
 		}
 
 		rc = ll_revalidate_it_finish(req, &oit, inode);
 		if (rc != 0) {
 			ll_intent_release(&oit);
-			goto out;
+			goto finish_request;
 		}
 
 		/* Unlinked? Unhash dentry, so it is not picked up later by
@@ -2946,7 +2946,7 @@ static int __ll_inode_revalidate(struct dentry *dentry, __u64 ibits)
 
 		rc = ll_prep_inode(&inode, req, NULL, NULL);
 	}
-out:
+finish_request:
 	ptlrpc_req_finished(req);
 	return rc;
 }
@@ -3315,25 +3315,25 @@ static int ll_layout_fetch(struct inode *inode, struct ldlm_lock *lock)
 	body = req_capsule_server_get(&req->rq_pill, &RMF_MDT_BODY);
 	if (body == NULL) {
 		rc = -EPROTO;
-		goto out;
+		goto finish_request;
 	}
 
 	lmmsize = body->eadatasize;
 	if (lmmsize == 0) /* empty layout */ {
 		rc = 0;
-		goto out;
+		goto finish_request;
 	}
 
 	lmm = req_capsule_server_sized_get(&req->rq_pill, &RMF_EADATA, lmmsize);
 	if (lmm == NULL) {
 		rc = -EFAULT;
-		goto out;
+		goto finish_request;
 	}
 
 	lvbdata = libcfs_kvzalloc(lmmsize, GFP_NOFS);
 	if (lvbdata == NULL) {
 		rc = -ENOMEM;
-		goto out;
+		goto finish_request;
 	}
 
 	memcpy(lvbdata, lmm, lmmsize);
@@ -3345,7 +3345,7 @@ static int ll_layout_fetch(struct inode *inode, struct ldlm_lock *lock)
 	lock->l_lvb_len = lmmsize;
 	unlock_res_and_lock(lock);
 
-out:
+finish_request:
 	ptlrpc_req_finished(req);
 	return rc;
 }
diff --git a/drivers/staging/lustre/lustre/llite/namei.c b/drivers/staging/lustre/lustre/llite/namei.c
index 2113dd4..7501f70 100644
--- a/drivers/staging/lustre/lustre/llite/namei.c
+++ b/drivers/staging/lustre/lustre/llite/namei.c
@@ -686,7 +686,7 @@ static struct inode *ll_create_node(struct inode *dir, struct lookup_intent *it)
 	rc = ll_prep_inode(&inode, request, dir->i_sb, it);
 	if (rc) {
 		inode = ERR_PTR(rc);
-		goto out;
+		goto finish_request;
 	}
 
 	LASSERT(hlist_empty(&inode->i_dentry));
@@ -697,7 +697,7 @@ static struct inode *ll_create_node(struct inode *dir, struct lookup_intent *it)
 	CDEBUG(D_DLMTRACE, "setting l_ast_data to inode %p (%lu/%u)\n",
 	       inode, inode->i_ino, inode->i_generation);
 	ll_set_lock_data(sbi->ll_md_exp, inode, it, NULL);
- out:
+ finish_request:
 	ptlrpc_req_finished(request);
 	return inode;
 }
@@ -960,13 +960,13 @@ static int ll_unlink(struct inode *dir, struct dentry *dentry)
 	rc = md_unlink(ll_i2sbi(dir)->ll_md_exp, op_data, &request);
 	ll_finish_md_op_data(op_data);
 	if (rc)
-		goto out;
+		goto finish_request;
 
 	ll_update_times(request, dir);
 	ll_stats_ops_tally(ll_i2sbi(dir), LPROC_LL_UNLINK, 1);
 
 	rc = ll_objects_destroy(request, dir);
- out:
+ finish_request:
 	ptlrpc_req_finished(request);
 	return rc;
 }
@@ -1059,11 +1059,11 @@ static int ll_link(struct dentry *old_dentry, struct inode *dir,
 	err = md_link(sbi->ll_md_exp, op_data, &request);
 	ll_finish_md_op_data(op_data);
 	if (err)
-		goto out;
+		goto finish_request;
 
 	ll_update_times(request, dir);
 	ll_stats_ops_tally(sbi, LPROC_LL_LINK, 1);
-out:
+finish_request:
 	ptlrpc_req_finished(request);
 	return err;
 }
diff --git a/drivers/staging/lustre/lustre/llite/xattr.c b/drivers/staging/lustre/lustre/llite/xattr.c
index 660b8ac..8d3287c 100644
--- a/drivers/staging/lustre/lustre/llite/xattr.c
+++ b/drivers/staging/lustre/lustre/llite/xattr.c
@@ -390,19 +390,19 @@ getxattr_nocache:
 		/* only detect the xattr size */
 		if (size == 0) {
 			rc = body->eadatasize;
-			goto out;
+			goto finish_request;
 		}
 
 		if (size < body->eadatasize) {
 			CERROR("server bug: replied size %u > %u\n",
 				body->eadatasize, (int)size);
 			rc = -ERANGE;
-			goto out;
+			goto finish_request;
 		}
 
 		if (body->eadatasize == 0) {
 			rc = -ENODATA;
-			goto out;
+			goto finish_request;
 		}
 
 		/* do not need swab xattr data */
@@ -410,7 +410,7 @@ getxattr_nocache:
 							body->eadatasize);
 		if (!xdata) {
 			rc = -EFAULT;
-			goto out;
+			goto finish_request;
 		}
 
 		memcpy(buffer, xdata, body->eadatasize);
@@ -425,14 +425,14 @@ getxattr_nocache:
 					(posix_acl_xattr_header *)buffer, rc);
 		if (IS_ERR(acl)) {
 			rc = PTR_ERR(acl);
-			goto out;
+			goto finish_request;
 		}
 
 		rc = ee_add(&sbi->ll_et, current_pid(), ll_inode2fid(inode),
 			    xattr_type, acl);
 		if (unlikely(rc < 0)) {
 			lustre_ext_acl_xattr_free(acl);
-			goto out;
+			goto finish_request;
 		}
 	}
 #endif
@@ -444,7 +444,7 @@ out_xattr:
 			ll_get_fsname(inode->i_sb, NULL, 0), rc);
 		sbi->ll_flags &= ~LL_SBI_USER_XATTR;
 	}
-out:
+finish_request:
 	ptlrpc_req_finished(req);
 	return rc;
 }
@@ -555,7 +555,7 @@ ssize_t ll_listxattr(struct dentry *dentry, char *buffer, size_t size)
 
 	rc = ll_getxattr_common(inode, NULL, buffer, size, OBD_MD_FLXATTRLS);
 	if (rc < 0)
-		goto out;
+		goto finish_request;
 
 	if (buffer != NULL) {
 		struct ll_sb_info *sbi = ll_i2sbi(inode);
@@ -589,7 +589,7 @@ ssize_t ll_listxattr(struct dentry *dentry, char *buffer, size_t size)
 
 	if (rc2 < 0) {
 		rc2 = 0;
-		goto out;
+		goto finish_request;
 	} else if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)) {
 		const int prefix_len = sizeof(XATTR_LUSTRE_PREFIX) - 1;
 		const size_t name_len   = sizeof("lov") - 1;
@@ -608,7 +608,7 @@ ssize_t ll_listxattr(struct dentry *dentry, char *buffer, size_t size)
 		}
 		rc2 = total_len;
 	}
-out:
+finish_request:
 	ptlrpc_req_finished(request);
 	rc = rc + rc2;
 
diff --git a/drivers/staging/lustre/lustre/mdc/mdc_request.c b/drivers/staging/lustre/lustre/mdc/mdc_request.c
index 920b1e9..2a76685 100644
--- a/drivers/staging/lustre/lustre/mdc/mdc_request.c
+++ b/drivers/staging/lustre/lustre/mdc/mdc_request.c
@@ -92,12 +92,12 @@ static int mdc_getstatus(struct obd_export *exp, struct lu_fid *rootfid)
 
 	rc = ptlrpc_queue_wait(req);
 	if (rc)
-		goto out;
+		goto finish_request;
 
 	body = req_capsule_server_get(&req->rq_pill, &RMF_MDT_BODY);
 	if (body == NULL) {
 		rc = -EPROTO;
-		goto out;
+		goto finish_request;
 	}
 
 	*rootfid = body->fid1;
@@ -105,7 +105,7 @@ static int mdc_getstatus(struct obd_export *exp, struct lu_fid *rootfid)
 	       "root fid="DFID", last_committed=%llu\n",
 	       PFID(rootfid),
 	       lustre_msg_get_last_committed(req->rq_repmsg));
-out:
+finish_request:
 	ptlrpc_req_finished(req);
 	return rc;
 }
@@ -1084,17 +1084,17 @@ static int mdc_statfs(const struct lu_env *env,
 		/* check connection error first */
 		if (imp->imp_connect_error)
 			rc = imp->imp_connect_error;
-		goto out;
+		goto finish_request;
 	}
 
 	msfs = req_capsule_server_get(&req->rq_pill, &RMF_OBD_STATFS);
 	if (msfs == NULL) {
 		rc = -EPROTO;
-		goto out;
+		goto finish_request;
 	}
 
 	*osfs = *msfs;
-out:
+finish_request:
 	ptlrpc_req_finished(req);
 output:
 	class_import_put(imp);
@@ -1163,7 +1163,7 @@ static int mdc_ioc_hsm_progress(struct obd_export *exp,
 					LUSTRE_MDS_VERSION, MDS_HSM_PROGRESS);
 	if (req == NULL) {
 		rc = -ENOMEM;
-		goto out;
+		goto finish_request;
 	}
 
 	mdc_pack_body(req, NULL, OBD_MD_FLRMTPERM, 0, 0, 0);
@@ -1172,7 +1172,7 @@ static int mdc_ioc_hsm_progress(struct obd_export *exp,
 	req_hpk = req_capsule_client_get(&req->rq_pill, &RMF_MDS_HSM_PROGRESS);
 	if (req_hpk == NULL) {
 		rc = -EPROTO;
-		goto out;
+		goto finish_request;
 	}
 
 	*req_hpk = *hpk;
@@ -1181,7 +1181,7 @@ static int mdc_ioc_hsm_progress(struct obd_export *exp,
 	ptlrpc_request_set_replen(req);
 
 	rc = mdc_queue_wait(req);
-out:
+finish_request:
 	ptlrpc_req_finished(req);
 	return rc;
 }
@@ -1197,7 +1197,7 @@ static int mdc_ioc_hsm_ct_register(struct obd_import *imp, __u32 archives)
 					MDS_HSM_CT_REGISTER);
 	if (req == NULL) {
 		rc = -ENOMEM;
-		goto out;
+		goto finish_request;
 	}
 
 	mdc_pack_body(req, NULL, OBD_MD_FLRMTPERM, 0, 0, 0);
@@ -1207,7 +1207,7 @@ static int mdc_ioc_hsm_ct_register(struct obd_import *imp, __u32 archives)
 					      &RMF_MDS_HSM_ARCHIVE);
 	if (archive_mask == NULL) {
 		rc = -EPROTO;
-		goto out;
+		goto finish_request;
 	}
 
 	*archive_mask = archives;
@@ -1215,7 +1215,7 @@ static int mdc_ioc_hsm_ct_register(struct obd_import *imp, __u32 archives)
 	ptlrpc_request_set_replen(req);
 
 	rc = mdc_queue_wait(req);
-out:
+finish_request:
 	ptlrpc_req_finished(req);
 	return rc;
 }
@@ -1246,18 +1246,18 @@ static int mdc_ioc_hsm_current_action(struct obd_export *exp,
 
 	rc = mdc_queue_wait(req);
 	if (rc)
-		goto out;
+		goto finish_request;
 
 	req_hca = req_capsule_server_get(&req->rq_pill,
 					 &RMF_MDS_HSM_CURRENT_ACTION);
 	if (req_hca == NULL) {
 		rc = -EPROTO;
-		goto out;
+		goto finish_request;
 	}
 
 	*hca = *req_hca;
 
-out:
+finish_request:
 	ptlrpc_req_finished(req);
 	return rc;
 }
@@ -1272,7 +1272,7 @@ static int mdc_ioc_hsm_ct_unregister(struct obd_import *imp)
 					MDS_HSM_CT_UNREGISTER);
 	if (req == NULL) {
 		rc = -ENOMEM;
-		goto out;
+		goto finish_request;
 	}
 
 	mdc_pack_body(req, NULL, OBD_MD_FLRMTPERM, 0, 0, 0);
@@ -1280,7 +1280,7 @@ static int mdc_ioc_hsm_ct_unregister(struct obd_import *imp)
 	ptlrpc_request_set_replen(req);
 
 	rc = mdc_queue_wait(req);
-out:
+finish_request:
 	ptlrpc_req_finished(req);
 	return rc;
 }
@@ -1311,17 +1311,17 @@ static int mdc_ioc_hsm_state_get(struct obd_export *exp,
 
 	rc = mdc_queue_wait(req);
 	if (rc)
-		goto out;
+		goto finish_request;
 
 	req_hus = req_capsule_server_get(&req->rq_pill, &RMF_HSM_USER_STATE);
 	if (req_hus == NULL) {
 		rc = -EPROTO;
-		goto out;
+		goto finish_request;
 	}
 
 	*hus = *req_hus;
 
-out:
+finish_request:
 	ptlrpc_req_finished(req);
 	return rc;
 }
@@ -1352,14 +1352,14 @@ static int mdc_ioc_hsm_state_set(struct obd_export *exp,
 	req_hss = req_capsule_client_get(&req->rq_pill, &RMF_HSM_STATE_SET);
 	if (req_hss == NULL) {
 		rc = -EPROTO;
-		goto out;
+		goto finish_request;
 	}
 	*req_hss = *hss;
 
 	ptlrpc_request_set_replen(req);
 
 	rc = mdc_queue_wait(req);
-out:
+finish_request:
 	ptlrpc_req_finished(req);
 	return rc;
 }
@@ -1377,7 +1377,7 @@ static int mdc_ioc_hsm_request(struct obd_export *exp,
 	req = ptlrpc_request_alloc(imp, &RQF_MDS_HSM_REQUEST);
 	if (req == NULL) {
 		rc = -ENOMEM;
-		goto out;
+		goto finish_request;
 	}
 
 	req_capsule_set_size(&req->rq_pill, &RMF_MDS_HSM_USER_ITEM, RCL_CLIENT,
@@ -1398,7 +1398,7 @@ static int mdc_ioc_hsm_request(struct obd_export *exp,
 	req_hr = req_capsule_client_get(&req->rq_pill, &RMF_MDS_HSM_REQUEST);
 	if (req_hr == NULL) {
 		rc = -EPROTO;
-		goto out;
+		goto finish_request;
 	}
 	*req_hr = hur->hur_request;
 
@@ -1406,7 +1406,7 @@ static int mdc_ioc_hsm_request(struct obd_export *exp,
 	req_hui = req_capsule_client_get(&req->rq_pill, &RMF_MDS_HSM_USER_ITEM);
 	if (req_hui == NULL) {
 		rc = -EPROTO;
-		goto out;
+		goto finish_request;
 	}
 	memcpy(req_hui, hur->hur_user_item,
 	       hur->hur_request.hr_itemcount * sizeof(struct hsm_user_item));
@@ -1415,14 +1415,14 @@ static int mdc_ioc_hsm_request(struct obd_export *exp,
 	req_opaque = req_capsule_client_get(&req->rq_pill, &RMF_GENERIC_DATA);
 	if (req_opaque == NULL) {
 		rc = -EPROTO;
-		goto out;
+		goto finish_request;
 	}
 	memcpy(req_opaque, hur_data(hur), hur->hur_request.hr_data_len);
 
 	ptlrpc_request_set_replen(req);
 
 	rc = mdc_queue_wait(req);
-out:
+finish_request:
 	ptlrpc_req_finished(req);
 	return rc;
 }
diff --git a/drivers/staging/lustre/lustre/osc/osc_request.c b/drivers/staging/lustre/lustre/osc/osc_request.c
index d6c1447..3a56fb7 100644
--- a/drivers/staging/lustre/lustre/osc/osc_request.c
+++ b/drivers/staging/lustre/lustre/osc/osc_request.c
@@ -291,12 +291,12 @@ static int osc_getattr(const struct lu_env *env, struct obd_export *exp,
 
 	rc = ptlrpc_queue_wait(req);
 	if (rc)
-		goto out;
+		goto finish_request;
 
 	body = req_capsule_server_get(&req->rq_pill, &RMF_OST_BODY);
 	if (body == NULL) {
 		rc = -EPROTO;
-		goto out;
+		goto finish_request;
 	}
 
 	CDEBUG(D_INODE, "mode: %o\n", body->oa.o_mode);
@@ -306,7 +306,7 @@ static int osc_getattr(const struct lu_env *env, struct obd_export *exp,
 	oinfo->oi_oa->o_blksize = cli_brw_size(exp->exp_obd);
 	oinfo->oi_oa->o_valid |= OBD_MD_FLBLKSZ;
 
- out:
+ finish_request:
 	ptlrpc_req_finished(req);
 	return rc;
 }
@@ -336,18 +336,18 @@ static int osc_setattr(const struct lu_env *env, struct obd_export *exp,
 
 	rc = ptlrpc_queue_wait(req);
 	if (rc)
-		goto out;
+		goto finish_request;
 
 	body = req_capsule_server_get(&req->rq_pill, &RMF_OST_BODY);
 	if (body == NULL) {
 		rc = -EPROTO;
-		goto out;
+		goto finish_request;
 	}
 
 	lustre_get_wire_obdo(&req->rq_import->imp_connect_data, oinfo->oi_oa,
 			     &body->oa);
 
-out:
+finish_request:
 	ptlrpc_req_finished(req);
 	return rc;
 }
@@ -1276,7 +1276,7 @@ static int osc_brw_prep_request(int cmd, struct client_obd *cli,
 
 	if (desc == NULL) {
 		rc = -ENOMEM;
-		goto out;
+		goto finish_request;
 	}
 	/* NB request now owns desc and will free it when it gets freed */
 
@@ -1407,7 +1407,7 @@ static int osc_brw_prep_request(int cmd, struct client_obd *cli,
 	*reqp = req;
 	return 0;
 
- out:
+ finish_request:
 	ptlrpc_req_finished(req);
 	return rc;
 }
@@ -2513,17 +2513,17 @@ static int osc_statfs(const struct lu_env *env, struct obd_export *exp,
 
 	rc = ptlrpc_queue_wait(req);
 	if (rc)
-		goto out;
+		goto finish_request;
 
 	msfs = req_capsule_server_get(&req->rq_pill, &RMF_OBD_STATFS);
 	if (msfs == NULL) {
 		rc = -EPROTO;
-		goto out;
+		goto finish_request;
 	}
 
 	*osfs = *msfs;
 
- out:
+ finish_request:
 	ptlrpc_req_finished(req);
 	return rc;
 }
@@ -2718,16 +2718,16 @@ static int osc_get_info(const struct lu_env *env, struct obd_export *exp,
 		ptlrpc_request_set_replen(req);
 		rc = ptlrpc_queue_wait(req);
 		if (rc)
-			goto out;
+			goto finish_request;
 
 		reply = req_capsule_server_get(&req->rq_pill, &RMF_OBD_ID);
 		if (reply == NULL) {
 			rc = -EPROTO;
-			goto out;
+			goto finish_request;
 		}
 
 		*((u64 *)val) = *reply;
-	out:
+	finish_request:
 		ptlrpc_req_finished(req);
 		return rc;
 	} else if (KEY_IS(KEY_FIEMAP)) {
diff --git a/drivers/staging/lustre/lustre/ptlrpc/llog_client.c b/drivers/staging/lustre/lustre/ptlrpc/llog_client.c
index 5122205..150d2ec 100644
--- a/drivers/staging/lustre/lustre/ptlrpc/llog_client.c
+++ b/drivers/staging/lustre/lustre/ptlrpc/llog_client.c
@@ -176,26 +176,26 @@ static int llog_client_next_block(const struct lu_env *env,
 	ptlrpc_request_set_replen(req);
 	rc = ptlrpc_queue_wait(req);
 	if (rc)
-		goto out;
+		goto finish_request;
 
 	body = req_capsule_server_get(&req->rq_pill, &RMF_LLOGD_BODY);
 	if (body == NULL) {
 		rc = -EFAULT;
-		goto out;
+		goto finish_request;
 	}
 
 	/* The log records are swabbed as they are processed */
 	ptr = req_capsule_server_get(&req->rq_pill, &RMF_EADATA);
 	if (ptr == NULL) {
 		rc = -EFAULT;
-		goto out;
+		goto finish_request;
 	}
 
 	*cur_idx = body->lgd_saved_index;
 	*cur_offset = body->lgd_cur_offset;
 
 	memcpy(buf, ptr, len);
-out:
+finish_request:
 	ptlrpc_req_finished(req);
 err_exit:
 	LLOG_CLIENT_EXIT(loghandle->lgh_ctxt, imp);
@@ -233,22 +233,22 @@ static int llog_client_prev_block(const struct lu_env *env,
 
 	rc = ptlrpc_queue_wait(req);
 	if (rc)
-		goto out;
+		goto finish_request;
 
 	body = req_capsule_server_get(&req->rq_pill, &RMF_LLOGD_BODY);
 	if (body == NULL) {
 		rc = -EFAULT;
-		goto out;
+		goto finish_request;
 	}
 
 	ptr = req_capsule_server_get(&req->rq_pill, &RMF_EADATA);
 	if (ptr == NULL) {
 		rc = -EFAULT;
-		goto out;
+		goto finish_request;
 	}
 
 	memcpy(buf, ptr, len);
-out:
+finish_request:
 	ptlrpc_req_finished(req);
 err_exit:
 	LLOG_CLIENT_EXIT(loghandle->lgh_ctxt, imp);
@@ -282,12 +282,12 @@ static int llog_client_read_header(const struct lu_env *env,
 	ptlrpc_request_set_replen(req);
 	rc = ptlrpc_queue_wait(req);
 	if (rc)
-		goto out;
+		goto finish_request;
 
 	hdr = req_capsule_server_get(&req->rq_pill, &RMF_LLOG_LOG_HDR);
 	if (hdr == NULL) {
 		rc = -EFAULT;
-		goto out;
+		goto finish_request;
 	}
 
 	memcpy(handle->lgh_hdr, hdr, sizeof(*hdr));
@@ -305,7 +305,7 @@ static int llog_client_read_header(const struct lu_env *env,
 		CERROR("you may need to re-run lconf --write_conf.\n");
 		rc = -EIO;
 	}
-out:
+finish_request:
 	ptlrpc_req_finished(req);
 err_exit:
 	LLOG_CLIENT_EXIT(handle->lgh_ctxt, imp);
-- 
2.6.3

[lustre-devel] [PATCH 2/7] staging: lustre: Rename a jump label for ptlrpc_req_finished() calls

[Dan Carpenter]

Markus, please stop sending these things to rename out labels unless
there is a bug.  CodingStyle allows out labels.

regards,
dan carpenter

[lustre-devel] [PATCH 2/7] staging: lustre: Rename a jump label for ptlrpc_req_finished() calls

[SF Markus Elfring]

> Markus, please stop sending these things to rename out labels unless
> there is a bug.  CodingStyle allows out labels.

How does this feedback fit to information like the following?

"?
Chapter 7: ?
?
Choose label names which say what the goto does or why the goto exists.
? Avoid using GW-BASIC names ?
?"

Regards,
Markus

[lustre-devel] [PATCH 2/7] staging: lustre: Rename a jump label for ptlrpc_req_finished() calls

[Dan Carpenter]

On Mon, Dec 14, 2015 at 10:08:03AM +0100, SF Markus Elfring wrote:
> > Markus, please stop sending these things to rename out labels unless
> > there is a bug.  CodingStyle allows out labels.
> 
> How does this feedback fit to information like the following?
> 
> "?
> Chapter 7: ?
> ?
> Choose label names which say what the goto does or why the goto exists.

A lot of people think "out" says what the goto does and why it exists.
I personally don't agree with them but if you look at when I complain
about it, it's almost always when it causes a bug.

> ? Avoid using GW-BASIC names ?

Those when people just use numbers for their label names instead of
words like out1, out2, out4, out5.  It's a different thing.

regards,
dan carpenter

[lustre-devel] staging: lustre: Rename a jump label for ptlrpc_req_finished() calls

[SF Markus Elfring]

>>> Markus, please stop sending these things to rename out labels unless
>>> there is a bug.  CodingStyle allows out labels.
>>
>> How does this feedback fit to information like the following?
>>
>> "?
>> Chapter 7: ?
>> ?
>> Choose label names which say what the goto does or why the goto exists.
> 
> A lot of people think "out" says what the goto does and why it exists.

I have got the impression that this short identifier is only partly appropriate.


> I personally don't agree with them

I guess that my opinion goes into a similar direction here.


> but if you look at when I complain about it, it's almost always
> when it causes a bug.

I agree that the combination with bug fixing is more appealing
than an attempt to improve coding style applications.


>> ? Avoid using GW-BASIC names ?
> 
> Those when people just use numbers for their label names instead of
> words like out1, out2, out4, out5.  It's a different thing.

The difference is not so clear for me as it appears to you.
How many software developers can still remember habits around
the selection of such identifiers from GW-BASIC times?

Regards,
Markus

[lustre-devel] [PATCH 3/7] staging: lustre: Rename a jump label for a kfree(key) call

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 13 Dec 2015 10:56:35 +0100

This issue was detected by using the Coccinelle software.

Choose a jump label according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/lustre/lustre/mdc/mdc_request.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/lustre/lustre/mdc/mdc_request.c b/drivers/staging/lustre/lustre/mdc/mdc_request.c
index 2a76685..2085ba6 100644
--- a/drivers/staging/lustre/lustre/mdc/mdc_request.c
+++ b/drivers/staging/lustre/lustre/mdc/mdc_request.c
@@ -1125,7 +1125,7 @@ static int mdc_ioc_fid2path(struct obd_export *exp, struct getinfo_fid2path *gf)
 
 	if (!fid_is_sane(&gf->gf_fid)) {
 		rc = -EINVAL;
-		goto out;
+		goto free_key;
 	}
 
 	/* Val is struct getinfo_fid2path result plus path */
@@ -1133,20 +1133,19 @@ static int mdc_ioc_fid2path(struct obd_export *exp, struct getinfo_fid2path *gf)
 
 	rc = obd_get_info(NULL, exp, keylen, key, &vallen, gf, NULL);
 	if (rc != 0 && rc != -EREMOTE)
-		goto out;
+		goto free_key;
 
 	if (vallen <= sizeof(*gf)) {
 		rc = -EPROTO;
-		goto out;
+		goto free_key;
 	} else if (vallen > sizeof(*gf) + gf->gf_pathlen) {
 		rc = -EOVERFLOW;
-		goto out;
+		goto free_key;
 	}
 
 	CDEBUG(D_IOCTL, "path get "DFID" from %llu #%d\n%s\n",
 	       PFID(&gf->gf_fid), gf->gf_recno, gf->gf_linkno, gf->gf_path);
-
-out:
+free_key:
 	kfree(key);
 	return rc;
 }
-- 
2.6.3

[lustre-devel] [PATCH 4/7] staging: lustre: Delete an unnecessary variable initialisation in mgc_process_recover_log()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 13 Dec 2015 12:00:32 +0100

The variable "mne_swab" will eventually be set to an appropriate value
from a call of the ptlrpc_rep_need_swab() function.
Thus let us omit the explicit initialisation at the beginning.

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

diff --git a/drivers/staging/lustre/lustre/mgc/mgc_request.c b/drivers/staging/lustre/lustre/mgc/mgc_request.c
index 2c48847..da130f4 100644
--- a/drivers/staging/lustre/lustre/mgc/mgc_request.c
+++ b/drivers/staging/lustre/lustre/mgc/mgc_request.c
@@ -1293,7 +1293,7 @@ static int mgc_process_recover_log(struct obd_device *obd,
 	struct page **pages;
 	int nrpages;
 	bool eof = true;
-	bool mne_swab = false;
+	bool mne_swab;
 	int i;
 	int ealen;
 	int rc;
-- 
2.6.3

[lustre-devel] [PATCH 5/7] staging: lustre: Less checks in mgc_process_recover_log() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 13 Dec 2015 12:21:17 +0100

A few checks would be performed by the mgc_process_recover_log() function
even if it is known already that the passed variable "pages" contained
a null pointer.

* Let us return directly if a call of the kcalloc() function failed.

* Move assignments for the variables "eof" and "req" behind
  this memory allocation.

* Delete a sanity check then.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/lustre/lustre/mgc/mgc_request.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/lustre/lustre/mgc/mgc_request.c b/drivers/staging/lustre/lustre/mgc/mgc_request.c
index da130f4..f3b4c30 100644
--- a/drivers/staging/lustre/lustre/mgc/mgc_request.c
+++ b/drivers/staging/lustre/lustre/mgc/mgc_request.c
@@ -1285,14 +1285,14 @@ static int mgc_apply_recover_logs(struct obd_device *mgc,
 static int mgc_process_recover_log(struct obd_device *obd,
 				   struct config_llog_data *cld)
 {
-	struct ptlrpc_request *req = NULL;
+	struct ptlrpc_request *req;
 	struct config_llog_instance *cfg = &cld->cld_cfg;
 	struct mgs_config_body *body;
 	struct mgs_config_res  *res;
 	struct ptlrpc_bulk_desc *desc;
 	struct page **pages;
 	int nrpages;
-	bool eof = true;
+	bool eof;
 	bool mne_swab;
 	int i;
 	int ealen;
@@ -1309,10 +1309,11 @@ static int mgc_process_recover_log(struct obd_device *obd,
 		nrpages = CONFIG_READ_NRPAGES_INIT;
 
 	pages = kcalloc(nrpages, sizeof(*pages), GFP_KERNEL);
-	if (pages == NULL) {
-		rc = -ENOMEM;
-		goto out;
-	}
+	if (!pages)
+		return -ENOMEM;
+
+	req = NULL;
+	eof = true;
 
 	for (i = 0; i < nrpages; i++) {
 		pages[i] = alloc_page(GFP_KERNEL);
@@ -1432,14 +1433,12 @@ out:
 	if (rc == 0 && !eof)
 		goto again;
 
-	if (pages) {
-		for (i = 0; i < nrpages; i++) {
-			if (pages[i] == NULL)
-				break;
-			__free_page(pages[i]);
-		}
-		kfree(pages);
+	for (i = 0; i < nrpages; i++) {
+		if (pages[i] == NULL)
+			break;
+		__free_page(pages[i]);
 	}
+	kfree(pages);
 	return rc;
 }
 
-- 
2.6.3

[lustre-devel] [PATCH 5/7] staging: lustre: Less checks in mgc_process_recover_log() after error detection

[Dan Carpenter]

On Sun, Dec 13, 2015 at 02:57:38PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 13 Dec 2015 12:21:17 +0100
> 
> A few checks would be performed by the mgc_process_recover_log() function
> even if it is known already that the passed variable "pages" contained
> a null pointer.
> 
> * Let us return directly if a call of the kcalloc() function failed.
> 
> * Move assignments for the variables "eof" and "req" behind
>   this memory allocation.

Why?  Then in the next patch it moves again.  It's like cup shuffle to
read these patches sometimes.

regards,
dan carpenter

[lustre-devel] [PATCH 5/7] staging: lustre: Less checks in mgc_process_recover_log() after error detection

[SF Markus Elfring]

>> A few checks would be performed by the mgc_process_recover_log() function
>> even if it is known already that the passed variable "pages" contained
>> a null pointer.
>>
>> * Let us return directly if a call of the kcalloc() function failed.
>>
>> * Move assignments for the variables "eof" and "req" behind
>>   this memory allocation.
> 
> Why?

The positions of their initialisation depends on the selected exception 
handling implementation, doesn't it?

Can you accept the proposed changes around the affected memory allocations?


> Then in the next patch it moves again.

This detail is a matter of patch granularity.


> It's like cup shuffle to read these patches sometimes.

Do you prefer to stash any changes together for a bigger update step?

Regards,
Markus

[lustre-devel] [PATCH 5/7] staging: lustre: Less checks in mgc_process_recover_log() after error detection

[Dan Carpenter]

On Mon, Dec 14, 2015 at 01:04:14PM +0100, SF Markus Elfring wrote:
> >> A few checks would be performed by the mgc_process_recover_log() function
> >> even if it is known already that the passed variable "pages" contained
> >> a null pointer.
> >>
> >> * Let us return directly if a call of the kcalloc() function failed.
> >>
> >> * Move assignments for the variables "eof" and "req" behind
> >>   this memory allocation.
> > 
> > Why?
> 
> The positions of their initialisation depends on the selected exception 
> handling implementation, doesn't it?
> 
> Can you accept the proposed changes around the affected memory allocations?
> 

Just leave it as-is if there is no reason.

> 
> > Then in the next patch it moves again.
> 
> This detail is a matter of patch granularity.
> 
> 
> > It's like cup shuffle to read these patches sometimes.
> 
> Do you prefer to stash any changes together for a bigger update step?

Yes.  Patches 5 and 6 would be easier to review if they were folded into
one patch.

regards,
dan carpenter

[lustre-devel] staging: lustre: Less checks in mgc_process_recover_log() after error detection

[SF Markus Elfring]

>> Can you accept the proposed changes around the affected memory allocations?
> 
> Just leave it as-is if there is no reason.

I suggest to make the implementation of the function "mgc_process_recover_log"
a bit more efficient.


>> Do you prefer to stash any changes together for a bigger update step?
> 
> Yes.  Patches 5 and 6 would be easier to review if they were folded into
> one patch.

I do not like patch squashing for my update suggestions here.

Regards,
Markus

[lustre-devel] staging: lustre: Less checks in mgc_process_recover_log() after error detection

[Dan Carpenter]

On Mon, Dec 14, 2015 at 01:45:11PM +0100, SF Markus Elfring wrote:
> >> Do you prefer to stash any changes together for a bigger update step?
> > 
> > Yes.  Patches 5 and 6 would be easier to review if they were folded into
> > one patch.
> 
> I do not like patch squashing for my update suggestions here.

I am a maintainer in drivers/staging.  I am telling you what you need to
do if you want us to apply your patch.  What you do with that
information is up to you.

regards,
dan carpenter

[lustre-devel] staging: lustre: Less checks in mgc_process_recover_log() after error detection

[SF Markus Elfring]

>> I do not like patch squashing for my update suggestions here.
> 
> I am a maintainer in drivers/staging.

Thanks for this information.


> I am telling you what you need to do if you want us to apply your patch.

I am still waiting for a bit more constructive feedback for this
patch series. How many days should I wait before I should send adjusted
update suggestions for this approach?


> What you do with that information is up to you.

Our software development dialogue seems to trigger special
challenges between us so far.
Are you generally willing to change the exception handling for
the memory allocations in the function "mgc_process_recover_log"
at all?

Regards,
Markus

[lustre-devel] staging: lustre: Less checks in mgc_process_recover_log() after error detection

[Dan Carpenter]

On Mon, Dec 14, 2015 at 06:43:15PM +0100, SF Markus Elfring wrote:
> Our software development dialogue seems to trigger special
> challenges between us so far.

I try very hard to review patches mechanically and not be biased so that
after a while people know if their patches will be merged or not without
waiting for feedback.

In this case, I had asked you not to send patches renaming out labels
and then the next day you sent me a string of patches renaming out
labels.  If you were a lustre dev then I would accept these renames
definitely.  But I believe that for anyone else, I would ask them what
the point of doing these renames is.  I do not think I have been unfair
to you.  There was no element of surprise.

Part of the reason we have CodingStyle is so that we can tell people
"That's not in CodingStyle, that's just your own opinion so don't redo
code just because you have a different opinion from the maintainer."

> Are you generally willing to change the exception handling for
> the memory allocations in the function "mgc_process_recover_log"
> at all?

I like the first patch in this series.  I do not like the renames.  I
don't care too much about patches 5 and 6 except that they should be
folded together and you should not move "req" and "eof" around.

Mostly I wish you would just focus on fixing bugs instead of these sorts
of patches.  It is a lot of work for me to explain how to redo patches
but it is worth it for bugfixes.

regards,
dan carpenter

[lustre-devel] staging: lustre: Less checks in mgc_process_recover_log() after error detection

[SF Markus Elfring]

> If you were a lustre dev then I would accept these renames definitely.

I find this information interesting.
Would any more contributors like to share their opinion?


> I do not think I have been unfair to you.

This view is correct in principle.


> There was no element of surprise.

I am trying to discuss further "special" update suggestions
where the topic focus might evolve to new directions.
I got the impression that you had some difficulties already
with my previous proposals. So I am unsure about the general
change acceptance from you alone.

You pointed out that you are maintainer for this software area.
I was not so aware about this detail while I noticed that
you are very active Linux software developer.
(You are not mentioned in the file "MAINTAINERS" for example.)


> Part of the reason we have CodingStyle is so that we can tell people
> "That's not in CodingStyle, that's just your own opinion so don't redo
> code just because you have a different opinion from the maintainer."

I find this description reasonable.

But I see some challenges to improve the coding style specification.
I would appreciate if some items can become less ambiguous and imprecise.
I assume that a few recommendations from the script "checkpatch.pl"
should also be mentioned there.

>
>> Are you generally willing to change the exception handling for
>> the memory allocations in the function "mgc_process_recover_log"
>> at all?
> I like the first patch in this series.

Thanks for a bit of positive feedback.


> I do not like the renames.

I guess that this design aspect can be clarified a bit more.


> I don't care too much about patches 5 and 6 except that they should be
> folded together and you should not move "req" and "eof" around.

I can understand this concern better than your first response
for these update steps.

I might send an adjusted patch series a few days later.


> Mostly I wish you would just focus on fixing bugs instead of these sorts
> of patches.

How often are deviations from the coding style also just ordinary bugs?

It seems that changes for this area are occasionally not so attractive
in comparison to software improvements for components
which are more popular.

Regards,
Markus

[lustre-devel] [PATCH 6/7] staging: lustre: A few checks less in mgc_process_recover_log() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 13 Dec 2015 13:03:58 +0100

A few checks would be performed by the mgc_process_recover_log() function
even if it was determined that a call of the alloc_page() function failed.

* This implementation detail could be improved by adjustments
  for jump targets according to the Linux coding style convention.

* Move the assignment for the variable "eof" behind the memory allocation.

* Delete another sanity check then.

* The variable "req" will eventually be set to an appropriate pointer
  from a call of the ptlrpc_request_alloc() function.
  Thus let us omit the explicit initialisation before.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/lustre/lustre/mgc/mgc_request.c | 32 +++++++++++--------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/lustre/lustre/mgc/mgc_request.c b/drivers/staging/lustre/lustre/mgc/mgc_request.c
index f3b4c30..7048722 100644
--- a/drivers/staging/lustre/lustre/mgc/mgc_request.c
+++ b/drivers/staging/lustre/lustre/mgc/mgc_request.c
@@ -1312,17 +1312,15 @@ static int mgc_process_recover_log(struct obd_device *obd,
 	if (!pages)
 		return -ENOMEM;
 
-	req = NULL;
-	eof = true;
-
 	for (i = 0; i < nrpages; i++) {
 		pages[i] = alloc_page(GFP_KERNEL);
 		if (pages[i] == NULL) {
 			rc = -ENOMEM;
-			goto out;
+			goto free_pages;
 		}
 	}
 
+	eof = true;
 again:
 	LASSERT(cld_is_recover(cld));
 	LASSERT(mutex_is_locked(&cld->cld_lock));
@@ -1330,12 +1328,12 @@ again:
 				   &RQF_MGS_CONFIG_READ);
 	if (req == NULL) {
 		rc = -ENOMEM;
-		goto out;
+		goto free_pages;
 	}
 
 	rc = ptlrpc_request_pack(req, LUSTRE_MGS_VERSION, MGS_CONFIG_READ);
 	if (rc)
-		goto out;
+		goto finish_request;
 
 	/* pack request */
 	body = req_capsule_client_get(&req->rq_pill, &RMF_MGS_CONFIG_BODY);
@@ -1344,7 +1342,7 @@ again:
 	if (strlcpy(body->mcb_name, cld->cld_logname, sizeof(body->mcb_name))
 	    >= sizeof(body->mcb_name)) {
 		rc = -E2BIG;
-		goto out;
+		goto finish_request;
 	}
 	body->mcb_offset = cfg->cfg_last_idx + 1;
 	body->mcb_type   = cld->cld_type;
@@ -1356,7 +1354,7 @@ again:
 				    MGS_BULK_PORTAL);
 	if (desc == NULL) {
 		rc = -ENOMEM;
-		goto out;
+		goto finish_request;
 	}
 
 	for (i = 0; i < nrpages; i++)
@@ -1365,12 +1363,12 @@ again:
 	ptlrpc_request_set_replen(req);
 	rc = ptlrpc_queue_wait(req);
 	if (rc)
-		goto out;
+		goto finish_request;
 
 	res = req_capsule_server_get(&req->rq_pill, &RMF_MGS_CONFIG_RES);
 	if (res->mcr_size < res->mcr_offset) {
 		rc = -EINVAL;
-		goto out;
+		goto finish_request;
 	}
 
 	/* always update the index even though it might have errors with
@@ -1384,18 +1382,18 @@ again:
 	ealen = sptlrpc_cli_unwrap_bulk_read(req, req->rq_bulk, 0);
 	if (ealen < 0) {
 		rc = ealen;
-		goto out;
+		goto finish_request;
 	}
 
 	if (ealen > nrpages << PAGE_CACHE_SHIFT) {
 		rc = -EINVAL;
-		goto out;
+		goto finish_request;
 	}
 
 	if (ealen == 0) { /* no logs transferred */
 		if (!eof)
 			rc = -EINVAL;
-		goto out;
+		goto finish_request;
 	}
 
 	mne_swab = !!ptlrpc_rep_need_swab(req);
@@ -1425,14 +1423,12 @@ again:
 
 		ealen -= PAGE_CACHE_SIZE;
 	}
-
-out:
-	if (req)
-		ptlrpc_req_finished(req);
+finish_request:
+	ptlrpc_req_finished(req);
 
 	if (rc == 0 && !eof)
 		goto again;
-
+free_pages:
 	for (i = 0; i < nrpages; i++) {
 		if (pages[i] == NULL)
 			break;
-- 
2.6.3

[lustre-devel] [PATCH 7/7] staging: lustre: Rename a jump label for module_put() calls

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 13 Dec 2015 14:05:57 +0100

This issue was detected by using the Coccinelle software.

Choose a jump label according to the current Linux coding style convention.
I suggest to improve this implementation detail by the reuse of a script
like the following for the semantic patch language.

@rename_jump_label exists@
identifier target != put_module, work;
type return_type;
@@
 return_type work(...)
 {
 ... when any
 goto
-target
+put_module
 ;
 ... when any
-target
+put_module
 :
 module_put(...);
 ... when any
 }

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/lustre/lustre/llite/lloop.c     |  8 ++--
 drivers/staging/lustre/lustre/mdc/mdc_request.c | 52 ++++++++++++-------------
 drivers/staging/lustre/lustre/osc/osc_request.c | 24 ++++++------
 3 files changed, 42 insertions(+), 42 deletions(-)

diff --git a/drivers/staging/lustre/lustre/llite/lloop.c b/drivers/staging/lustre/lustre/llite/lloop.c
index 420d391..ebeef3b 100644
--- a/drivers/staging/lustre/lustre/llite/lloop.c
+++ b/drivers/staging/lustre/lustre/llite/lloop.c
@@ -484,14 +484,14 @@ static int loop_set_fd(struct lloop_device *lo, struct file *unused,
 
 	error = -EBUSY;
 	if (lo->lo_state != LLOOP_UNBOUND)
-		goto out;
+		goto put_module;
 
 	mapping = file->f_mapping;
 	inode = mapping->host;
 
 	error = -EINVAL;
 	if (!S_ISREG(inode->i_mode) || inode->i_sb->s_magic != LL_SUPER_MAGIC)
-		goto out;
+		goto put_module;
 
 	if (!(file->f_mode & FMODE_WRITE))
 		lo_flags |= LO_FLAGS_READ_ONLY;
@@ -500,7 +500,7 @@ static int loop_set_fd(struct lloop_device *lo, struct file *unused,
 
 	if ((loff_t)(sector_t)size != size) {
 		error = -EFBIG;
-		goto out;
+		goto put_module;
 	}
 
 	/* remove all pages in cache so as dirty pages not to be existent. */
@@ -542,7 +542,7 @@ static int loop_set_fd(struct lloop_device *lo, struct file *unused,
 	down(&lo->lo_sem);
 	return 0;
 
-out:
+put_module:
 	/* This is safe: open() is still holding a reference. */
 	module_put(THIS_MODULE);
 	return error;
diff --git a/drivers/staging/lustre/lustre/mdc/mdc_request.c b/drivers/staging/lustre/lustre/mdc/mdc_request.c
index 2085ba6..eaeca9a 100644
--- a/drivers/staging/lustre/lustre/mdc/mdc_request.c
+++ b/drivers/staging/lustre/lustre/mdc/mdc_request.c
@@ -1734,7 +1734,7 @@ static int mdc_iocontrol(unsigned int cmd, struct obd_export *exp, int len,
 	switch (cmd) {
 	case OBD_IOC_CHANGELOG_SEND:
 		rc = mdc_ioc_changelog_send(obd, karg);
-		goto out;
+		goto put_module;
 	case OBD_IOC_CHANGELOG_CLEAR: {
 		struct ioc_changelog *icc = karg;
 		struct changelog_setinfo cs = {
@@ -1745,47 +1745,47 @@ static int mdc_iocontrol(unsigned int cmd, struct obd_export *exp, int len,
 		rc = obd_set_info_async(NULL, exp, strlen(KEY_CHANGELOG_CLEAR),
 					KEY_CHANGELOG_CLEAR, sizeof(cs), &cs,
 					NULL);
-		goto out;
+		goto put_module;
 	}
 	case OBD_IOC_FID2PATH:
 		rc = mdc_ioc_fid2path(exp, karg);
-		goto out;
+		goto put_module;
 	case LL_IOC_HSM_CT_START:
 		rc = mdc_ioc_hsm_ct_start(exp, karg);
 		/* ignore if it was already registered on this MDS. */
 		if (rc == -EEXIST)
 			rc = 0;
-		goto out;
+		goto put_module;
 	case LL_IOC_HSM_PROGRESS:
 		rc = mdc_ioc_hsm_progress(exp, karg);
-		goto out;
+		goto put_module;
 	case LL_IOC_HSM_STATE_GET:
 		rc = mdc_ioc_hsm_state_get(exp, karg);
-		goto out;
+		goto put_module;
 	case LL_IOC_HSM_STATE_SET:
 		rc = mdc_ioc_hsm_state_set(exp, karg);
-		goto out;
+		goto put_module;
 	case LL_IOC_HSM_ACTION:
 		rc = mdc_ioc_hsm_current_action(exp, karg);
-		goto out;
+		goto put_module;
 	case LL_IOC_HSM_REQUEST:
 		rc = mdc_ioc_hsm_request(exp, karg);
-		goto out;
+		goto put_module;
 	case OBD_IOC_CLIENT_RECOVER:
 		rc = ptlrpc_recover_import(imp, data->ioc_inlbuf1, 0);
 		if (rc < 0)
-			goto out;
+			goto put_module;
 		rc = 0;
-		goto out;
+		goto put_module;
 	case IOC_OSC_SET_ACTIVE:
 		rc = ptlrpc_set_import_active(imp, data->ioc_offset);
-		goto out;
+		goto put_module;
 	case OBD_IOC_POLL_QUOTACHECK:
 		rc = mdc_quota_poll_check(exp, (struct if_quotacheck *)karg);
-		goto out;
+		goto put_module;
 	case OBD_IOC_PING_TARGET:
 		rc = ptlrpc_obd_ping(obd);
-		goto out;
+		goto put_module;
 	/*
 	 * Normally IOC_OBD_STATFS, OBD_IOC_QUOTACTL iocontrol are handled by
 	 * LMV instead of MDC. But when the cluster is upgraded from 1.8,
@@ -1798,7 +1798,7 @@ static int mdc_iocontrol(unsigned int cmd, struct obd_export *exp, int len,
 
 		if (*((__u32 *) data->ioc_inlbuf2) != 0) {
 			rc = -ENODEV;
-			goto out;
+			goto put_module;
 		}
 
 		/* copy UUID */
@@ -1806,24 +1806,24 @@ static int mdc_iocontrol(unsigned int cmd, struct obd_export *exp, int len,
 				 min_t(size_t, data->ioc_plen2,
 					       sizeof(struct obd_uuid)))) {
 			rc = -EFAULT;
-			goto out;
+			goto put_module;
 		}
 
 		rc = mdc_statfs(NULL, obd->obd_self_export, &stat_buf,
 				cfs_time_shift_64(-OBD_STATFS_CACHE_SECONDS),
 				0);
 		if (rc != 0)
-			goto out;
+			goto put_module;
 
 		if (copy_to_user(data->ioc_pbuf1, &stat_buf,
 				 min_t(size_t, data->ioc_plen1,
 					       sizeof(stat_buf)))) {
 			rc = -EFAULT;
-			goto out;
+			goto put_module;
 		}
 
 		rc = 0;
-		goto out;
+		goto put_module;
 	}
 	case OBD_IOC_QUOTACTL: {
 		struct if_quotactl *qctl = karg;
@@ -1832,7 +1832,7 @@ static int mdc_iocontrol(unsigned int cmd, struct obd_export *exp, int len,
 		oqctl = kzalloc(sizeof(*oqctl), GFP_NOFS);
 		if (!oqctl) {
 			rc = -ENOMEM;
-			goto out;
+			goto put_module;
 		}
 
 		QCTL_COPY(oqctl, qctl);
@@ -1844,26 +1844,26 @@ static int mdc_iocontrol(unsigned int cmd, struct obd_export *exp, int len,
 		}
 
 		kfree(oqctl);
-		goto out;
+		goto put_module;
 	}
 	case LL_IOC_GET_CONNECT_FLAGS:
 		if (copy_to_user(uarg, exp_connect_flags_ptr(exp),
 				 sizeof(*exp_connect_flags_ptr(exp)))) {
 			rc = -EFAULT;
-			goto out;
+			goto put_module;
 		}
 
 		rc = 0;
-		goto out;
+		goto put_module;
 	case LL_IOC_LOV_SWAP_LAYOUTS:
 		rc = mdc_ioc_swap_layouts(exp, karg);
-		goto out;
+		goto put_module;
 	default:
 		CERROR("unrecognised ioctl: cmd = %#x\n", cmd);
 		rc = -ENOTTY;
-		goto out;
+		goto put_module;
 	}
-out:
+put_module:
 	module_put(THIS_MODULE);
 
 	return rc;
diff --git a/drivers/staging/lustre/lustre/osc/osc_request.c b/drivers/staging/lustre/lustre/osc/osc_request.c
index 3a56fb7..3ee1ff8 100644
--- a/drivers/staging/lustre/lustre/osc/osc_request.c
+++ b/drivers/staging/lustre/lustre/osc/osc_request.c
@@ -2611,7 +2611,7 @@ static int osc_iocontrol(unsigned int cmd, struct obd_export *exp, int len,
 		len = 0;
 		if (obd_ioctl_getdata(&buf, &len, uarg)) {
 			err = -EINVAL;
-			goto out;
+			goto put_module;
 		}
 
 		data = (struct obd_ioctl_data *)buf;
@@ -2619,13 +2619,13 @@ static int osc_iocontrol(unsigned int cmd, struct obd_export *exp, int len,
 		if (sizeof(*desc) > data->ioc_inllen1) {
 			obd_ioctl_freedata(buf, len);
 			err = -EINVAL;
-			goto out;
+			goto put_module;
 		}
 
 		if (data->ioc_inllen2 < sizeof(uuid)) {
 			obd_ioctl_freedata(buf, len);
 			err = -EINVAL;
-			goto out;
+			goto put_module;
 		}
 
 		desc = (struct lov_desc *)data->ioc_inlbuf1;
@@ -2643,39 +2643,39 @@ static int osc_iocontrol(unsigned int cmd, struct obd_export *exp, int len,
 		if (err)
 			err = -EFAULT;
 		obd_ioctl_freedata(buf, len);
-		goto out;
+		goto put_module;
 	}
 	case LL_IOC_LOV_SETSTRIPE:
 		err = obd_alloc_memmd(exp, karg);
 		if (err > 0)
 			err = 0;
-		goto out;
+		goto put_module;
 	case LL_IOC_LOV_GETSTRIPE:
 		err = osc_getstripe(karg, uarg);
-		goto out;
+		goto put_module;
 	case OBD_IOC_CLIENT_RECOVER:
 		err = ptlrpc_recover_import(obd->u.cli.cl_import,
 					    data->ioc_inlbuf1, 0);
 		if (err > 0)
 			err = 0;
-		goto out;
+		goto put_module;
 	case IOC_OSC_SET_ACTIVE:
 		err = ptlrpc_set_import_active(obd->u.cli.cl_import,
 					       data->ioc_offset);
-		goto out;
+		goto put_module;
 	case OBD_IOC_POLL_QUOTACHECK:
 		err = osc_quota_poll_check(exp, karg);
-		goto out;
+		goto put_module;
 	case OBD_IOC_PING_TARGET:
 		err = ptlrpc_obd_ping(obd);
-		goto out;
+		goto put_module;
 	default:
 		CDEBUG(D_INODE, "unrecognised ioctl %#x by %s\n",
 		       cmd, current_comm());
 		err = -ENOTTY;
-		goto out;
+		goto put_module;
 	}
-out:
+put_module:
 	module_put(THIS_MODULE);
 	return err;
 }
-- 
2.6.3

[lustre-devel] [PATCH 00/12] staging-Lustre: Fine-tuning for seven function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>

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

Markus Elfring (12):
  ldlm: Delete unnecessary checks before the function call "kset_unregister"
  Delete unnecessary checks before the function call "kobject_put"
  One function call less in class_register_type() after error detection
  Split a condition check in class_register_type()
  Optimize error handling in class_register_type()
  Return directly after a failed kcalloc() in mgc_process_recover_log()
  Less checks after a failed alloc_page() in mgc_process_recover_log()
  Less checks after a failed ptlrpc_request_alloc() inmgc_process_recover_log()
  Delete a check for the variable "req" in mgc_process_recover_log()
  Rename jump labels in mgc_process_recover_log()
  Move an assignment for the variable "eof" in mgc_process_recover_log()
  Delete an unnecessary variable initialisation in mgc_process_recover_log()

 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/mgc/mgc_request.c | 50 ++++++++++++-------------
 drivers/staging/lustre/lustre/obdclass/genops.c | 41 ++++++++++++--------
 5 files changed, 54 insertions(+), 56 deletions(-)

-- 
2.9.2

[lustre-devel] [PATCH 01/12] staging/lustre/ldlm: Delete unnecessary checks before the function call "kset_unregister"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 26 Jul 2016 11:33:43 +0200

The kset_unregister() function tests whether its argument is NULL
and then returns immediately. Thus the test around the calls is 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 | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c b/drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c
index 821939f..2c1c2fc 100644
--- a/drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c
+++ b/drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c
@@ -1067,10 +1067,8 @@ static int ldlm_cleanup(void)
 	if (ldlm_state->ldlm_cb_service)
 		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);
+	kset_unregister(ldlm_ns_kset);
+	kset_unregister(ldlm_svc_kset);
 	if (ldlm_kobj)
 		kobject_put(ldlm_kobj);
 
-- 
2.9.2

[lustre-devel] [PATCH 02/12] staging: lustre: Delete unnecessary checks before the function call "kobject_put"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 26 Jul 2016 13:00:32 +0200

The kobject_put() function tests whether its argument is NULL and then
returns immediately. Thus the test around the calls is 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 | 4 +---
 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, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c b/drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c
index 2c1c2fc..52c5dd4 100644
--- a/drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c
+++ b/drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c
@@ -1069,9 +1069,7 @@ static int ldlm_cleanup(void)
 
 	kset_unregister(ldlm_ns_kset);
 	kset_unregister(ldlm_svc_kset);
-	if (ldlm_kobj)
-		kobject_put(ldlm_kobj);
-
+	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 0e1588a..8c2e5b3 100644
--- a/drivers/staging/lustre/lustre/lmv/lmv_obd.c
+++ b/drivers/staging/lustre/lustre/lmv/lmv_obd.c
@@ -238,7 +238,7 @@ static int lmv_connect(const struct lu_env *env,
 	if (data && 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;
@@ -648,8 +648,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 9b92d55..df701f7 100644
--- a/drivers/staging/lustre/lustre/lov/lov_obd.c
+++ b/drivers/staging/lustre/lustre/lov/lov_obd.c
@@ -106,9 +106,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 99c2da6..1b5aa9b 100644
--- a/drivers/staging/lustre/lustre/obdclass/genops.c
+++ b/drivers/staging/lustre/lustre/obdclass/genops.c
@@ -203,8 +203,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);
@@ -231,8 +230,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.9.2

[lustre-devel] [PATCH 03/12] staging: lustre: One function call less in class_register_type() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 26 Jul 2016 13:40:47 +0200

The kobject_put() function was called in a few cases by the
class_register_type() function during error handling even if the passed
data structure element did not contain a pointer for a valid data item.

Adjust jump targets according to the Linux coding style convention.

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

diff --git a/drivers/staging/lustre/lustre/obdclass/genops.c b/drivers/staging/lustre/lustre/obdclass/genops.c
index 1b5aa9b..10dd145 100644
--- a/drivers/staging/lustre/lustre/obdclass/genops.c
+++ b/drivers/staging/lustre/lustre/obdclass/genops.c
@@ -164,7 +164,7 @@ int class_register_type(struct obd_ops *dt_ops, struct md_ops *md_ops,
 	if (!type->typ_dt_ops ||
 	    !type->typ_md_ops ||
 	    !type->typ_name)
-		goto failed;
+		goto free_name;
 
 	*(type->typ_dt_ops) = *dt_ops;
 	/* md_ops is optional */
@@ -180,20 +180,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) {
 		type->typ_lu = ldt;
 		rc = lu_device_type_init(ldt);
 		if (rc != 0)
-			goto failed;
+			goto put_object;
 	}
 
 	spin_lock(&obd_types_lock);
@@ -201,9 +201,9 @@ 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);
 	kfree(type->typ_md_ops);
 	kfree(type->typ_dt_ops);
-- 
2.9.2

[lustre-devel] [PATCH 03/12] staging: lustre: One function call less in class_register_type() after error detection

[Oleg Drokin]

On Jul 26, 2016, at 3:02 PM, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 26 Jul 2016 13:40:47 +0200
> 
> The kobject_put() function was called in a few cases by the
> class_register_type() function during error handling even if the passed
> data structure element did not contain a pointer for a valid data item.

But kobject_put() already checks for NULL, right? you just submitted
another batch about that in other area.

> Adjust jump targets according to the Linux coding style convention.

Not that I am totally against this patch, but when we do not need the extra
checks, a single jump target is ok too in my mind (extra benefit - there's
not going to be any chance of a mistake to where to jump to).
And when we have a single jump target, there's no supersmart naming
like free_this_and_that_and_that_other_thing_too.

> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> drivers/staging/lustre/lustre/obdclass/genops.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/obdclass/genops.c b/drivers/staging/lustre/lustre/obdclass/genops.c
> index 1b5aa9b..10dd145 100644
> --- a/drivers/staging/lustre/lustre/obdclass/genops.c
> +++ b/drivers/staging/lustre/lustre/obdclass/genops.c
> @@ -164,7 +164,7 @@ int class_register_type(struct obd_ops *dt_ops, struct md_ops *md_ops,
> 	if (!type->typ_dt_ops ||
> 	    !type->typ_md_ops ||
> 	    !type->typ_name)
> -		goto failed;
> +		goto free_name;
> 
> 	*(type->typ_dt_ops) = *dt_ops;
> 	/* md_ops is optional */
> @@ -180,20 +180,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) {
> 		type->typ_lu = ldt;
> 		rc = lu_device_type_init(ldt);
> 		if (rc != 0)
> -			goto failed;
> +			goto put_object;
> 	}
> 
> 	spin_lock(&obd_types_lock);
> @@ -201,9 +201,9 @@ 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);
> 	kfree(type->typ_md_ops);
> 	kfree(type->typ_dt_ops);
> -- 
> 2.9.2

[lustre-devel] staging: lustre: One function call less in class_register_type() after error detection

[SF Markus Elfring]

> But kobject_put() already checks for NULL, right?

Yes. - Such an input parameter validation is performed by the
function implementation.


> you just submitted another batch about that in other area.

I sent update suggestions because of this function property for two
Linux software modules in the year 2015.


>> Adjust jump targets according to the Linux coding style convention.
> 
> Not that I am totally against this patch,

Thanks for your feedback.


> but when we do not need the extra checks, a single jump target is ok too in my mind

A single goto label will look convenient for a while. It will often work
for several use cases.


> (extra benefit - there's not going to be any chance of a mistake to where to jump to).

I have got an other opinion when you would like to care for a bit
more software efficiency.


> And when we have a single jump target, there's no supersmart naming
> like free_this_and_that_and_that_other_thing_too.

How often do you care for efficient exception handling in the shown
function implementations?

Regards,
Markus

[lustre-devel] staging: lustre: One function call less in class_register_type() after error detection

[Oleg Drokin]

On Jul 26, 2016, at 3:56 PM, SF Markus Elfring wrote:

>> But kobject_put() already checks for NULL, right?
> 
> Yes. - Such an input parameter validation is performed by the
> function implementation.
> 
> 
>> you just submitted another batch about that in other area.
> 
> I sent update suggestions because of this function property for two
> Linux software modules in the year 2015.
> 
> 
>>> Adjust jump targets according to the Linux coding style convention.
>> 
>> Not that I am totally against this patch,
> 
> Thanks for your feedback.
> 
> 
>> but when we do not need the extra checks, a single jump target is ok too in my mind
> 
> A single goto label will look convenient for a while. It will often work
> for several use cases.
> 
> 
>> (extra benefit - there's not going to be any chance of a mistake to where to jump to).
> 
> I have got an other opinion when you would like to care for a bit
> more software efficiency.
> 
> 
>> And when we have a single jump target, there's no supersmart naming
>> like free_this_and_that_and_that_other_thing_too.
> 
> How often do you care for efficient exception handling in the shown
> function implementations?

This function is called several times during lustre module insert.
Namely it's called 5 times for 5 types:
osc, mdc, lov, lmv, mgc.

It's not called any more than that, so it's not exactly a super hot-path function
to overoptimize it, and the failure is presumed to never happen too
(or the module would be non-functional).

I guess you have already did all the work so I don't have any principal objections
here, it's just like I said, in a non-super contended path, a single
"fail" label is probably easier on the developer when they need to add another
check there, as opposed to figuring and possibly adding a correct another
label that would do something sensible.

Thank you for your contributions.

[lustre-devel] staging: lustre: One function call less in class_register_type() after error detection

[SF Markus Elfring]

> This function is called several times during lustre module insert.
> Namely it's called 5 times for 5 types:
> osc, mdc, lov, lmv, mgc.

Will any extra memory accesses matter for the successful execution
in this use case?


> It's not called any more than that, so it's not exactly a super hot-path function
> to overoptimize it, and the failure is presumed to never happen too
> (or the module would be non-functional).

Did the assignment for the local variable "rc" with a well-known error code
influence the run-time characteristics in unwanted ways?
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/staging/lustre/lustre/obdclass/genops.c?id=6a5b99a46bedc2cfbba96dec6d255c4b90af9ff8#n140

Regards,
Markus

[lustre-devel] staging: lustre: One function call less in class_register_type() after error detection

[Oleg Drokin]

On Jul 28, 2016, at 1:53 AM, SF Markus Elfring wrote:

>> This function is called several times during lustre module insert.
>> Namely it's called 5 times for 5 types:
>> osc, mdc, lov, lmv, mgc.
> 
> Will any extra memory accesses matter for the successful execution
> in this use case?

I doubt it.

In typical deployments outside of testing environment, this function is
called 5 times every system boot and never again.

>> It's not called any more than that, so it's not exactly a super hot-path function
>> to overoptimize it, and the failure is presumed to never happen too
>> (or the module would be non-functional).
> 
> Did the assignment for the local variable "rc" with a well-known error code
> influence the run-time characteristics in unwanted ways?
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/staging/lustre/lustre/obdclass/genops.c?id=6a5b99a46bedc2cfbba96dec6d255c4b90af9ff8#n140

I am not sure what do you mean here.

[lustre-devel] staging: lustre: One function call less in class_register_type() after error detection

[SF Markus Elfring]

> In typical deployments outside of testing environment, this function is
> called 5 times every system boot and never again.

Does this information mean that a bit more fine-tuning is insignificant
at such a source code place?


>> Did the assignment for the local variable "rc" with a well-known error code
>> influence the run-time characteristics in unwanted ways?
>> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/staging/lustre/lustre/obdclass/genops.c?id=6a5b99a46bedc2cfbba96dec6d255c4b90af9ff8#n140
> 
> I am not sure what do you mean here.

I suggest to take another look at corresponding implementation details.

An error code is assigned to the variable "rc" before four memory
allocations succeeded so far.
We hope that this function will usually return zero as a constant
for the indication of a successful execution. I find that this variable
should not be touched in the preferred case.
Will such an unnecessary assignment reduce the execution speed a bit
for the desired file system initialisation?

Regards,
Markus

[lustre-devel] [PATCH 04/12] staging: lustre: Split a condition check in class_register_type()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 26 Jul 2016 14:10:55 +0200

The kfree() function was called in up to three cases by the
class_register_type() function during error handling even if
the passed data structure element contained a null pointer.

* Split a condition check for memory allocation failures.

* Improve this implementation detail by the introduction of a few
  jump labels.

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

diff --git a/drivers/staging/lustre/lustre/obdclass/genops.c b/drivers/staging/lustre/lustre/obdclass/genops.c
index 10dd145..fd5e61f 100644
--- a/drivers/staging/lustre/lustre/obdclass/genops.c
+++ b/drivers/staging/lustre/lustre/obdclass/genops.c
@@ -158,13 +158,22 @@ 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) {
+		rc = -ENOMEM;
+		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_dt_ops) {
+		rc = -ENOMEM;
+		goto free_dt_ops;
+	}
 
-	if (!type->typ_dt_ops ||
-	    !type->typ_md_ops ||
-	    !type->typ_name)
-		goto free_name;
+	type->typ_name = kzalloc(strlen(name) + 1, GFP_NOFS);
+	if (!type->typ_name) {
+		rc = -ENOMEM;
+		goto free_md_ops;
+	}
 
 	*(type->typ_dt_ops) = *dt_ops;
 	/* md_ops is optional */
@@ -205,8 +214,11 @@ 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.9.2

[lustre-devel] [PATCH 05/12] staging: lustre: Optimize error handling in class_register_type()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 26 Jul 2016 14:23:23 +0200

Return a constant error code without storing it in the local variable "rc"
after a failed memory allocation at the beginning of this function.

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

diff --git a/drivers/staging/lustre/lustre/obdclass/genops.c b/drivers/staging/lustre/lustre/obdclass/genops.c
index fd5e61f..4752091 100644
--- a/drivers/staging/lustre/lustre/obdclass/genops.c
+++ b/drivers/staging/lustre/lustre/obdclass/genops.c
@@ -152,10 +152,9 @@ int class_register_type(struct obd_ops *dt_ops, struct md_ops *md_ops,
 		return -EEXIST;
 	}
 
-	rc = -ENOMEM;
 	type = kzalloc(sizeof(*type), GFP_NOFS);
 	if (!type)
-		return rc;
+		return -ENOMEM;
 
 	type->typ_dt_ops = kzalloc(sizeof(*type->typ_dt_ops), GFP_NOFS);
 	if (!type->typ_dt_ops) {
-- 
2.9.2

[lustre-devel] [PATCH 05/12] staging: lustre: Optimize error handling in class_register_type()

[Oleg Drokin]

On Jul 26, 2016, at 3:05 PM, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 26 Jul 2016 14:23:23 +0200
> 
> Return a constant error code without storing it in the local variable "rc"
> after a failed memory allocation at the beginning of this function.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> drivers/staging/lustre/lustre/obdclass/genops.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/obdclass/genops.c b/drivers/staging/lustre/lustre/obdclass/genops.c
> index fd5e61f..4752091 100644
> --- a/drivers/staging/lustre/lustre/obdclass/genops.c
> +++ b/drivers/staging/lustre/lustre/obdclass/genops.c
> @@ -152,10 +152,9 @@ int class_register_type(struct obd_ops *dt_ops, struct md_ops *md_ops,
> 		return -EEXIST;
> 	}
> 
> -	rc = -ENOMEM;

NAK.
when you do this, the next statement below breaks:

> 	type = kzalloc(sizeof(*type), GFP_NOFS);
> 	if (!type)
> -		return rc;
> +		return -ENOMEM;
> 
> 	type->typ_dt_ops = kzalloc(sizeof(*type->typ_dt_ops), GFP_NOFS);
> 	if (!type->typ_dt_ops) {
?
                goto failed;

 failed:
?
return rc;

So we are now returning an unitialized rc, did you get a gcc warning about it when compiling?

[lustre-devel] [PATCH 05/12] staging: lustre: Optimize error handling in class_register_type()

[Oleg Drokin]

On Jul 26, 2016, at 3:11 PM, Oleg Drokin wrote:

> 
> On Jul 26, 2016, at 3:05 PM, SF Markus Elfring wrote:
> 
>> From: Markus Elfring <elfring@users.sourceforge.net>
>> Date: Tue, 26 Jul 2016 14:23:23 +0200
>> 
>> Return a constant error code without storing it in the local variable "rc"
>> after a failed memory allocation at the beginning of this function.
>> 
>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>> ---
>> drivers/staging/lustre/lustre/obdclass/genops.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>> 
>> diff --git a/drivers/staging/lustre/lustre/obdclass/genops.c b/drivers/staging/lustre/lustre/obdclass/genops.c
>> index fd5e61f..4752091 100644
>> --- a/drivers/staging/lustre/lustre/obdclass/genops.c
>> +++ b/drivers/staging/lustre/lustre/obdclass/genops.c
>> @@ -152,10 +152,9 @@ int class_register_type(struct obd_ops *dt_ops, struct md_ops *md_ops,
>> 		return -EEXIST;
>> 	}
>> 
>> -	rc = -ENOMEM;
> 
> NAK.
> when you do this, the next statement below breaks:

Ah, I see there was patch 4 before patch 5 that actually placed rc assignments
everywhere.

So I guess it is fine after all.

Sorry for the noise.

> 
>> 	type = kzalloc(sizeof(*type), GFP_NOFS);
>> 	if (!type)
>> -		return rc;
>> +		return -ENOMEM;
>> 
>> 	type->typ_dt_ops = kzalloc(sizeof(*type->typ_dt_ops), GFP_NOFS);
>> 	if (!type->typ_dt_ops) {
> ?
>                goto failed;
> 
> failed:
> ?
> return rc;
> 
> So we are now returning an unitialized rc, did you get a gcc warning about it when compiling?
> _______________________________________________
> lustre-devel mailing list
> lustre-devel at lists.lustre.org
> http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org

[lustre-devel] staging: lustre: Optimize error handling in class_register_type()

[SF Markus Elfring]

> NAK.
> when you do this, the next statement below breaks:

I wonder about this conclusion.


>> 	type = kzalloc(sizeof(*type), GFP_NOFS);
>> 	if (!type)
>> -		return rc;
>> +		return -ENOMEM;
>>
>> 	type->typ_dt_ops = kzalloc(sizeof(*type->typ_dt_ops), GFP_NOFS);
>> 	if (!type->typ_dt_ops) {
> ?
>                 goto failed;
> 
>  failed:
> ?
> return rc;
> 
> So we are now returning an unitialized rc, did you get a gcc warning about it when compiling?

I do not get such an impression if my corresponding update suggestion
"[PATCH 04/12] staging: lustre: Split a condition check in class_register_type()"
will be considered for this use case once more.
https://lkml.org/lkml/2016/7/26/462
https://www.mail-archive.com/linux-kernel at vger.kernel.org/msg1197227.html

Regards,
Markus

[lustre-devel] [PATCH 06/12] staging: lustre: Return directly after a failed kcalloc() in mgc_process_recover_log()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 26 Jul 2016 16:32:31 +0200

Return directly after a memory allocation failed at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/lustre/lustre/mgc/mgc_request.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/lustre/lustre/mgc/mgc_request.c b/drivers/staging/lustre/lustre/mgc/mgc_request.c
index 9d0bd47..d716bb2 100644
--- a/drivers/staging/lustre/lustre/mgc/mgc_request.c
+++ b/drivers/staging/lustre/lustre/mgc/mgc_request.c
@@ -1339,10 +1339,8 @@ static int mgc_process_recover_log(struct obd_device *obd,
 		nrpages = CONFIG_READ_NRPAGES_INIT;
 
 	pages = kcalloc(nrpages, sizeof(*pages), GFP_KERNEL);
-	if (!pages) {
-		rc = -ENOMEM;
-		goto out;
-	}
+	if (!pages)
+		return -ENOMEM;
 
 	for (i = 0; i < nrpages; i++) {
 		pages[i] = alloc_page(GFP_KERNEL);
-- 
2.9.2

[lustre-devel] [PATCH 07/12] staging: lustre: Less checks after a failed alloc_page() in mgc_process_recover_log()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 26 Jul 2016 17:12:51 +0200

Release memory directly after a page allocation failed at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/lustre/lustre/mgc/mgc_request.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/lustre/lustre/mgc/mgc_request.c b/drivers/staging/lustre/lustre/mgc/mgc_request.c
index d716bb2..b064bd3 100644
--- a/drivers/staging/lustre/lustre/mgc/mgc_request.c
+++ b/drivers/staging/lustre/lustre/mgc/mgc_request.c
@@ -1346,7 +1346,7 @@ static int mgc_process_recover_log(struct obd_device *obd,
 		pages[i] = alloc_page(GFP_KERNEL);
 		if (!pages[i]) {
 			rc = -ENOMEM;
-			goto out;
+			goto free_pages;
 		}
 	}
 
@@ -1461,14 +1461,13 @@ out:
 	if (rc == 0 && !eof)
 		goto again;
 
-	if (pages) {
-		for (i = 0; i < nrpages; i++) {
-			if (!pages[i])
-				break;
-			__free_page(pages[i]);
-		}
-		kfree(pages);
+free_pages:
+	for (i = 0; i < nrpages; i++) {
+		if (!pages[i])
+			break;
+		__free_page(pages[i]);
 	}
+	kfree(pages);
 	return rc;
 }
 
-- 
2.9.2

[lustre-devel] [PATCH 08/12] staging: lustre: Less checks after a failed ptlrpc_request_alloc() in mgc_process_recover_log()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 26 Jul 2016 17:54:24 +0200

Release memory directly after a call of the function
"ptlrpc_request_alloc" failed.

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

diff --git a/drivers/staging/lustre/lustre/mgc/mgc_request.c b/drivers/staging/lustre/lustre/mgc/mgc_request.c
index b064bd3..f65bb45 100644
--- a/drivers/staging/lustre/lustre/mgc/mgc_request.c
+++ b/drivers/staging/lustre/lustre/mgc/mgc_request.c
@@ -1357,7 +1357,7 @@ again:
 				   &RQF_MGS_CONFIG_READ);
 	if (!req) {
 		rc = -ENOMEM;
-		goto out;
+		goto free_pages;
 	}
 
 	rc = ptlrpc_request_pack(req, LUSTRE_MGS_VERSION, MGS_CONFIG_READ);
-- 
2.9.2

[lustre-devel] [PATCH 09/12] staging: lustre: Delete a check for the variable "req" in mgc_process_recover_log()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 26 Jul 2016 17:56:49 +0200

Delete a check for the local variable "req" which became unnecessary with
the previous update step.

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

diff --git a/drivers/staging/lustre/lustre/mgc/mgc_request.c b/drivers/staging/lustre/lustre/mgc/mgc_request.c
index f65bb45..2207af7 100644
--- a/drivers/staging/lustre/lustre/mgc/mgc_request.c
+++ b/drivers/staging/lustre/lustre/mgc/mgc_request.c
@@ -1455,8 +1455,7 @@ again:
 	}
 
 out:
-	if (req)
-		ptlrpc_req_finished(req);
+	ptlrpc_req_finished(req);
 
 	if (rc == 0 && !eof)
 		goto again;
-- 
2.9.2

[lustre-devel] [PATCH 10/12] staging: lustre: Rename jump labels in mgc_process_recover_log()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 26 Jul 2016 19:29:11 +0200

Adjust jump targets according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/lustre/lustre/mgc/mgc_request.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/lustre/lustre/mgc/mgc_request.c b/drivers/staging/lustre/lustre/mgc/mgc_request.c
index 2207af7..ff60b9b 100644
--- a/drivers/staging/lustre/lustre/mgc/mgc_request.c
+++ b/drivers/staging/lustre/lustre/mgc/mgc_request.c
@@ -1362,7 +1362,7 @@ again:
 
 	rc = ptlrpc_request_pack(req, LUSTRE_MGS_VERSION, MGS_CONFIG_READ);
 	if (rc)
-		goto out;
+		goto finish_request;
 
 	/* pack request */
 	body = req_capsule_client_get(&req->rq_pill, &RMF_MGS_CONFIG_BODY);
@@ -1370,7 +1370,7 @@ again:
 	if (strlcpy(body->mcb_name, cld->cld_logname, sizeof(body->mcb_name))
 	    >= sizeof(body->mcb_name)) {
 		rc = -E2BIG;
-		goto out;
+		goto finish_request;
 	}
 	body->mcb_offset = cfg->cfg_last_idx + 1;
 	body->mcb_type   = cld->cld_type;
@@ -1382,7 +1382,7 @@ again:
 				    MGS_BULK_PORTAL);
 	if (!desc) {
 		rc = -ENOMEM;
-		goto out;
+		goto finish_request;
 	}
 
 	for (i = 0; i < nrpages; i++)
@@ -1391,12 +1391,12 @@ again:
 	ptlrpc_request_set_replen(req);
 	rc = ptlrpc_queue_wait(req);
 	if (rc)
-		goto out;
+		goto finish_request;
 
 	res = req_capsule_server_get(&req->rq_pill, &RMF_MGS_CONFIG_RES);
 	if (res->mcr_size < res->mcr_offset) {
 		rc = -EINVAL;
-		goto out;
+		goto finish_request;
 	}
 
 	/* always update the index even though it might have errors with
@@ -1411,18 +1411,18 @@ again:
 	ealen = sptlrpc_cli_unwrap_bulk_read(req, req->rq_bulk, 0);
 	if (ealen < 0) {
 		rc = ealen;
-		goto out;
+		goto finish_request;
 	}
 
 	if (ealen > nrpages << PAGE_SHIFT) {
 		rc = -EINVAL;
-		goto out;
+		goto finish_request;
 	}
 
 	if (ealen == 0) { /* no logs transferred */
 		if (!eof)
 			rc = -EINVAL;
-		goto out;
+		goto finish_request;
 	}
 
 	mne_swab = !!ptlrpc_rep_need_swab(req);
@@ -1453,8 +1453,7 @@ again:
 
 		ealen -= PAGE_SIZE;
 	}
-
-out:
+finish_request:
 	ptlrpc_req_finished(req);
 
 	if (rc == 0 && !eof)
-- 
2.9.2

[lustre-devel] [PATCH 11/12] staging: lustre: Move an assignment for the variable "eof" in mgc_process_recover_log()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 26 Jul 2016 19:40:28 +0200

Move the assignment for the local variable "eof" behind the source code
for memory allocations by this function.

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

diff --git a/drivers/staging/lustre/lustre/mgc/mgc_request.c b/drivers/staging/lustre/lustre/mgc/mgc_request.c
index ff60b9b..8f5db79 100644
--- a/drivers/staging/lustre/lustre/mgc/mgc_request.c
+++ b/drivers/staging/lustre/lustre/mgc/mgc_request.c
@@ -1322,7 +1322,7 @@ static int mgc_process_recover_log(struct obd_device *obd,
 	struct ptlrpc_bulk_desc *desc;
 	struct page **pages;
 	int nrpages;
-	bool eof = true;
+	bool eof;
 	bool mne_swab;
 	int i;
 	int ealen;
@@ -1350,6 +1350,7 @@ static int mgc_process_recover_log(struct obd_device *obd,
 		}
 	}
 
+	eof = true;
 again:
 	LASSERT(cld_is_recover(cld));
 	LASSERT(mutex_is_locked(&cld->cld_lock));
-- 
2.9.2

[lustre-devel] [PATCH 12/12] staging: lustre: Delete an unnecessary variable initialisation in mgc_process_recover_log()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 26 Jul 2016 19:50:40 +0200

The variable "req" will eventually be set to an appropriate pointer
from a call of the ptlrpc_request_alloc() function.
Thus omit the explicit initialisation which became unnecessary with
a previous update step.

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

diff --git a/drivers/staging/lustre/lustre/mgc/mgc_request.c b/drivers/staging/lustre/lustre/mgc/mgc_request.c
index 8f5db79..6e8368e 100644
--- a/drivers/staging/lustre/lustre/mgc/mgc_request.c
+++ b/drivers/staging/lustre/lustre/mgc/mgc_request.c
@@ -1315,7 +1315,7 @@ static int mgc_apply_recover_logs(struct obd_device *mgc,
 static int mgc_process_recover_log(struct obd_device *obd,
 				   struct config_llog_data *cld)
 {
-	struct ptlrpc_request *req = NULL;
+	struct ptlrpc_request *req;
 	struct config_llog_instance *cfg = &cld->cld_cfg;
 	struct mgs_config_body *body;
 	struct mgs_config_res  *res;
-- 
2.9.2

[lustre-devel] [PATCH] staging/lustre/llite: Use memdup_user() rather than duplicating its implementation

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 21 Aug 2016 11:30:57 +0200

Reuse existing functionality from memdup_user() instead of keeping
duplicate source code.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/lustre/lustre/llite/dir.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/lustre/lustre/llite/dir.c b/drivers/staging/lustre/lustre/llite/dir.c
index 031c9e4..8b70e42 100644
--- a/drivers/staging/lustre/lustre/llite/dir.c
+++ b/drivers/staging/lustre/lustre/llite/dir.c
@@ -1676,14 +1676,9 @@ out_poll:
 	case LL_IOC_QUOTACTL: {
 		struct if_quotactl *qctl;
 
-		qctl = kzalloc(sizeof(*qctl), GFP_NOFS);
-		if (!qctl)
-			return -ENOMEM;
-
-		if (copy_from_user(qctl, (void __user *)arg, sizeof(*qctl))) {
-			rc = -EFAULT;
-			goto out_quotactl;
-		}
+		qctl = memdup_user((void __user *)arg, sizeof(*qctl));
+		if (IS_ERR(qctl))
+			return PTR_ERR(qctl);
 
 		rc = quotactl_ioctl(sbi, qctl);
 
@@ -1691,7 +1686,6 @@ out_poll:
 					    sizeof(*qctl)))
 			rc = -EFAULT;
 
-out_quotactl:
 		kfree(qctl);
 		return rc;
 	}
-- 
2.9.3