staging: lustre/lustre/lov: Cleanup style issues in lov_request.c

staging: lustre/lustre/lov: Cleanup style issues in lov_request.c

[Riccardo Lucchese]

Hello,

This patch series fixes two coding style issues and removes an unneeded
'if' statement in lov_request.c.

Thanks,
riccardo

[PATCH 1/3] staging: lustre/lustre/lov: Remove unneeded 'if' statement in lov_request.c/lov_check_set()

[Riccardo Lucchese]

It is silly to go through an if statement to set a single boolean
value in function of a single boolean expression. In the function
lov_check_set, assign the return value directly.

Signed-off-by: Riccardo Lucchese <riccardo.lucchese@gmail.com>
---
 drivers/staging/lustre/lustre/lov/lov_request.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/lustre/lustre/lov/lov_request.c b/drivers/staging/lustre/lustre/lov/lov_request.c
index ce830e4..90fc66a 100644
--- a/drivers/staging/lustre/lustre/lov/lov_request.c
+++ b/drivers/staging/lustre/lustre/lov/lov_request.c
@@ -140,14 +140,13 @@ void lov_set_add_req(struct lov_request *req, struct lov_request_set *set)
 
 static int lov_check_set(struct lov_obd *lov, int idx)
 {
-	int rc = 0;
+	int rc;
 	mutex_lock(&lov->lov_lock);
 
-	if (lov->lov_tgts[idx] == NULL ||
-	    lov->lov_tgts[idx]->ltd_active ||
-	    (lov->lov_tgts[idx]->ltd_exp != NULL &&
-	     class_exp2cliimp(lov->lov_tgts[idx]->ltd_exp)->imp_connect_tried))
-		rc = 1;
+	rc = lov->lov_tgts[idx] == NULL ||
+		lov->lov_tgts[idx]->ltd_active ||
+		(lov->lov_tgts[idx]->ltd_exp != NULL &&
+		 class_exp2cliimp(lov->lov_tgts[idx]->ltd_exp)->imp_connect_tried);
 
 	mutex_unlock(&lov->lov_lock);
 	return rc;
-- 
1.9.1

[PATCH 2/3] staging: lustre/lustre/lov: Add a blank line after declarations in lov_request.c

[Riccardo Lucchese]

Fix the following checkpatch.pl issue in lov_request.c:
WARNING: Missing a blank line after declarations

Signed-off-by: Riccardo Lucchese <riccardo.lucchese@gmail.com>
---
 drivers/staging/lustre/lustre/lov/lov_request.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/lustre/lustre/lov/lov_request.c b/drivers/staging/lustre/lustre/lov/lov_request.c
index 90fc66a..d9344c7 100644
--- a/drivers/staging/lustre/lustre/lov/lov_request.c
+++ b/drivers/staging/lustre/lustre/lov/lov_request.c
@@ -141,14 +141,14 @@ void lov_set_add_req(struct lov_request *req, struct lov_request_set *set)
 static int lov_check_set(struct lov_obd *lov, int idx)
 {
 	int rc;
-	mutex_lock(&lov->lov_lock);
 
+	mutex_lock(&lov->lov_lock);
 	rc = lov->lov_tgts[idx] == NULL ||
 		lov->lov_tgts[idx]->ltd_active ||
 		(lov->lov_tgts[idx]->ltd_exp != NULL &&
 		 class_exp2cliimp(lov->lov_tgts[idx]->ltd_exp)->imp_connect_tried);
-
 	mutex_unlock(&lov->lov_lock);
+
 	return rc;
 }
 
@@ -835,6 +835,7 @@ static int cb_getattr_update(void *cookie, int rc)
 {
 	struct obd_info *oinfo = cookie;
 	struct lov_request *lovreq;
+
 	lovreq = container_of(oinfo, struct lov_request, rq_oi);
 	return lov_update_common_set(lovreq->rq_rqset, lovreq, rc);
 }
@@ -1017,6 +1018,7 @@ static int cb_setattr_update(void *cookie, int rc)
 {
 	struct obd_info *oinfo = cookie;
 	struct lov_request *lovreq;
+
 	lovreq = container_of(oinfo, struct lov_request, rq_oi);
 	return lov_update_setattr_set(lovreq->rq_rqset, lovreq, rc);
 }
@@ -1140,6 +1142,7 @@ static int cb_update_punch(void *cookie, int rc)
 {
 	struct obd_info *oinfo = cookie;
 	struct lov_request *lovreq;
+
 	lovreq = container_of(oinfo, struct lov_request, rq_oi);
 	return lov_update_punch_set(lovreq->rq_rqset, lovreq, rc);
 }
-- 
1.9.1

[PATCH 3/3] staging: lustre/lustre/lov: Add a space before open braces '{' in lov_request.c

[Riccardo Lucchese]

Fix the following checkpatch.pl issue in lov_request.c:
ERROR: space required before the open brace '{'

Signed-off-by: Riccardo Lucchese <riccardo.lucchese@gmail.com>
---
 drivers/staging/lustre/lustre/lov/lov_request.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/lustre/lustre/lov/lov_request.c b/drivers/staging/lustre/lustre/lov/lov_request.c
index d9344c7..57ee94d 100644
--- a/drivers/staging/lustre/lustre/lov/lov_request.c
+++ b/drivers/staging/lustre/lustre/lov/lov_request.c
@@ -477,7 +477,7 @@ int lov_prep_match_set(struct obd_export *exp, struct obd_info *oinfo,
 		GOTO(out_set, rc = -ENOMEM);
 	lockh->cookie = set->set_lockh->llh_handle.h_cookie;
 
-	for (i = 0; i < lsm->lsm_stripe_count; i++){
+	for (i = 0; i < lsm->lsm_stripe_count; i++) {
 		struct lov_oinfo *loi;
 		struct lov_request *req;
 		obd_off start, end;
@@ -565,7 +565,7 @@ int lov_prep_cancel_set(struct obd_export *exp, struct obd_info *oinfo,
 	}
 	lockh->cookie = set->set_lockh->llh_handle.h_cookie;
 
-	for (i = 0; i < lsm->lsm_stripe_count; i++){
+	for (i = 0; i < lsm->lsm_stripe_count; i++) {
 		struct lov_request *req;
 		struct lustre_handle *lov_lockhp;
 		struct lov_oinfo *loi = lsm->lsm_oinfo[i];
@@ -733,7 +733,7 @@ int lov_prep_brw_set(struct obd_export *exp, struct obd_info *oinfo,
 
 	/* alloc and initialize lov request */
 	shift = 0;
-	for (i = 0; i < oinfo->oi_md->lsm_stripe_count; i++){
+	for (i = 0; i < oinfo->oi_md->lsm_stripe_count; i++) {
 		struct lov_oinfo *loi = NULL;
 		struct lov_request *req;
 
-- 
1.9.1

Re: [PATCH 1/3] staging: lustre/lustre/lov: Remove unneeded 'if' statement in lov_request.c/lov_check_set()

[Joe Perches]

On Sat, 2014-07-19 at 21:34 +0200, Riccardo Lucchese wrote:
> It is silly to go through an if statement to set a single boolean
> value in function of a single boolean expression. In the function
> lov_check_set, assign the return value directly.
[]
> diff --git a/drivers/staging/lustre/lustre/lov/lov_request.c b/drivers/staging/lustre/lustre/lov/lov_request.c
[]
> @@ -140,14 +140,13 @@ void lov_set_add_req(struct lov_request *req, struct lov_request_set *set)
>  
>  static int lov_check_set(struct lov_obd *lov, int idx)
>  {
> -	int rc = 0;
> +	int rc;
>  	mutex_lock(&lov->lov_lock);
>  
> -	if (lov->lov_tgts[idx] == NULL ||
> -	    lov->lov_tgts[idx]->ltd_active ||
> -	    (lov->lov_tgts[idx]->ltd_exp != NULL &&
> -	     class_exp2cliimp(lov->lov_tgts[idx]->ltd_exp)->imp_connect_tried))
> -		rc = 1;
> +	rc = lov->lov_tgts[idx] == NULL ||
> +		lov->lov_tgts[idx]->ltd_active ||
> +		(lov->lov_tgts[idx]->ltd_exp != NULL &&
> +		 class_exp2cliimp(lov->lov_tgts[idx]->ltd_exp)->imp_connect_tried);
>  
>  	mutex_unlock(&lov->lov_lock);
>  	return rc;

Maybe consider using a temporary for lov->lov_tgtx[idx] like:

---

 drivers/staging/lustre/lustre/lov/lov_request.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/lustre/lustre/lov/lov_request.c
b/drivers/staging/lustre/lustre/lov/lov_request.c
index ce830e4..53a4575 100644
--- a/drivers/staging/lustre/lustre/lov/lov_request.c
+++ b/drivers/staging/lustre/lustre/lov/lov_request.c
@@ -140,16 +140,18 @@ void lov_set_add_req(struct lov_request *req,
struct lov_request_set *set)
 
 static int lov_check_set(struct lov_obd *lov, int idx)
 {
-	int rc = 0;
+	int rc;
+	struct lmv_tgt_desc *tgt;
+
 	mutex_lock(&lov->lov_lock);
 
-	if (lov->lov_tgts[idx] == NULL ||
-	    lov->lov_tgts[idx]->ltd_active ||
-	    (lov->lov_tgts[idx]->ltd_exp != NULL &&
-	     class_exp2cliimp(lov->lov_tgts[idx]->ltd_exp)->imp_connect_tried))
-		rc = 1;
+	tgt = lov->lov_tgts[idx];
+	rc = (!tgt || tgt->ltd_active ||
+	      (tgt->ltd_exp &&
+	       class_exp2cliimp(tgt->ltd_exp)->imp_connect_tried));
 
 	mutex_unlock(&lov->lov_lock);
+
 	return rc;
 }
 

Re: [PATCH 1/3] staging: lustre/lustre/lov: Remove unneeded 'if' statement in lov_request.c/lov_check_set()

[Dan Carpenter]

On Sat, Jul 19, 2014 at 09:34:56PM +0200, Riccardo Lucchese wrote:
> It is silly to go through an if statement to set a single boolean
> value in function of a single boolean expression. In the function
> lov_check_set, assign the return value directly.
> 
> Signed-off-by: Riccardo Lucchese <riccardo.lucchese@gmail.com>
> ---
>  drivers/staging/lustre/lustre/lov/lov_request.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/lov/lov_request.c b/drivers/staging/lustre/lustre/lov/lov_request.c
> index ce830e4..90fc66a 100644
> --- a/drivers/staging/lustre/lustre/lov/lov_request.c
> +++ b/drivers/staging/lustre/lustre/lov/lov_request.c
> @@ -140,14 +140,13 @@ void lov_set_add_req(struct lov_request *req, struct lov_request_set *set)
>  
>  static int lov_check_set(struct lov_obd *lov, int idx)
>  {
> -	int rc = 0;
> +	int rc;
>  	mutex_lock(&lov->lov_lock);
>  
> -	if (lov->lov_tgts[idx] == NULL ||
> -	    lov->lov_tgts[idx]->ltd_active ||
> -	    (lov->lov_tgts[idx]->ltd_exp != NULL &&
> -	     class_exp2cliimp(lov->lov_tgts[idx]->ltd_exp)->imp_connect_tried))
> -		rc = 1;
> +	rc = lov->lov_tgts[idx] == NULL ||
> +		lov->lov_tgts[idx]->ltd_active ||
> +		(lov->lov_tgts[idx]->ltd_exp != NULL &&
> +		 class_exp2cliimp(lov->lov_tgts[idx]->ltd_exp)->imp_connect_tried);

I don't see how this makes the code more readable at all.

regards,
dan carpenter

Re: [PATCH 1/3] staging: lustre/lustre/lov: Remove unneeded 'if' statement in lov_request.c/lov_check_set()

[Riccardo Lucchese]

On Sat, Jul 19, 2014 at 12:59:22PM -0700, Joe Perches wrote:
> On Sat, 2014-07-19 at 21:34 +0200, Riccardo Lucchese wrote:
> > It is silly to go through an if statement to set a single boolean
> > value in function of a single boolean expression. In the function
> > lov_check_set, assign the return value directly.
> []
> > diff --git a/drivers/staging/lustre/lustre/lov/lov_request.c b/drivers/staging/lustre/lustre/lov/lov_request.c
> []
> > @@ -140,14 +140,13 @@ void lov_set_add_req(struct lov_request *req, struct lov_request_set *set)
> >  
> >  static int lov_check_set(struct lov_obd *lov, int idx)
> >  {
> > -	int rc = 0;
> > +	int rc;
> >  	mutex_lock(&lov->lov_lock);
> >  
> > -	if (lov->lov_tgts[idx] == NULL ||
> > -	    lov->lov_tgts[idx]->ltd_active ||
> > -	    (lov->lov_tgts[idx]->ltd_exp != NULL &&
> > -	     class_exp2cliimp(lov->lov_tgts[idx]->ltd_exp)->imp_connect_tried))
> > -		rc = 1;
> > +	rc = lov->lov_tgts[idx] == NULL ||
> > +		lov->lov_tgts[idx]->ltd_active ||
> > +		(lov->lov_tgts[idx]->ltd_exp != NULL &&
> > +		 class_exp2cliimp(lov->lov_tgts[idx]->ltd_exp)->imp_connect_tried);
> >  
> >  	mutex_unlock(&lov->lov_lock);
> >  	return rc;
> 
> Maybe consider using a temporary for lov->lov_tgtx[idx] like:
[...]

Indeed, it is nicer this way.

Thanks,
Riccardo

Re: [PATCH 1/3] staging: lustre/lustre/lov: Remove unneeded 'if' statement in lov_request.c/lov_check_set()

[Riccardo Lucchese]

Dan,

On Sun, Jul 20, 2014 at 07:52:53AM +0300, Dan Carpenter wrote:
> On Sat, Jul 19, 2014 at 09:34:56PM +0200, Riccardo Lucchese wrote:
> > It is silly to go through an if statement to set a single boolean
> > value in function of a single boolean expression. In the function
> > lov_check_set, assign the return value directly.
> > 
> > Signed-off-by: Riccardo Lucchese <riccardo.lucchese@gmail.com>
> > ---
> >  drivers/staging/lustre/lustre/lov/lov_request.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/staging/lustre/lustre/lov/lov_request.c b/drivers/staging/lustre/lustre/lov/lov_request.c
> > index ce830e4..90fc66a 100644
> > --- a/drivers/staging/lustre/lustre/lov/lov_request.c
> > +++ b/drivers/staging/lustre/lustre/lov/lov_request.c
> > @@ -140,14 +140,13 @@ void lov_set_add_req(struct lov_request *req, struct lov_request_set *set)
> >  
> >  static int lov_check_set(struct lov_obd *lov, int idx)
> >  {
> > -	int rc = 0;
> > +	int rc;
> >  	mutex_lock(&lov->lov_lock);
> >  
> > -	if (lov->lov_tgts[idx] == NULL ||
> > -	    lov->lov_tgts[idx]->ltd_active ||
> > -	    (lov->lov_tgts[idx]->ltd_exp != NULL &&
> > -	     class_exp2cliimp(lov->lov_tgts[idx]->ltd_exp)->imp_connect_tried))
> > -		rc = 1;
> > +	rc = lov->lov_tgts[idx] == NULL ||
> > +		lov->lov_tgts[idx]->ltd_active ||
> > +		(lov->lov_tgts[idx]->ltd_exp != NULL &&
> > +		 class_exp2cliimp(lov->lov_tgts[idx]->ltd_exp)->imp_connect_tried);
> 
> I don't see how this makes the code more readable at all.

Thank you for the comment. Would you consider something like the
following diff instead ? Otherwise, I will resend the series for
review without this change.

riccardo

---

diff --git a/drivers/staging/lustre/lustre/lov/lov_request.c b/drivers/staging/lustre/lustre/lov/lov_request.c
index ce830e4..ae670bb 100644
--- a/drivers/staging/lustre/lustre/lov/lov_request.c
+++ b/drivers/staging/lustre/lustre/lov/lov_request.c
@@ -140,14 +140,14 @@ void lov_set_add_req(struct lov_request *req, struct lov_request_set *set)
 
 static int lov_check_set(struct lov_obd *lov, int idx)
 {
-	int rc = 0;
+	int rc;
+	struct lov_tgt_desc *desc;
 	mutex_lock(&lov->lov_lock);
 
-	if (lov->lov_tgts[idx] == NULL ||
-	    lov->lov_tgts[idx]->ltd_active ||
-	    (lov->lov_tgts[idx]->ltd_exp != NULL &&
-	     class_exp2cliimp(lov->lov_tgts[idx]->ltd_exp)->imp_connect_tried))
-		rc = 1;
+	desc = lov->lov_tgts[idx];
+	rc = !desc || desc->ltd_active ||
+		(desc->ltd_exp &&
+		 class_exp2cliimp(desc->ltd_exp)->imp_connect_tried);
 
 	mutex_unlock(&lov->lov_lock);
 	return rc;
-- 
1.9.1

Re: [PATCH 1/3] staging: lustre/lustre/lov: Remove unneeded 'if' statement in lov_request.c/lov_check_set()

[Dan Carpenter]

On Sun, Jul 20, 2014 at 01:08:36PM +0200, Riccardo Lucchese wrote:
> Dan,
> 
> On Sun, Jul 20, 2014 at 07:52:53AM +0300, Dan Carpenter wrote:
> > On Sat, Jul 19, 2014 at 09:34:56PM +0200, Riccardo Lucchese wrote:
> > > It is silly to go through an if statement to set a single boolean
> > > value in function of a single boolean expression. In the function
> > > lov_check_set, assign the return value directly.
> > > 
> > > Signed-off-by: Riccardo Lucchese <riccardo.lucchese@gmail.com>
> > > ---
> > >  drivers/staging/lustre/lustre/lov/lov_request.c | 11 +++++------
> > >  1 file changed, 5 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/staging/lustre/lustre/lov/lov_request.c b/drivers/staging/lustre/lustre/lov/lov_request.c
> > > index ce830e4..90fc66a 100644
> > > --- a/drivers/staging/lustre/lustre/lov/lov_request.c
> > > +++ b/drivers/staging/lustre/lustre/lov/lov_request.c
> > > @@ -140,14 +140,13 @@ void lov_set_add_req(struct lov_request *req, struct lov_request_set *set)
> > >  
> > >  static int lov_check_set(struct lov_obd *lov, int idx)
> > >  {
> > > -	int rc = 0;
> > > +	int rc;
> > >  	mutex_lock(&lov->lov_lock);
> > >  
> > > -	if (lov->lov_tgts[idx] == NULL ||
> > > -	    lov->lov_tgts[idx]->ltd_active ||
> > > -	    (lov->lov_tgts[idx]->ltd_exp != NULL &&
> > > -	     class_exp2cliimp(lov->lov_tgts[idx]->ltd_exp)->imp_connect_tried))
> > > -		rc = 1;
> > > +	rc = lov->lov_tgts[idx] == NULL ||
> > > +		lov->lov_tgts[idx]->ltd_active ||
> > > +		(lov->lov_tgts[idx]->ltd_exp != NULL &&
> > > +		 class_exp2cliimp(lov->lov_tgts[idx]->ltd_exp)->imp_connect_tried);
> > 
> > I don't see how this makes the code more readable at all.
> 
> Thank you for the comment. Would you consider something like the
> following diff instead ? Otherwise, I will resend the series for
> review without this change.
> 
> riccardo
> 
> ---
> 
> diff --git a/drivers/staging/lustre/lustre/lov/lov_request.c b/drivers/staging/lustre/lustre/lov/lov_request.c
> index ce830e4..ae670bb 100644
> --- a/drivers/staging/lustre/lustre/lov/lov_request.c
> +++ b/drivers/staging/lustre/lustre/lov/lov_request.c
> @@ -140,14 +140,14 @@ void lov_set_add_req(struct lov_request *req, struct lov_request_set *set)
>  
>  static int lov_check_set(struct lov_obd *lov, int idx)
>  {
> -	int rc = 0;
> +	int rc;
> +	struct lov_tgt_desc *desc;
>  	mutex_lock(&lov->lov_lock);
>  
> -	if (lov->lov_tgts[idx] == NULL ||
> -	    lov->lov_tgts[idx]->ltd_active ||
> -	    (lov->lov_tgts[idx]->ltd_exp != NULL &&
> -	     class_exp2cliimp(lov->lov_tgts[idx]->ltd_exp)->imp_connect_tried))
> -		rc = 1;
> +	desc = lov->lov_tgts[idx];
> +	rc = !desc || desc->ltd_active ||
> +		(desc->ltd_exp &&
> +		 class_exp2cliimp(desc->ltd_exp)->imp_connect_tried);

Sure, I suppose.  Using "desc" is a clean up.  Otherwise the original
code was not "silly".  It was fine.

I'm curious why you think if statements are less readable than other
statements.  That seems like nonsense.

regards,
dan carpenter

Re: [PATCH 1/3] staging: lustre/lustre/lov: Remove unneeded 'if' statement in lov_request.c/lov_check_set()

[Riccardo Lucchese]

On Sun, Jul 20, 2014 at 02:37:47PM +0300, Dan Carpenter wrote:
> On Sun, Jul 20, 2014 at 01:08:36PM +0200, Riccardo Lucchese wrote:
> > Dan,
> > 
> > On Sun, Jul 20, 2014 at 07:52:53AM +0300, Dan Carpenter wrote:
> > > On Sat, Jul 19, 2014 at 09:34:56PM +0200, Riccardo Lucchese wrote:
[...]
> > > I don't see how this makes the code more readable at all.
> > 
> > Thank you for the comment. Would you consider something like the
> > following diff instead ? Otherwise, I will resend the series for
> > review without this change.
> > 
> > riccardo
> > 
> > ---
> > 
> > diff --git a/drivers/staging/lustre/lustre/lov/lov_request.c b/drivers/staging/lustre/lustre/lov/lov_request.c
> > index ce830e4..ae670bb 100644
> > --- a/drivers/staging/lustre/lustre/lov/lov_request.c
> > +++ b/drivers/staging/lustre/lustre/lov/lov_request.c
> > @@ -140,14 +140,14 @@ void lov_set_add_req(struct lov_request *req, struct lov_request_set *set)
> >  
> >  static int lov_check_set(struct lov_obd *lov, int idx)
> >  {
> > -	int rc = 0;
> > +	int rc;
> > +	struct lov_tgt_desc *desc;
> >  	mutex_lock(&lov->lov_lock);
> >  
> > -	if (lov->lov_tgts[idx] == NULL ||
> > -	    lov->lov_tgts[idx]->ltd_active ||
> > -	    (lov->lov_tgts[idx]->ltd_exp != NULL &&
> > -	     class_exp2cliimp(lov->lov_tgts[idx]->ltd_exp)->imp_connect_tried))
> > -		rc = 1;
> > +	desc = lov->lov_tgts[idx];
> > +	rc = !desc || desc->ltd_active ||
> > +		(desc->ltd_exp &&
> > +		 class_exp2cliimp(desc->ltd_exp)->imp_connect_tried);
> 
> Sure, I suppose.  Using "desc" is a clean up.  Otherwise the original
> code was not "silly".  It was fine.

The adjective "silly" was inappropriate and misleading, sorry about
that.

> I'm curious why you think if statements are less readable than other
> statements.  That seems like nonsense.

Not in general but, in this case, I find the patched code clearer.

Thanks,
riccardo

staging: lustre: lov: Cleanup style issues in lov_request.c

[Riccardo Lucchese]

Hello,

This patch series contains one cleanup and two coding style fixes for lov_request.c.

This iteration addresses the review comments received by v1.

Changes in v2:
 - Improved the commit messages.
 - Introduced a local variable in lov_check_set() to make the code clearer.

Thanks,
riccardo

[PATCH v2 1/3] staging: lustre: lov: Cleanup lov_check_set() in lov_request.c

[Riccardo Lucchese]

Make the code clearer by introducing a local variable and removing the
unnecessary 'if' statement.

Signed-off-by: Riccardo Lucchese <riccardo.lucchese@gmail.com>
Acked-by: Julia Lawall <julia.lawall@lip6.fr>
---
Changes in v2:
 - Improved the commit message:
    - changed the subject line to follow the same convention used by
      previous commits to the same file.
    - rewrote the message body using a more formal language.
 - Introduced a local variable to make the code clearer.
 - Added Acked-by - Julia.

 drivers/staging/lustre/lustre/lov/lov_request.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/lustre/lustre/lov/lov_request.c b/drivers/staging/lustre/lustre/lov/lov_request.c
index ce830e4..ae670bb 100644
--- a/drivers/staging/lustre/lustre/lov/lov_request.c
+++ b/drivers/staging/lustre/lustre/lov/lov_request.c
@@ -140,14 +140,14 @@ void lov_set_add_req(struct lov_request *req, struct lov_request_set *set)
 
 static int lov_check_set(struct lov_obd *lov, int idx)
 {
-	int rc = 0;
+	int rc;
+	struct lov_tgt_desc *desc;
 	mutex_lock(&lov->lov_lock);
 
-	if (lov->lov_tgts[idx] == NULL ||
-	    lov->lov_tgts[idx]->ltd_active ||
-	    (lov->lov_tgts[idx]->ltd_exp != NULL &&
-	     class_exp2cliimp(lov->lov_tgts[idx]->ltd_exp)->imp_connect_tried))
-		rc = 1;
+	desc = lov->lov_tgts[idx];
+	rc = !desc || desc->ltd_active ||
+		(desc->ltd_exp &&
+		 class_exp2cliimp(desc->ltd_exp)->imp_connect_tried);
 
 	mutex_unlock(&lov->lov_lock);
 	return rc;
-- 
1.9.1

[PATCH v2 2/3] staging: lustre: lov: Add a blank line after declarations in lov_request.c

[Riccardo Lucchese]

Fix the following checkpatch.pl issue in lov_request.c:
WARNING: Missing a blank line after declarations

Signed-off-by: Riccardo Lucchese <riccardo.lucchese@gmail.com>
Acked-by: Julia Lawall <julia.lawall@lip6.fr>
---
Changes in v2:
 - Changed the subject line to follow the same convention used by
   previous commits to the same file.
 - Added Acked-by - Julia.

 drivers/staging/lustre/lustre/lov/lov_request.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/lustre/lustre/lov/lov_request.c b/drivers/staging/lustre/lustre/lov/lov_request.c
index ae670bb..7491f26 100644
--- a/drivers/staging/lustre/lustre/lov/lov_request.c
+++ b/drivers/staging/lustre/lustre/lov/lov_request.c
@@ -142,14 +142,14 @@ static int lov_check_set(struct lov_obd *lov, int idx)
 {
 	int rc;
 	struct lov_tgt_desc *desc;
-	mutex_lock(&lov->lov_lock);
 
+	mutex_lock(&lov->lov_lock);
 	desc = lov->lov_tgts[idx];
 	rc = !desc || desc->ltd_active ||
 		(desc->ltd_exp &&
 		 class_exp2cliimp(desc->ltd_exp)->imp_connect_tried);
-
 	mutex_unlock(&lov->lov_lock);
+
 	return rc;
 }
 
@@ -836,6 +836,7 @@ static int cb_getattr_update(void *cookie, int rc)
 {
 	struct obd_info *oinfo = cookie;
 	struct lov_request *lovreq;
+
 	lovreq = container_of(oinfo, struct lov_request, rq_oi);
 	return lov_update_common_set(lovreq->rq_rqset, lovreq, rc);
 }
@@ -1018,6 +1019,7 @@ static int cb_setattr_update(void *cookie, int rc)
 {
 	struct obd_info *oinfo = cookie;
 	struct lov_request *lovreq;
+
 	lovreq = container_of(oinfo, struct lov_request, rq_oi);
 	return lov_update_setattr_set(lovreq->rq_rqset, lovreq, rc);
 }
@@ -1141,6 +1143,7 @@ static int cb_update_punch(void *cookie, int rc)
 {
 	struct obd_info *oinfo = cookie;
 	struct lov_request *lovreq;
+
 	lovreq = container_of(oinfo, struct lov_request, rq_oi);
 	return lov_update_punch_set(lovreq->rq_rqset, lovreq, rc);
 }
-- 
1.9.1

[PATCH v2 3/3] staging: lustre: lov: Add a space before open braces '{' in lov_request.c

[Riccardo Lucchese]

Fix the following checkpatch.pl issue in lov_request.c:
ERROR: space required before the open brace '{'

Signed-off-by: Riccardo Lucchese <riccardo.lucchese@gmail.com>
Acked-by: Julia Lawall <julia.lawall@lip6.fr>
---
Changes in v2:
 - Changed the subject line to follow the same convention used by
   previous commits to the same file.
 - Added Acked-by - Julia.

 drivers/staging/lustre/lustre/lov/lov_request.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/lustre/lustre/lov/lov_request.c b/drivers/staging/lustre/lustre/lov/lov_request.c
index 7491f26..caa4a01 100644
--- a/drivers/staging/lustre/lustre/lov/lov_request.c
+++ b/drivers/staging/lustre/lustre/lov/lov_request.c
@@ -478,7 +478,7 @@ int lov_prep_match_set(struct obd_export *exp, struct obd_info *oinfo,
 		GOTO(out_set, rc = -ENOMEM);
 	lockh->cookie = set->set_lockh->llh_handle.h_cookie;
 
-	for (i = 0; i < lsm->lsm_stripe_count; i++){
+	for (i = 0; i < lsm->lsm_stripe_count; i++) {
 		struct lov_oinfo *loi;
 		struct lov_request *req;
 		obd_off start, end;
@@ -566,7 +566,7 @@ int lov_prep_cancel_set(struct obd_export *exp, struct obd_info *oinfo,
 	}
 	lockh->cookie = set->set_lockh->llh_handle.h_cookie;
 
-	for (i = 0; i < lsm->lsm_stripe_count; i++){
+	for (i = 0; i < lsm->lsm_stripe_count; i++) {
 		struct lov_request *req;
 		struct lustre_handle *lov_lockhp;
 		struct lov_oinfo *loi = lsm->lsm_oinfo[i];
@@ -734,7 +734,7 @@ int lov_prep_brw_set(struct obd_export *exp, struct obd_info *oinfo,
 
 	/* alloc and initialize lov request */
 	shift = 0;
-	for (i = 0; i < oinfo->oi_md->lsm_stripe_count; i++){
+	for (i = 0; i < oinfo->oi_md->lsm_stripe_count; i++) {
 		struct lov_oinfo *loi = NULL;
 		struct lov_request *req;
 
-- 
1.9.1

Re: [PATCH v2 1/3] staging: lustre: lov: Cleanup lov_check_set() in lov_request.c

[Joe Perches]

On Sun, 2014-07-20 at 15:22 +0200, Riccardo Lucchese wrote:
> Make the code clearer by introducing a local variable and removing the
> unnecessary 'if' statement.
> 
> Signed-off-by: Riccardo Lucchese <riccardo.lucchese@gmail.com>
> Acked-by: Julia Lawall <julia.lawall@lip6.fr>
> ---
> Changes in v2:
>  - Improved the commit message:
>     - changed the subject line to follow the same convention used by
>       previous commits to the same file.
>     - rewrote the message body using a more formal language.
>  - Introduced a local variable to make the code clearer.
>  - Added Acked-by - Julia.
> 
>  drivers/staging/lustre/lustre/lov/lov_request.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/lov/lov_request.c b/drivers/staging/lustre/lustre/lov/lov_request.c
> index ce830e4..ae670bb 100644
> --- a/drivers/staging/lustre/lustre/lov/lov_request.c
> +++ b/drivers/staging/lustre/lustre/lov/lov_request.c
> @@ -140,14 +140,14 @@ void lov_set_add_req(struct lov_request *req, struct lov_request_set *set)
>  
>  static int lov_check_set(struct lov_obd *lov, int idx)
>  {
> -	int rc = 0;
> +	int rc;
> +	struct lov_tgt_desc *desc;
>  	mutex_lock(&lov->lov_lock);
>  
> -	if (lov->lov_tgts[idx] == NULL ||
> -	    lov->lov_tgts[idx]->ltd_active ||
> -	    (lov->lov_tgts[idx]->ltd_exp != NULL &&
> -	     class_exp2cliimp(lov->lov_tgts[idx]->ltd_exp)->imp_connect_tried))
> -		rc = 1;
> +	desc = lov->lov_tgts[idx];
> +	rc = !desc || desc->ltd_active ||
> +		(desc->ltd_exp &&
> +		 class_exp2cliimp(desc->ltd_exp)->imp_connect_tried);
>  
>  	mutex_unlock(&lov->lov_lock);
>  	return rc;

desc is not a good temporary name here.

For consistency with the rest of the code
please use tgt

$ git grep "= lov->lov_tgts.*];"
drivers/staging/lustre/lustre/lov/lov_obd.c:                    tgt = lov->lov_tgts[i];
drivers/staging/lustre/lustre/lov/lov_obd.c:            tgt = lov->lov_tgts[i];
drivers/staging/lustre/lustre/lov/lov_obd.c:            tgt = lov->lov_tgts[index];
drivers/staging/lustre/lustre/lov/lov_obd.c:            tgt = lov->lov_tgts[index];
drivers/staging/lustre/lustre/lov/lov_obd.c:                    tgt = lov->lov_tgts[qctl->qc_idx];
drivers/staging/lustre/lustre/lov/lov_obd.c:                            tgt = lov->lov_tgts[i];
drivers/staging/lustre/lustre/lov/lov_obd.c:            tgt = lov->lov_tgts[info->idx];
drivers/staging/lustre/lustre/lov/lov_obd.c:            tgt = lov->lov_tgts[ost_idx];
drivers/staging/lustre/lustre/lov/lov_obd.c:                    tgt = lov->lov_tgts[((struct obd_id_info *)val)->idx];
drivers/staging/lustre/lustre/lov/lov_obd.c:                    tgt = lov->lov_tgts[i];
drivers/staging/lustre/lustre/lov/lov_obd.c:            tgt = lov->lov_tgts[i];
drivers/staging/lustre/lustre/lov/lov_request.c:        tgt = lov->lov_tgts[ost_idx];
drivers/staging/lustre/lustre/lov/lov_request.c:        tgt = lov->lov_tgts[lovreq->rq_idx];

Re: [PATCH v2 1/3] staging: lustre: lov: Cleanup lov_check_set() in lov_request.c

[Riccardo Lucchese]

On Sun, Jul 20, 2014 at 08:30:23AM -0700, Joe Perches wrote:
> On Sun, 2014-07-20 at 15:22 +0200, Riccardo Lucchese wrote:
> > Make the code clearer by introducing a local variable and removing the
> > unnecessary 'if' statement.
> > 
> > Signed-off-by: Riccardo Lucchese <riccardo.lucchese@gmail.com>
> > Acked-by: Julia Lawall <julia.lawall@lip6.fr>
> > ---
> > Changes in v2:
> >  - Improved the commit message:
> >     - changed the subject line to follow the same convention used by
> >       previous commits to the same file.
> >     - rewrote the message body using a more formal language.
> >  - Introduced a local variable to make the code clearer.
> >  - Added Acked-by - Julia.
> > 
> >  drivers/staging/lustre/lustre/lov/lov_request.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/staging/lustre/lustre/lov/lov_request.c b/drivers/staging/lustre/lustre/lov/lov_request.c
> > index ce830e4..ae670bb 100644
> > --- a/drivers/staging/lustre/lustre/lov/lov_request.c
> > +++ b/drivers/staging/lustre/lustre/lov/lov_request.c
> > @@ -140,14 +140,14 @@ void lov_set_add_req(struct lov_request *req, struct lov_request_set *set)
> >  
> >  static int lov_check_set(struct lov_obd *lov, int idx)
> >  {
> > -	int rc = 0;
> > +	int rc;
> > +	struct lov_tgt_desc *desc;
> >  	mutex_lock(&lov->lov_lock);
> >  
> > -	if (lov->lov_tgts[idx] == NULL ||
> > -	    lov->lov_tgts[idx]->ltd_active ||
> > -	    (lov->lov_tgts[idx]->ltd_exp != NULL &&
> > -	     class_exp2cliimp(lov->lov_tgts[idx]->ltd_exp)->imp_connect_tried))
> > -		rc = 1;
> > +	desc = lov->lov_tgts[idx];
> > +	rc = !desc || desc->ltd_active ||
> > +		(desc->ltd_exp &&
> > +		 class_exp2cliimp(desc->ltd_exp)->imp_connect_tried);
> >  
> >  	mutex_unlock(&lov->lov_lock);
> >  	return rc;
> 
> desc is not a good temporary name here.
> 
> For consistency with the rest of the code
> please use tgt
> 
> $ git grep "= lov->lov_tgts.*];"
> drivers/staging/lustre/lustre/lov/lov_obd.c:                    tgt = lov->lov_tgts[i];
> drivers/staging/lustre/lustre/lov/lov_obd.c:            tgt = lov->lov_tgts[i];
> drivers/staging/lustre/lustre/lov/lov_obd.c:            tgt = lov->lov_tgts[index];
> drivers/staging/lustre/lustre/lov/lov_obd.c:            tgt = lov->lov_tgts[index];
> drivers/staging/lustre/lustre/lov/lov_obd.c:                    tgt = lov->lov_tgts[qctl->qc_idx];
> drivers/staging/lustre/lustre/lov/lov_obd.c:                            tgt = lov->lov_tgts[i];
> drivers/staging/lustre/lustre/lov/lov_obd.c:            tgt = lov->lov_tgts[info->idx];
> drivers/staging/lustre/lustre/lov/lov_obd.c:            tgt = lov->lov_tgts[ost_idx];
> drivers/staging/lustre/lustre/lov/lov_obd.c:                    tgt = lov->lov_tgts[((struct obd_id_info *)val)->idx];
> drivers/staging/lustre/lustre/lov/lov_obd.c:                    tgt = lov->lov_tgts[i];
> drivers/staging/lustre/lustre/lov/lov_obd.c:            tgt = lov->lov_tgts[i];
> drivers/staging/lustre/lustre/lov/lov_request.c:        tgt = lov->lov_tgts[ost_idx];
> drivers/staging/lustre/lustre/lov/lov_request.c:        tgt = lov->lov_tgts[lovreq->rq_idx];
> 

Thank you for noticing this. I will send a v3.

riccardo

staging: lustre: lov: Cleanup style issues in lov_request.c

[Riccardo Lucchese]

Hello,

This patch series contains one cleanup and two coding style fixes for
lov_request.c.

This iteration addresses the review comments received by v2.

Changes in v3:
 - Renamed the local variable introduced in v2-1/3 to be consistent
   with other definitions in the driver.

Changes in v2:
 - Improved the commit messages.
 - Introduced a local variable in lov_check_set() to make the code
   clearer.

Thanks,
riccardo

[PATCH v3 1/3] staging: lustre: lov: Cleanup lov_check_set() in lov_request.c

[Riccardo Lucchese]

Make the code clearer by introducing a local variable and removing the
unnecessary 'if' statement.

Signed-off-by: Riccardo Lucchese <riccardo.lucchese@gmail.com>
Acked-by: Julia Lawall <julia.lawall@lip6.fr>
---
Changes in v3:
 - Renamed the local variable 'desc' to 'tgt' to be consistent with
   other definitions in the driver.

Changes in v2:
 - Improved the commit message:
    - changed the subject line to follow the same convention used by
      previous commits to the same file.
    - rewrote the message body using a more formal language.
 - Introduced a local variable to make the code clearer.
 - Added Acked-by - Julia.

 drivers/staging/lustre/lustre/lov/lov_request.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/lustre/lustre/lov/lov_request.c b/drivers/staging/lustre/lustre/lov/lov_request.c
index ce830e4..da424de 100644
--- a/drivers/staging/lustre/lustre/lov/lov_request.c
+++ b/drivers/staging/lustre/lustre/lov/lov_request.c
@@ -140,14 +140,14 @@ void lov_set_add_req(struct lov_request *req, struct lov_request_set *set)
 
 static int lov_check_set(struct lov_obd *lov, int idx)
 {
-	int rc = 0;
+	int rc;
+	struct lov_tgt_desc *tgt;
 	mutex_lock(&lov->lov_lock);
 
-	if (lov->lov_tgts[idx] == NULL ||
-	    lov->lov_tgts[idx]->ltd_active ||
-	    (lov->lov_tgts[idx]->ltd_exp != NULL &&
-	     class_exp2cliimp(lov->lov_tgts[idx]->ltd_exp)->imp_connect_tried))
-		rc = 1;
+	tgt = lov->lov_tgts[idx];
+	rc = !tgt || tgt->ltd_active ||
+		(tgt->ltd_exp &&
+		 class_exp2cliimp(tgt->ltd_exp)->imp_connect_tried);
 
 	mutex_unlock(&lov->lov_lock);
 	return rc;
-- 
1.9.1

[PATCH v3 2/3] staging: lustre: lov: Add a blank line after declarations in lov_request.c

[Riccardo Lucchese]

Fix the following checkpatch.pl issue in lov_request.c:
WARNING: Missing a blank line after declarations

Signed-off-by: Riccardo Lucchese <riccardo.lucchese@gmail.com>
Acked-by: Julia Lawall <julia.lawall@lip6.fr>
---
No changes in v3.

Changes in v2:
 - Changed the subject line to follow the same convention used by
   previous commits to the same file.
 - Added Acked-by - Julia.

 drivers/staging/lustre/lustre/lov/lov_request.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/lustre/lustre/lov/lov_request.c b/drivers/staging/lustre/lustre/lov/lov_request.c
index da424de..887b4b0 100644
--- a/drivers/staging/lustre/lustre/lov/lov_request.c
+++ b/drivers/staging/lustre/lustre/lov/lov_request.c
@@ -142,14 +142,14 @@ static int lov_check_set(struct lov_obd *lov, int idx)
 {
 	int rc;
 	struct lov_tgt_desc *tgt;
-	mutex_lock(&lov->lov_lock);
 
+	mutex_lock(&lov->lov_lock);
 	tgt = lov->lov_tgts[idx];
 	rc = !tgt || tgt->ltd_active ||
 		(tgt->ltd_exp &&
 		 class_exp2cliimp(tgt->ltd_exp)->imp_connect_tried);
-
 	mutex_unlock(&lov->lov_lock);
+
 	return rc;
 }
 
@@ -836,6 +836,7 @@ static int cb_getattr_update(void *cookie, int rc)
 {
 	struct obd_info *oinfo = cookie;
 	struct lov_request *lovreq;
+
 	lovreq = container_of(oinfo, struct lov_request, rq_oi);
 	return lov_update_common_set(lovreq->rq_rqset, lovreq, rc);
 }
@@ -1018,6 +1019,7 @@ static int cb_setattr_update(void *cookie, int rc)
 {
 	struct obd_info *oinfo = cookie;
 	struct lov_request *lovreq;
+
 	lovreq = container_of(oinfo, struct lov_request, rq_oi);
 	return lov_update_setattr_set(lovreq->rq_rqset, lovreq, rc);
 }
@@ -1141,6 +1143,7 @@ static int cb_update_punch(void *cookie, int rc)
 {
 	struct obd_info *oinfo = cookie;
 	struct lov_request *lovreq;
+
 	lovreq = container_of(oinfo, struct lov_request, rq_oi);
 	return lov_update_punch_set(lovreq->rq_rqset, lovreq, rc);
 }
-- 
1.9.1

[PATCH v3 3/3] staging: lustre: lov: Add a space before open braces '{' in lov_request.c

[Riccardo Lucchese]

Fix the following checkpatch.pl issue in lov_request.c:
ERROR: space required before the open brace '{'

Signed-off-by: Riccardo Lucchese <riccardo.lucchese@gmail.com>
Acked-by: Julia Lawall <julia.lawall@lip6.fr>
---
No changes in v3.

Changes in v2:
 - Changed the subject line to follow the same convention used by
   previous commits to the same file.
 - Added Acked-by - Julia.

 drivers/staging/lustre/lustre/lov/lov_request.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/lustre/lustre/lov/lov_request.c b/drivers/staging/lustre/lustre/lov/lov_request.c
index 887b4b0..e4bb02a 100644
--- a/drivers/staging/lustre/lustre/lov/lov_request.c
+++ b/drivers/staging/lustre/lustre/lov/lov_request.c
@@ -478,7 +478,7 @@ int lov_prep_match_set(struct obd_export *exp, struct obd_info *oinfo,
 		GOTO(out_set, rc = -ENOMEM);
 	lockh->cookie = set->set_lockh->llh_handle.h_cookie;
 
-	for (i = 0; i < lsm->lsm_stripe_count; i++){
+	for (i = 0; i < lsm->lsm_stripe_count; i++) {
 		struct lov_oinfo *loi;
 		struct lov_request *req;
 		obd_off start, end;
@@ -566,7 +566,7 @@ int lov_prep_cancel_set(struct obd_export *exp, struct obd_info *oinfo,
 	}
 	lockh->cookie = set->set_lockh->llh_handle.h_cookie;
 
-	for (i = 0; i < lsm->lsm_stripe_count; i++){
+	for (i = 0; i < lsm->lsm_stripe_count; i++) {
 		struct lov_request *req;
 		struct lustre_handle *lov_lockhp;
 		struct lov_oinfo *loi = lsm->lsm_oinfo[i];
@@ -734,7 +734,7 @@ int lov_prep_brw_set(struct obd_export *exp, struct obd_info *oinfo,
 
 	/* alloc and initialize lov request */
 	shift = 0;
-	for (i = 0; i < oinfo->oi_md->lsm_stripe_count; i++){
+	for (i = 0; i < oinfo->oi_md->lsm_stripe_count; i++) {
 		struct lov_oinfo *loi = NULL;
 		struct lov_request *req;
 
-- 
1.9.1