[PATCH v2] staging: dgnc: replace DGNC_VERIFY_BOARD macro with dgnc_verify_board function.

[PATCH v2] staging: dgnc: replace DGNC_VERIFY_BOARD macro with dgnc_verify_board function.

[Elise Lennion]

Fix checkpatch warning:

WARNING: Macros with flow control statements should be avoided

Macros with flow control statements should be avoided. They break
the flow of the calling function and make it harder to test the code.

Signed-off-by: Elise Lennion <elise.lennion@gmail.com>
---

v2: Fixed the return values of dgnc_verify_board. 
    Reworked to return NULL instead of 0, as suggested 
    by Julia Lawall <julia.lawall@lip6.fr>.

 drivers/staging/dgnc/dgnc_sysfs.c | 74 +++++++++++++++++++++++++++------------
 1 file changed, 51 insertions(+), 23 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_sysfs.c b/drivers/staging/dgnc/dgnc_sysfs.c
index 290bf6e..cdec40a 100644
--- a/drivers/staging/dgnc/dgnc_sysfs.c
+++ b/drivers/staging/dgnc/dgnc_sysfs.c
@@ -90,17 +90,21 @@ void dgnc_remove_driver_sysfiles(struct pci_driver *dgnc_driver)
 	driver_remove_file(driverfs, &driver_attr_pollrate);
 }
 
-#define DGNC_VERIFY_BOARD(p, bd)				\
-	do {							\
-		if (!p)						\
-			return 0;				\
-								\
-		bd = dev_get_drvdata(p);			\
-		if (!bd || bd->magic != DGNC_BOARD_MAGIC)	\
-			return 0;				\
-		if (bd->state != BOARD_READY)			\
-			return 0;				\
-	} while (0)
+static struct dgnc_board *dgnc_verify_board(struct device *p)
+{
+	struct dgnc_board *bd;
+
+	if (!p)
+		return NULL;
+
+	bd = dev_get_drvdata(p);
+	if (!bd || bd->magic != DGNC_BOARD_MAGIC)
+		return NULL;
+	if (bd->state != BOARD_READY)
+		return NULL;
+
+	return bd;
+}
 
 static ssize_t vpd_show(struct device *p, struct device_attribute *attr,
 			char *buf)
@@ -109,7 +113,9 @@ static ssize_t vpd_show(struct device *p, struct device_attribute *attr,
 	int count = 0;
 	int i = 0;
 
-	DGNC_VERIFY_BOARD(p, bd);
+	bd = dgnc_verify_board(p);
+	if (!bd)
+		return 0;
 
 	count += sprintf(buf + count,
 		"\n      0  1  2  3  4  5  6  7  8  9  A  B  C  D  E  F");
@@ -130,7 +136,9 @@ static ssize_t serial_number_show(struct device *p,
 	struct dgnc_board *bd;
 	int count = 0;
 
-	DGNC_VERIFY_BOARD(p, bd);
+	bd = dgnc_verify_board(p);
+	if (!bd)
+		return 0;
 
 	if (bd->serial_num[0] == '\0')
 		count += sprintf(buf + count, "<UNKNOWN>\n");
@@ -148,7 +156,9 @@ static ssize_t ports_state_show(struct device *p,
 	int count = 0;
 	int i = 0;
 
-	DGNC_VERIFY_BOARD(p, bd);
+	bd = dgnc_verify_board(p);
+	if (!bd)
+		return 0;
 
 	for (i = 0; i < bd->nasync; i++) {
 		count += snprintf(buf + count, PAGE_SIZE - count,
@@ -166,7 +176,9 @@ static ssize_t ports_baud_show(struct device *p,
 	int count = 0;
 	int i = 0;
 
-	DGNC_VERIFY_BOARD(p, bd);
+	bd = dgnc_verify_board(p);
+	if (!bd)
+		return 0;
 
 	for (i = 0; i < bd->nasync; i++) {
 		count +=  snprintf(buf + count, PAGE_SIZE - count,
@@ -184,7 +196,9 @@ static ssize_t ports_msignals_show(struct device *p,
 	int count = 0;
 	int i = 0;
 
-	DGNC_VERIFY_BOARD(p, bd);
+	bd = dgnc_verify_board(p);
+	if (!bd)
+		return 0;
 
 	for (i = 0; i < bd->nasync; i++) {
 		struct channel_t *ch = bd->channels[i];
@@ -215,7 +229,9 @@ static ssize_t ports_iflag_show(struct device *p,
 	int count = 0;
 	int i = 0;
 
-	DGNC_VERIFY_BOARD(p, bd);
+	bd = dgnc_verify_board(p);
+	if (!bd)
+		return 0;
 
 	for (i = 0; i < bd->nasync; i++) {
 		count += snprintf(buf + count, PAGE_SIZE - count, "%d %x\n",
@@ -233,7 +249,9 @@ static ssize_t ports_cflag_show(struct device *p,
 	int count = 0;
 	int i = 0;
 
-	DGNC_VERIFY_BOARD(p, bd);
+	bd = dgnc_verify_board(p);
+	if (!bd)
+		return 0;
 
 	for (i = 0; i < bd->nasync; i++) {
 		count += snprintf(buf + count, PAGE_SIZE - count, "%d %x\n",
@@ -251,7 +269,9 @@ static ssize_t ports_oflag_show(struct device *p,
 	int count = 0;
 	int i = 0;
 
-	DGNC_VERIFY_BOARD(p, bd);
+	bd = dgnc_verify_board(p);
+	if (!bd)
+		return 0;
 
 	for (i = 0; i < bd->nasync; i++) {
 		count += snprintf(buf + count, PAGE_SIZE - count, "%d %x\n",
@@ -269,7 +289,9 @@ static ssize_t ports_lflag_show(struct device *p,
 	int count = 0;
 	int i = 0;
 
-	DGNC_VERIFY_BOARD(p, bd);
+	bd = dgnc_verify_board(p);
+	if (!bd)
+		return 0;
 
 	for (i = 0; i < bd->nasync; i++) {
 		count += snprintf(buf + count, PAGE_SIZE - count, "%d %x\n",
@@ -287,7 +309,9 @@ static ssize_t ports_digi_flag_show(struct device *p,
 	int count = 0;
 	int i = 0;
 
-	DGNC_VERIFY_BOARD(p, bd);
+	bd = dgnc_verify_board(p);
+	if (!bd)
+		return 0;
 
 	for (i = 0; i < bd->nasync; i++) {
 		count += snprintf(buf + count, PAGE_SIZE - count, "%d %x\n",
@@ -305,7 +329,9 @@ static ssize_t ports_rxcount_show(struct device *p,
 	int count = 0;
 	int i = 0;
 
-	DGNC_VERIFY_BOARD(p, bd);
+	bd = dgnc_verify_board(p);
+	if (!bd)
+		return 0;
 
 	for (i = 0; i < bd->nasync; i++) {
 		count += snprintf(buf + count, PAGE_SIZE - count, "%d %ld\n",
@@ -323,7 +349,9 @@ static ssize_t ports_txcount_show(struct device *p,
 	int count = 0;
 	int i = 0;
 
-	DGNC_VERIFY_BOARD(p, bd);
+	bd = dgnc_verify_board(p);
+	if (!bd)
+		return 0;
 
 	for (i = 0; i < bd->nasync; i++) {
 		count += snprintf(buf + count, PAGE_SIZE - count, "%d %ld\n",
-- 
2.7.4

Re: [PATCH v2] staging: dgnc: replace DGNC_VERIFY_BOARD macro with dgnc_verify_board function.

[Greg KH]

On Mon, Oct 10, 2016 at 11:00:16AM -0300, Elise Lennion wrote:
> Fix checkpatch warning:
> 
> WARNING: Macros with flow control statements should be avoided
> 
> Macros with flow control statements should be avoided. They break
> the flow of the calling function and make it harder to test the code.
> 
> Signed-off-by: Elise Lennion <elise.lennion@gmail.com>
> ---
> 
> v2: Fixed the return values of dgnc_verify_board. 
>     Reworked to return NULL instead of 0, as suggested 
>     by Julia Lawall <julia.lawall@lip6.fr>.
> 
>  drivers/staging/dgnc/dgnc_sysfs.c | 74 +++++++++++++++++++++++++++------------
>  1 file changed, 51 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/staging/dgnc/dgnc_sysfs.c b/drivers/staging/dgnc/dgnc_sysfs.c
> index 290bf6e..cdec40a 100644
> --- a/drivers/staging/dgnc/dgnc_sysfs.c
> +++ b/drivers/staging/dgnc/dgnc_sysfs.c
> @@ -90,17 +90,21 @@ void dgnc_remove_driver_sysfiles(struct pci_driver *dgnc_driver)
>  	driver_remove_file(driverfs, &driver_attr_pollrate);
>  }
>  
> -#define DGNC_VERIFY_BOARD(p, bd)				\
> -	do {							\
> -		if (!p)						\
> -			return 0;				\
> -								\
> -		bd = dev_get_drvdata(p);			\
> -		if (!bd || bd->magic != DGNC_BOARD_MAGIC)	\
> -			return 0;				\
> -		if (bd->state != BOARD_READY)			\
> -			return 0;				\
> -	} while (0)
> +static struct dgnc_board *dgnc_verify_board(struct device *p)
> +{
> +	struct dgnc_board *bd;
> +
> +	if (!p)
> +		return NULL;
> +
> +	bd = dev_get_drvdata(p);
> +	if (!bd || bd->magic != DGNC_BOARD_MAGIC)
> +		return NULL;
> +	if (bd->state != BOARD_READY)
> +		return NULL;
> +
> +	return bd;
> +}

While this is a nice cleanup, take a step back and see what is being
done with this code.

First off, (!p) can never be true, so checking for it shows that the
original author didn't understand how sysfs worked.  After that, bd will
also always be not NULL, as the driver already set that up.  Same for
the ->magic field, that's pointless to check as it can't be wrong.

So maybe, BOARD_READY matters, but that's impossible to be not correct
as it is set before the sysfs files are created, and never changes from
what I can tell.

So all of these checks are pointless and can just be removed.

But then, step back even further, what code is checking for these
things?  It's odd sysfs files that I have said need to be removed from
the driver as they are pointless and not something that any tty driver
should have.

So how about just dropping the sysfs files instead?  That deletes a
bunch of code (some of it wrong and pointless as described above), gets
the driver closer to being mergable out of the staging directory, and it
keeps anyone from ever copying bad coding patterns like this ever again.

thanks,

greg k-h