[PATCH] staging : lustre : Remove braces from single-line body.

[PATCH] staging : lustre : Remove braces from single-line body.

[Tabrez khan]

Remove unnecessary braces {} for single while statement.
This warning is found using checkpatch.pl.

Signed-off-by: Tabrez khan <khan.tabrez21@gmail.com>
---
 drivers/staging/lustre/lustre/obdclass/cl_io.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/lustre/lustre/obdclass/cl_io.c b/drivers/staging/lustre/lustre/obdclass/cl_io.c
index bc4b7b6..2daede8 100644
--- a/drivers/staging/lustre/lustre/obdclass/cl_io.c
+++ b/drivers/staging/lustre/lustre/obdclass/cl_io.c
@@ -1371,9 +1371,9 @@ int cl_sync_io_wait(const struct lu_env *env, struct cl_sync_io *anchor,
 	LASSERT(atomic_read(&anchor->csi_sync_nr) == 0);
 
 	/* wait until cl_sync_io_note() has done wakeup */
-	while (unlikely(atomic_read(&anchor->csi_barrier) != 0)) {
+	while (unlikely(atomic_read(&anchor->csi_barrier) != 0))
 		cpu_relax();
-	}
+
 
 	return rc;
 }
-- 
2.7.4

[lustre-devel] [PATCH] staging : lustre : Remove braces from single-line body.

[Tabrez khan]

Remove unnecessary braces {} for single while statement.
This warning is found using checkpatch.pl.

Signed-off-by: Tabrez khan <khan.tabrez21@gmail.com>
---
 drivers/staging/lustre/lustre/obdclass/cl_io.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/lustre/lustre/obdclass/cl_io.c b/drivers/staging/lustre/lustre/obdclass/cl_io.c
index bc4b7b6..2daede8 100644
--- a/drivers/staging/lustre/lustre/obdclass/cl_io.c
+++ b/drivers/staging/lustre/lustre/obdclass/cl_io.c
@@ -1371,9 +1371,9 @@ int cl_sync_io_wait(const struct lu_env *env, struct cl_sync_io *anchor,
 	LASSERT(atomic_read(&anchor->csi_sync_nr) == 0);
 
 	/* wait until cl_sync_io_note() has done wakeup */
-	while (unlikely(atomic_read(&anchor->csi_barrier) != 0)) {
+	while (unlikely(atomic_read(&anchor->csi_barrier) != 0))
 		cpu_relax();
-	}
+
 
 	return rc;
 }
-- 
2.7.4

Re: [PATCH] staging : lustre : Remove braces from single-line body.

[Joe Perches]

On Fri, 2016-12-16 at 19:59 +0530, Tabrez khan wrote:
> Remove unnecessary braces {} for single while statement.

Your patch is fine Tabrez, but to the lustre folk:

> diff --git a/drivers/staging/lustre/lustre/obdclass/cl_io.c b/drivers/staging/lustre/lustre/obdclass/cl_io.c
[]
> @@ -1371,9 +1371,9 @@ int cl_sync_io_wait(const struct lu_env *env, struct cl_sync_io *anchor,
 	LASSERT(atomic_read(&anchor->csi_sync_nr) == 0);
 
 	/* wait until cl_sync_io_note() has done wakeup */
-	while (unlikely(atomic_read(&anchor->csi_barrier) != 0)) {
+	while (unlikely(atomic_read(&anchor->csi_barrier) != 0))
 		cpu_relax();
-	}
+

What if the wakeup never occurs/succeeds?
Should there be a timeout?

[lustre-devel] [PATCH] staging : lustre : Remove braces from single-line body.

[Joe Perches]

On Fri, 2016-12-16 at 19:59 +0530, Tabrez khan wrote:
> Remove unnecessary braces {} for single while statement.

Your patch is fine Tabrez, but to the lustre folk:

> diff --git a/drivers/staging/lustre/lustre/obdclass/cl_io.c b/drivers/staging/lustre/lustre/obdclass/cl_io.c
[]
> @@ -1371,9 +1371,9 @@ int cl_sync_io_wait(const struct lu_env *env, struct cl_sync_io *anchor,
 	LASSERT(atomic_read(&anchor->csi_sync_nr) == 0);
 
 	/* wait until cl_sync_io_note() has done wakeup */
-	while (unlikely(atomic_read(&anchor->csi_barrier) != 0)) {
+	while (unlikely(atomic_read(&anchor->csi_barrier) != 0))
 		cpu_relax();
-	}
+

What if the wakeup never occurs/succeeds?
Should there be a timeout?

Re: [lustre-devel] [PATCH] staging : lustre : Remove braces from single-line body.

[Xiong, Jinshan]

> On Dec 16, 2016, at 8:05 AM, Joe Perches <joe@perches.com> wrote:
> 
> On Fri, 2016-12-16 at 19:59 +0530, Tabrez khan wrote:
>> Remove unnecessary braces {} for single while statement.
> 
> Your patch is fine Tabrez, but to the lustre folk:
> 
>> diff --git a/drivers/staging/lustre/lustre/obdclass/cl_io.c b/drivers/staging/lustre/lustre/obdclass/cl_io.c
> []
>> @@ -1371,9 +1371,9 @@ int cl_sync_io_wait(const struct lu_env *env, struct cl_sync_io *anchor,
> 	LASSERT(atomic_read(&anchor->csi_sync_nr) == 0);
> 
> 	/* wait until cl_sync_io_note() has done wakeup */
> -	while (unlikely(atomic_read(&anchor->csi_barrier) != 0)) {
> +	while (unlikely(atomic_read(&anchor->csi_barrier) != 0))
> 		cpu_relax();
> -	}
> +
> 
> What if the wakeup never occurs/succeeds?
> Should there be a timeout?

There is no wakeup at all. This piece of code is to solve the preempting race condition in cl_sync_io_end(), where it calls wake_up_all() to wake up the cl_sync_io_wait() process, and then is preempted _inside_ wake_up_all(), and then cl_sync_io_wait() process gains the CPU and frees memory cl_sync_io. Therefore when cl_sync_io_end() comes back to finish its work in wake_up_all(), a piece of freed memory will be accessed.

csi_barrier is proposed to solve this problem, which makes sure wake_up_all() is complete before cl_sync_io_wait() can continue. It should be a really short time so it’s reasonable for cl_sync_io_wait() to do a busy loop wait.

Jinshan

> _______________________________________________
> lustre-devel mailing list
> lustre-devel@lists.lustre.org
> http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org

[lustre-devel] [PATCH] staging : lustre : Remove braces from single-line body.

[Xiong, Jinshan]

> On Dec 16, 2016, at 8:05 AM, Joe Perches <joe@perches.com> wrote:
> 
> On Fri, 2016-12-16 at 19:59 +0530, Tabrez khan wrote:
>> Remove unnecessary braces {} for single while statement.
> 
> Your patch is fine Tabrez, but to the lustre folk:
> 
>> diff --git a/drivers/staging/lustre/lustre/obdclass/cl_io.c b/drivers/staging/lustre/lustre/obdclass/cl_io.c
> []
>> @@ -1371,9 +1371,9 @@ int cl_sync_io_wait(const struct lu_env *env, struct cl_sync_io *anchor,
> 	LASSERT(atomic_read(&anchor->csi_sync_nr) == 0);
> 
> 	/* wait until cl_sync_io_note() has done wakeup */
> -	while (unlikely(atomic_read(&anchor->csi_barrier) != 0)) {
> +	while (unlikely(atomic_read(&anchor->csi_barrier) != 0))
> 		cpu_relax();
> -	}
> +
> 
> What if the wakeup never occurs/succeeds?
> Should there be a timeout?

There is no wakeup at all. This piece of code is to solve the preempting race condition in cl_sync_io_end(), where it calls wake_up_all() to wake up the cl_sync_io_wait() process, and then is preempted _inside_ wake_up_all(), and then cl_sync_io_wait() process gains the CPU and frees memory cl_sync_io. Therefore when cl_sync_io_end() comes back to finish its work in wake_up_all(), a piece of freed memory will be accessed.

csi_barrier is proposed to solve this problem, which makes sure wake_up_all() is complete before cl_sync_io_wait() can continue. It should be a really short time so it?s reasonable for cl_sync_io_wait() to do a busy loop wait.

Jinshan

> _______________________________________________
> lustre-devel mailing list
> lustre-devel at lists.lustre.org
> http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org

Re: [PATCH] staging : lustre : Remove braces from single-line body.

[Greg KH]

On Wed, Dec 28, 2016 at 07:40:09PM +0530, Tabrez khan wrote:
> Remove unnecessary braces from single-line if statement.
> This warning is found using checkpatch.pl.
> 
> Signed-off-by: Tabrez khan <khan.tabrez21@gmail.com>
> ---
>  drivers/staging/lustre/lustre/ptlrpc/import.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

You have sent me 5 patches, with 4 of them having duplicated subject
lines, yet they do different things.  Also, what order am I supposed to
apply these patches in?

Please resend them as a patch series, properly numbered, and with unique
subjects.

thanks,

greg k-h

[PATCH] staging:lustre:remove braces from single-line body

[Tabrez khan]

Remove unnecessary braces from single line if statement.
This warning is found using checkpatch.pl.

Signed-off-by: Tabrez khan <khan.tabrez21@gmail.com>
---
 drivers/staging/lustre/lustre/obdecho/echo_client.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/lustre/lustre/obdecho/echo_client.c b/drivers/staging/lustre/lustre/obdecho/echo_client.c
index 505582f..3050fa7 100644
--- a/drivers/staging/lustre/lustre/obdecho/echo_client.c
+++ b/drivers/staging/lustre/lustre/obdecho/echo_client.c
@@ -1694,9 +1694,9 @@ static int echo_client_connect(const struct lu_env *env,
 	struct lustre_handle conn = { 0 };
 
 	rc = class_connect(&conn, src, cluuid);
-	if (rc == 0) {
+	if (rc == 0)
 		*exp = class_conn2export(&conn);
-	}
+
 
 	return rc;
 }
-- 
2.7.4

[PATCH] staging : lustre : Remove braces from single-line body.

[Tabrez khan]

Remove unnecessary braces from single-line if statement.
This warning is found using checkpatch.pl.

Signed-off-by: Tabrez khan <khan.tabrez21@gmail.com>
---
 drivers/staging/lustre/lustre/ptlrpc/import.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/lustre/lustre/ptlrpc/import.c b/drivers/staging/lustre/lustre/ptlrpc/import.c
index 477d832..b55680b 100644
--- a/drivers/staging/lustre/lustre/ptlrpc/import.c
+++ b/drivers/staging/lustre/lustre/ptlrpc/import.c
@@ -1392,9 +1392,9 @@ int ptlrpc_import_recovery_state_machine(struct obd_import *imp)
 	}
 
 	if (imp->imp_state == LUSTRE_IMP_REPLAY_WAIT) {
-		if (atomic_read(&imp->imp_replay_inflight) == 0) {
+		if (atomic_read(&imp->imp_replay_inflight) == 0)
 			IMPORT_SET_STATE(imp, LUSTRE_IMP_RECOVER);
-		}
+
 	}
 
 	if (imp->imp_state == LUSTRE_IMP_RECOVER) {
-- 
2.7.4

Re: [PATCH] staging : lustre : Remove braces from single-line body.

[Joe Perches]

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

On Fri, 2016-12-16 at 21:53 +0300, Dan Carpenter wrote:
> On Fri, Dec 16, 2016 at 10:43:24AM -0800, Joe Perches wrote:
> > On Fri, 2016-12-16 at 23:29 +0530, Tabrez khan wrote:
> > > Remove unnecessary braces {} from single line if statement.
> > > This warning is found using checkpatch.pl.
> > 
> > []
> > > diff --git a/drivers/staging/lustre/lustre/ptlrpc/import.c b/drivers/staging/lustre/lustre/ptlrpc/import.c
> > 
> > []
> > > @@ -1134,9 +1134,9 @@ static int ptlrpc_connect_interpret(const struct lu_env *env,
> > >  	}
> > >  
> > >  	/* Sanity checks for a reconnected import. */
> > > -	if (!(imp->imp_replayable) != !(msg_flags & MSG_CONNECT_REPLAYABLE)) {
> > > +	if (!(imp->imp_replayable) != !(msg_flags & MSG_CONNECT_REPLAYABLE))
> > >  		CERROR("imp_replayable flag does not match server after reconnect. We should LBUG right here.\n");
> > > -	}
> > > +
> > 
> > There are one too many blank lines now.
> 
> I was expecting checkpatch.pl to catch the extra blank.  It was there in
> the last patch as well.  Apparently it doesn't.

checkpatch is imperfect and always will be.

checkpatch is a stupid little script.

It mostly works on added lines and generally
only looks for style defects on lines that
precede those added lines.

Here the now unnecessary blank line follows the
added blank line.

Oh well.

Anyway, here's an almost completely untested
possible patch to checkpatch.

(also attached because evolution is a terrible
 email client, but I seem to be stuck with it)

---
 scripts/checkpatch.pl | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 1579230ebacc..97078f7629e2 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3063,6 +3063,19 @@ sub process {
 			$last_blank_line = $linenr;
 		}
 
+# Another check for multiple consecutive blank lines where the following
+# line is part of the context and is also blank
+		if ($line =~ /^\+\s*$/ &&
+		    defined $rawlines[$linenr] &&
+		    $rawlines[$linenr] =~ /^ \s*$/) {
+			if (CHK("LINE_SPACING",
+				"Please don't use multiple blank lines\n" . $hereprev) &&
+			    $fix) {
+				fix_delete_line($fixlinenr, $rawline);
+			}
+			$last_blank_line = $linenr;
+		}
+
 # check for missing blank lines after declarations
 		if ($sline =~ /^\+\s+\S/ &&			#Not at char 1
 			# actual declarations

[-- Attachment #2: cp_trailing_context_blank.diff --]
[-- Type: text/x-patch, Size: 860 bytes --]

 scripts/checkpatch.pl | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 1579230ebacc..97078f7629e2 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3063,6 +3063,19 @@ sub process {
 			$last_blank_line = $linenr;
 		}
 
+# Another check for multiple consecutive blank lines where the following
+# line is part of the context and is also blank
+		if ($line =~ /^\+\s*$/ &&
+		    defined $rawlines[$linenr] &&
+		    $rawlines[$linenr] =~ /^ \s*$/) {
+			if (CHK("LINE_SPACING",
+				"Please don't use multiple blank lines\n" . $hereprev) &&
+			    $fix) {
+				fix_delete_line($fixlinenr, $rawline);
+			}
+			$last_blank_line = $linenr;
+		}
+
 # check for missing blank lines after declarations
 		if ($sline =~ /^\+\s+\S/ &&			#Not at char 1
 			# actual declarations

Re: [PATCH] staging : lustre : Remove braces from single-line body.

[Dan Carpenter]

On Fri, Dec 16, 2016 at 10:43:24AM -0800, Joe Perches wrote:
> On Fri, 2016-12-16 at 23:29 +0530, Tabrez khan wrote:
> > Remove unnecessary braces {} from single line if statement.
> > This warning is found using checkpatch.pl.
> []
> > diff --git a/drivers/staging/lustre/lustre/ptlrpc/import.c b/drivers/staging/lustre/lustre/ptlrpc/import.c
> []
> > @@ -1134,9 +1134,9 @@ static int ptlrpc_connect_interpret(const struct lu_env *env,
> >  	}
> >  
> >  	/* Sanity checks for a reconnected import. */
> > -	if (!(imp->imp_replayable) != !(msg_flags & MSG_CONNECT_REPLAYABLE)) {
> > +	if (!(imp->imp_replayable) != !(msg_flags & MSG_CONNECT_REPLAYABLE))
> >  		CERROR("imp_replayable flag does not match server after reconnect. We should LBUG right here.\n");
> > -	}
> > +
> 
> There are one too many blank lines now.

I was expecting checkpatch.pl to catch the extra blank.  It was there in
the last patch as well.  Apparently it doesn't.

regards,
dan carpenter

Re: [PATCH] staging : lustre : Remove braces from single-line body.

[Joe Perches]

On Fri, 2016-12-16 at 23:29 +0530, Tabrez khan wrote:
> Remove unnecessary braces {} from single line if statement.
> This warning is found using checkpatch.pl.
[]
> diff --git a/drivers/staging/lustre/lustre/ptlrpc/import.c b/drivers/staging/lustre/lustre/ptlrpc/import.c
[]
> @@ -1134,9 +1134,9 @@ static int ptlrpc_connect_interpret(const struct lu_env *env,
>  	}
>  
>  	/* Sanity checks for a reconnected import. */
> -	if (!(imp->imp_replayable) != !(msg_flags & MSG_CONNECT_REPLAYABLE)) {
> +	if (!(imp->imp_replayable) != !(msg_flags & MSG_CONNECT_REPLAYABLE))
>  		CERROR("imp_replayable flag does not match server after reconnect. We should LBUG right here.\n");
> -	}
> +

There are one too many blank lines now.

And that's an awful lot of !s.

Generically, it might make simpler reading code to
cast to bool instead of using !.

Most code uses !! to make sure whatever value is
either 0 or 1 without changing the logic/polarity.

[PATCH] staging : lustre : Remove braces from single-line body.

[Tabrez khan]

Remove unnecessary braces {} from single line if statement.
This warning is found using checkpatch.pl.

Signed-off-by: Tabrez khan <khan.tabrez21@gmail.com>
---
 drivers/staging/lustre/lustre/ptlrpc/import.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/lustre/lustre/ptlrpc/import.c b/drivers/staging/lustre/lustre/ptlrpc/import.c
index a23d0a0..477d832 100644
--- a/drivers/staging/lustre/lustre/ptlrpc/import.c
+++ b/drivers/staging/lustre/lustre/ptlrpc/import.c
@@ -1134,9 +1134,9 @@ static int ptlrpc_connect_interpret(const struct lu_env *env,
 	}
 
 	/* Sanity checks for a reconnected import. */
-	if (!(imp->imp_replayable) != !(msg_flags & MSG_CONNECT_REPLAYABLE)) {
+	if (!(imp->imp_replayable) != !(msg_flags & MSG_CONNECT_REPLAYABLE))
 		CERROR("imp_replayable flag does not match server after reconnect. We should LBUG right here.\n");
-	}
+
 
 	if (lustre_msg_get_last_committed(request->rq_repmsg) > 0 &&
 	    lustre_msg_get_last_committed(request->rq_repmsg) <
-- 
2.7.4