[PATCH 0/3] staging: lustre: include: replace macros by functions

[PATCH 0/3] staging: lustre: include: replace macros by functions

[Aya Mahfouz]

Replace two macros with static inline functions that return values.
Inline functions are preferred because compilers are aware of them and
can optimize the calls. In addition, if macros are treated like
functions they can introduce bugs into the code.

Aya Mahfouz (3):
  staging: lustre: include: replace OBD_CHECK_DEV by obd_check_dev
  staging: lustre: include: replace OBD_CHECK_DEV_ACTIVE by
    obd_check_dev_active
  staging: lustre: include: remove unused macros

 drivers/staging/lustre/lustre/include/obd_class.h | 77 +++++++++++++++--------
 1 file changed, 50 insertions(+), 27 deletions(-)

-- 
2.1.0


-- 
Kind Regards,
Aya Saif El-yazal Mahfouz

[PATCH 1/3] staging: lustre: include: replace OBD_CHECK_DEV by obd_check_dev

[Aya Mahfouz]

Static inline functions are preferred over macros. Hence, the function
obd_check_dev was introduced. obd_check_dev replaces the macro
OBD_CHECK_DEV. All functions that call obd_check_dev store the return
values and return them if they represent an error code.

Signed-off-by: Aya Mahfouz <mahfouz.saif.elyazal@gmail.com>
---
 drivers/staging/lustre/lustre/include/obd_class.h | 33 ++++++++++++++++++-----
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/lustre/lustre/include/obd_class.h b/drivers/staging/lustre/lustre/include/obd_class.h
index 5314f6f..ddc212a 100644
--- a/drivers/staging/lustre/lustre/include/obd_class.h
+++ b/drivers/staging/lustre/lustre/include/obd_class.h
@@ -341,6 +341,15 @@ do {							    \
 	}						       \
 } while (0)
 
+static inline int obd_check_dev(struct obd_device *obd)
+{
+	if (!(obd)) {
+		CERROR("NULL device\n");
+		return -ENODEV;
+	}
+	return 0;
+}
+
 /* ensure obd_setup and !obd_stopping */
 #define OBD_CHECK_DEV_ACTIVE(obd)			       \
 do {							    \
@@ -594,7 +603,9 @@ static inline int obd_precleanup(struct obd_device *obd,
 	int rc;
 	DECLARE_LU_VARS(ldt, d);
 
-	OBD_CHECK_DEV(obd);
+	rc = obd_check_dev(obd);
+	if (rc)
+		return rc;
 	ldt = obd->obd_type->typ_lu;
 	d = obd->obd_lu_dev;
 	if (ldt != NULL && d != NULL) {
@@ -620,7 +631,9 @@ static inline int obd_cleanup(struct obd_device *obd)
 	int rc;
 	DECLARE_LU_VARS(ldt, d);
 
-	OBD_CHECK_DEV(obd);
+	rc = obd_check_dev(obd);
+	if (rc)
+		return rc;
 
 	ldt = obd->obd_type->typ_lu;
 	d = obd->obd_lu_dev;
@@ -668,7 +681,9 @@ obd_process_config(struct obd_device *obd, int datalen, void *data)
 	int rc;
 	DECLARE_LU_VARS(ldt, d);
 
-	OBD_CHECK_DEV(obd);
+	rc = obd_check_dev(obd);
+	if (rc)
+		return rc;
 
 	obd->obd_process_conf = 1;
 	ldt = obd->obd_type->typ_lu;
@@ -1280,7 +1295,9 @@ static inline int obd_notify(struct obd_device *obd,
 {
 	int rc;
 
-	OBD_CHECK_DEV(obd);
+	rc = obd_check_dev(obd);
+	if (rc)
+		return rc;
 
 	/* the check for async_recov is a complete hack - I'm hereby
 	   overloading the meaning to also mean "this was called from
@@ -1381,7 +1398,11 @@ static inline int obd_health_check(const struct lu_env *env,
 static inline int obd_register_observer(struct obd_device *obd,
 					struct obd_device *observer)
 {
-	OBD_CHECK_DEV(obd);
+	int rc;
+
+	rc = obd_check_dev(obd);
+	if (rc)
+		return rc;
 	down_write(&obd->obd_observer_link_sem);
 	if (obd->obd_observer && observer) {
 		up_write(&obd->obd_observer_link_sem);
@@ -1389,7 +1410,7 @@ static inline int obd_register_observer(struct obd_device *obd,
 	}
 	obd->obd_observer = observer;
 	up_write(&obd->obd_observer_link_sem);
-	return 0;
+	return rc;
 }
 
 #if 0
-- 
2.1.0


-- 
Kind Regards,
Aya Saif El-yazal Mahfouz

[PATCH 2/3] staging: lustre: include: replace OBD_CHECK_DEV_ACTIVE by obd_check_dev_active

[Aya Mahfouz]

Static inline functions are preferred over macros. The inline function
obd_check_dev_active is introduced to replace OBD_CHECK_DEV_ACTIVE.
All functions that call obd_check_dev_active store the return values
and return them if they represent an error code.

Signed-off-by: Aya Mahfouz <mahfouz.saif.elyazal@gmail.com>
---
 drivers/staging/lustre/lustre/include/obd_class.h | 28 +++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/lustre/lustre/include/obd_class.h b/drivers/staging/lustre/lustre/include/obd_class.h
index ddc212a..3c444fa 100644
--- a/drivers/staging/lustre/lustre/include/obd_class.h
+++ b/drivers/staging/lustre/lustre/include/obd_class.h
@@ -361,7 +361,19 @@ do {							    \
 	}						       \
 } while (0)
 
+static inline int obd_check_dev_active(struct obd_device *obd)
+{
+	int rc;
 
+	rc = obd_check_dev(obd);
+	if (rc)
+		return rc;
+	if (!(obd)->obd_set_up || (obd)->obd_stopping) {
+		CERROR("Device %d not setup\n", (obd)->obd_minor);
+		return -ENODEV;
+	}
+	return rc;
+}
 #if defined (CONFIG_PROC_FS)
 #define OBD_COUNTER_OFFSET(op)				  \
 	((offsetof(struct obd_ops, o_ ## op) -		  \
@@ -901,7 +913,9 @@ static inline int obd_add_conn(struct obd_import *imp, struct obd_uuid *uuid,
 	struct obd_device *obd = imp->imp_obd;
 	int rc;
 
-	OBD_CHECK_DEV_ACTIVE(obd);
+	rc = obd_check_dev_active(obd);
+	if (rc)
+		return rc;
 	OBD_CHECK_DT_OP(obd, add_conn, -EOPNOTSUPP);
 	OBD_COUNTER_INCREMENT(obd, add_conn);
 
@@ -914,7 +928,9 @@ static inline int obd_del_conn(struct obd_import *imp, struct obd_uuid *uuid)
 	struct obd_device *obd = imp->imp_obd;
 	int rc;
 
-	OBD_CHECK_DEV_ACTIVE(obd);
+	rc = obd_check_dev_active(obd);
+	if (rc)
+		return rc;
 	OBD_CHECK_DT_OP(obd, del_conn, -EOPNOTSUPP);
 	OBD_COUNTER_INCREMENT(obd, del_conn);
 
@@ -948,7 +964,9 @@ static inline int obd_connect(const struct lu_env *env,
 	__u64 ocf = data ? data->ocd_connect_flags : 0; /* for post-condition
 						   * check */
 
-	OBD_CHECK_DEV_ACTIVE(obd);
+	rc = obd_check_dev_active(obd);
+	if (rc)
+		return rc;
 	OBD_CHECK_DT_OP(obd, connect, -EOPNOTSUPP);
 	OBD_COUNTER_INCREMENT(obd, connect);
 
@@ -970,7 +988,9 @@ static inline int obd_reconnect(const struct lu_env *env,
 	__u64 ocf = d ? d->ocd_connect_flags : 0; /* for post-condition
 						   * check */
 
-	OBD_CHECK_DEV_ACTIVE(obd);
+	rc = obd_check_dev_active(obd);
+	if (rc)
+		return rc;
 	OBD_CHECK_DT_OP(obd, reconnect, 0);
 	OBD_COUNTER_INCREMENT(obd, reconnect);
 
-- 
2.1.0


-- 
Kind Regards,
Aya Saif El-yazal Mahfouz

[PATCH 3/3] staging: lustre: include: remove unused macros

[Aya Mahfouz]

OBD_CHECK_DEV and OBD_CHECK_DEV_ACTIVE have been replaced by static
inline functions. They are removed since they are not used anymore.

Signed-off-by: Aya Mahfouz <mahfouz.saif.elyazal@gmail.com>
---
 drivers/staging/lustre/lustre/include/obd_class.h | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/drivers/staging/lustre/lustre/include/obd_class.h b/drivers/staging/lustre/lustre/include/obd_class.h
index 3c444fa..9af2b43 100644
--- a/drivers/staging/lustre/lustre/include/obd_class.h
+++ b/drivers/staging/lustre/lustre/include/obd_class.h
@@ -333,14 +333,6 @@ void obdo_le_to_cpu(struct obdo *dobdo, struct obdo *sobdo);
 
 /* Ensure obd_setup: used for cleanup which must be called
    while obd is stopping */
-#define OBD_CHECK_DEV(obd)				      \
-do {							    \
-	if (!(obd)) {					   \
-		CERROR("NULL device\n");			\
-		return -ENODEV;				\
-	}						       \
-} while (0)
-
 static inline int obd_check_dev(struct obd_device *obd)
 {
 	if (!(obd)) {
@@ -351,16 +343,6 @@ static inline int obd_check_dev(struct obd_device *obd)
 }
 
 /* ensure obd_setup and !obd_stopping */
-#define OBD_CHECK_DEV_ACTIVE(obd)			       \
-do {							    \
-	OBD_CHECK_DEV(obd);				     \
-	if (!(obd)->obd_set_up || (obd)->obd_stopping) {	\
-		CERROR("Device %d not setup\n",		 \
-		       (obd)->obd_minor);		       \
-		return -ENODEV;				\
-	}						       \
-} while (0)
-
 static inline int obd_check_dev_active(struct obd_device *obd)
 {
 	int rc;
-- 
2.1.0


-- 
Kind Regards,
Aya Saif El-yazal Mahfouz

Re: [Outreachy kernel] [PATCH 1/3] staging: lustre: include: replace OBD_CHECK_DEV by obd_check_dev

[Julia Lawall]

> +static inline int obd_check_dev(struct obd_device *obd)
> +{
> +	if (!(obd)) {

Now that it is a function, obd is just a variable.  So the parentheses are
not needed.

julia

> +		CERROR("NULL device\n");
> +		return -ENODEV;
> +	}
> +	return 0;
> +}
> +
>  /* ensure obd_setup and !obd_stopping */
>  #define OBD_CHECK_DEV_ACTIVE(obd)			       \
>  do {							    \
> @@ -594,7 +603,9 @@ static inline int obd_precleanup(struct obd_device *obd,
>  	int rc;
>  	DECLARE_LU_VARS(ldt, d);
>
> -	OBD_CHECK_DEV(obd);
> +	rc = obd_check_dev(obd);
> +	if (rc)
> +		return rc;
>  	ldt = obd->obd_type->typ_lu;
>  	d = obd->obd_lu_dev;
>  	if (ldt != NULL && d != NULL) {
> @@ -620,7 +631,9 @@ static inline int obd_cleanup(struct obd_device *obd)
>  	int rc;
>  	DECLARE_LU_VARS(ldt, d);
>
> -	OBD_CHECK_DEV(obd);
> +	rc = obd_check_dev(obd);
> +	if (rc)
> +		return rc;
>
>  	ldt = obd->obd_type->typ_lu;
>  	d = obd->obd_lu_dev;
> @@ -668,7 +681,9 @@ obd_process_config(struct obd_device *obd, int datalen, void *data)
>  	int rc;
>  	DECLARE_LU_VARS(ldt, d);
>
> -	OBD_CHECK_DEV(obd);
> +	rc = obd_check_dev(obd);
> +	if (rc)
> +		return rc;
>
>  	obd->obd_process_conf = 1;
>  	ldt = obd->obd_type->typ_lu;
> @@ -1280,7 +1295,9 @@ static inline int obd_notify(struct obd_device *obd,
>  {
>  	int rc;
>
> -	OBD_CHECK_DEV(obd);
> +	rc = obd_check_dev(obd);
> +	if (rc)
> +		return rc;
>
>  	/* the check for async_recov is a complete hack - I'm hereby
>  	   overloading the meaning to also mean "this was called from
> @@ -1381,7 +1398,11 @@ static inline int obd_health_check(const struct lu_env *env,
>  static inline int obd_register_observer(struct obd_device *obd,
>  					struct obd_device *observer)
>  {
> -	OBD_CHECK_DEV(obd);
> +	int rc;
> +
> +	rc = obd_check_dev(obd);
> +	if (rc)
> +		return rc;
>  	down_write(&obd->obd_observer_link_sem);
>  	if (obd->obd_observer && observer) {
>  		up_write(&obd->obd_observer_link_sem);
> @@ -1389,7 +1410,7 @@ static inline int obd_register_observer(struct obd_device *obd,
>  	}
>  	obd->obd_observer = observer;
>  	up_write(&obd->obd_observer_link_sem);
> -	return 0;
> +	return rc;
>  }
>
>  #if 0
> --
> 2.1.0
>
>
> --
> Kind Regards,
> Aya Saif El-yazal Mahfouz
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/5ad6d9280a9080c3be07ca184ecdf84501c24c94.1427894168.git.mahfouz.saif.elyazal%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH 2/3] staging: lustre: include: replace OBD_CHECK_DEV_ACTIVE by obd_check_dev_active

[Julia Lawall]

> +static inline int obd_check_dev_active(struct obd_device *obd)
> +{
> +	int rc;
>
> +	rc = obd_check_dev(obd);
> +	if (rc)
> +		return rc;
> +	if (!(obd)->obd_set_up || (obd)->obd_stopping) {

You can get rid of the parentheses here too.

julia

> +		CERROR("Device %d not setup\n", (obd)->obd_minor);
> +		return -ENODEV;
> +	}
> +	return rc;
> +}
>  #if defined (CONFIG_PROC_FS)
>  #define OBD_COUNTER_OFFSET(op)				  \
>  	((offsetof(struct obd_ops, o_ ## op) -		  \
> @@ -901,7 +913,9 @@ static inline int obd_add_conn(struct obd_import *imp, struct obd_uuid *uuid,
>  	struct obd_device *obd = imp->imp_obd;
>  	int rc;
>
> -	OBD_CHECK_DEV_ACTIVE(obd);
> +	rc = obd_check_dev_active(obd);
> +	if (rc)
> +		return rc;
>  	OBD_CHECK_DT_OP(obd, add_conn, -EOPNOTSUPP);
>  	OBD_COUNTER_INCREMENT(obd, add_conn);
>
> @@ -914,7 +928,9 @@ static inline int obd_del_conn(struct obd_import *imp, struct obd_uuid *uuid)
>  	struct obd_device *obd = imp->imp_obd;
>  	int rc;
>
> -	OBD_CHECK_DEV_ACTIVE(obd);
> +	rc = obd_check_dev_active(obd);
> +	if (rc)
> +		return rc;
>  	OBD_CHECK_DT_OP(obd, del_conn, -EOPNOTSUPP);
>  	OBD_COUNTER_INCREMENT(obd, del_conn);
>
> @@ -948,7 +964,9 @@ static inline int obd_connect(const struct lu_env *env,
>  	__u64 ocf = data ? data->ocd_connect_flags : 0; /* for post-condition
>  						   * check */
>
> -	OBD_CHECK_DEV_ACTIVE(obd);
> +	rc = obd_check_dev_active(obd);
> +	if (rc)
> +		return rc;
>  	OBD_CHECK_DT_OP(obd, connect, -EOPNOTSUPP);
>  	OBD_COUNTER_INCREMENT(obd, connect);
>
> @@ -970,7 +988,9 @@ static inline int obd_reconnect(const struct lu_env *env,
>  	__u64 ocf = d ? d->ocd_connect_flags : 0; /* for post-condition
>  						   * check */
>
> -	OBD_CHECK_DEV_ACTIVE(obd);
> +	rc = obd_check_dev_active(obd);
> +	if (rc)
> +		return rc;
>  	OBD_CHECK_DT_OP(obd, reconnect, 0);
>  	OBD_COUNTER_INCREMENT(obd, reconnect);
>
> --
> 2.1.0
>
>
> --
> Kind Regards,
> Aya Saif El-yazal Mahfouz
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/c9c6240dc1e51a0ffdcdfe0fc7b392712b7ee4e9.1427894169.git.mahfouz.saif.elyazal%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH 2/3] staging: lustre: include: replace OBD_CHECK_DEV_ACTIVE by obd_check_dev_active

[Aya Mahfouz]

On Wed, Apr 01, 2015 at 04:35:37PM +0200, Julia Lawall wrote:
> > +static inline int obd_check_dev_active(struct obd_device *obd)
> > +{
> > +	int rc;
> >
> > +	rc = obd_check_dev(obd);
> > +	if (rc)
> > +		return rc;
> > +	if (!(obd)->obd_set_up || (obd)->obd_stopping) {
> 
> You can get rid of the parentheses here too.
> 
> julia
> 

Definitely! Thanks Julia!

> > +		CERROR("Device %d not setup\n", (obd)->obd_minor);
> > +		return -ENODEV;
> > +	}
> > +	return rc;
> > +}
> >  #if defined (CONFIG_PROC_FS)
> >  #define OBD_COUNTER_OFFSET(op)				  \
> >  	((offsetof(struct obd_ops, o_ ## op) -		  \
> > @@ -901,7 +913,9 @@ static inline int obd_add_conn(struct obd_import *imp, struct obd_uuid *uuid,
> >  	struct obd_device *obd = imp->imp_obd;
> >  	int rc;
> >
> > -	OBD_CHECK_DEV_ACTIVE(obd);
> > +	rc = obd_check_dev_active(obd);
> > +	if (rc)
> > +		return rc;
> >  	OBD_CHECK_DT_OP(obd, add_conn, -EOPNOTSUPP);
> >  	OBD_COUNTER_INCREMENT(obd, add_conn);
> >
> > @@ -914,7 +928,9 @@ static inline int obd_del_conn(struct obd_import *imp, struct obd_uuid *uuid)
> >  	struct obd_device *obd = imp->imp_obd;
> >  	int rc;
> >
> > -	OBD_CHECK_DEV_ACTIVE(obd);
> > +	rc = obd_check_dev_active(obd);
> > +	if (rc)
> > +		return rc;
> >  	OBD_CHECK_DT_OP(obd, del_conn, -EOPNOTSUPP);
> >  	OBD_COUNTER_INCREMENT(obd, del_conn);
> >
> > @@ -948,7 +964,9 @@ static inline int obd_connect(const struct lu_env *env,
> >  	__u64 ocf = data ? data->ocd_connect_flags : 0; /* for post-condition
> >  						   * check */
> >
> > -	OBD_CHECK_DEV_ACTIVE(obd);
> > +	rc = obd_check_dev_active(obd);
> > +	if (rc)
> > +		return rc;
> >  	OBD_CHECK_DT_OP(obd, connect, -EOPNOTSUPP);
> >  	OBD_COUNTER_INCREMENT(obd, connect);
> >
> > @@ -970,7 +988,9 @@ static inline int obd_reconnect(const struct lu_env *env,
> >  	__u64 ocf = d ? d->ocd_connect_flags : 0; /* for post-condition
> >  						   * check */
> >
> > -	OBD_CHECK_DEV_ACTIVE(obd);
> > +	rc = obd_check_dev_active(obd);
> > +	if (rc)
> > +		return rc;
> >  	OBD_CHECK_DT_OP(obd, reconnect, 0);
> >  	OBD_COUNTER_INCREMENT(obd, reconnect);
> >
> > --
> > 2.1.0
> >
> >
> > --
> > Kind Regards,
> > Aya Saif El-yazal Mahfouz
> >
> > --
> > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> > To post to this group, send email to outreachy-kernel@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/c9c6240dc1e51a0ffdcdfe0fc7b392712b7ee4e9.1427894169.git.mahfouz.saif.elyazal%40gmail.com.
> > For more options, visit https://groups.google.com/d/optout.
> >

-- 
Kind Regards,
Aya Saif El-yazal Mahfouz