[PATCH 0/5] ISDN-CAPI: Fine-tuning for several function implementations

STALE3671d

13 messages, 3 authors, 2016-09-26 · open the first message on its own page

[PATCH 0/5] ISDN-CAPI: Fine-tuning for several function implementations

From: SF Markus Elfring <hidden>
Date: 2016-09-25 11:10:16

From: Markus Elfring <redacted>
Date: Sun, 25 Sep 2016 13:03:13 +0200

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

Markus Elfring (5):
  Use kmalloc_array() in capidrv_addcontr()
  Delete error messages for a failed memory allocation in four functions
  Adjust 17 function calls together with variable assignments
  Adjust checks for null pointers in four functions
  Delete unnecessary braces

 drivers/isdn/capi/capidrv.c | 111 +++++++++++++++++++++-----------------------
 1 file changed, 54 insertions(+), 57 deletions(-)

-- 
2.10.0

[PATCH 1/5] ISDN-CAPI: Use kmalloc_array() in capidrv_addcontr()

From: SF Markus Elfring <hidden>
Date: 2016-09-25 11:11:21

From: Markus Elfring <redacted>
Date: Sun, 25 Sep 2016 11:06:17 +0200

A multiplication for the size determination of a memory allocation
indicated that an array data structure should be processed.
Thus use the corresponding function "kmalloc_array".

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <redacted>
---
 drivers/isdn/capi/capidrv.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/isdn/capi/capidrv.c b/drivers/isdn/capi/capidrv.c
index 85cfa4f..cd8e1a6 100644
--- a/drivers/isdn/capi/capidrv.c
+++ b/drivers/isdn/capi/capidrv.c

@@ -2268,7 +2268,9 @@ static int capidrv_addcontr(u16 contr, struct capi_profile *profp)
 	strcpy(card->name, id);
 	card->contrnr = contr;
 	card->nbchan = profp->nbchannel;
-	card->bchans = kmalloc(sizeof(capidrv_bchan) * card->nbchan, GFP_ATOMIC);
+	card->bchans = kmalloc_array(card->nbchan,
+				     sizeof(capidrv_bchan),
+				     GFP_ATOMIC);
 	if (!card->bchans) {
 		printk(KERN_WARNING
 		       "capidrv: (%s) Could not allocate bchan-structs.\n", id);

-- 
2.10.0

[PATCH 2/5] ISDN-CAPI: Delete error messages for a failed memory allocation in four functions

From: SF Markus Elfring <hidden>
Date: 2016-09-25 11:12:36

From: Markus Elfring <redacted>
Date: Sun, 25 Sep 2016 11:17:39 +0200

Omit an extra message for a memory allocation failure in a few functions.

Link: http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf

Signed-off-by: Markus Elfring <redacted>
---
 drivers/isdn/capi/capidrv.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/isdn/capi/capidrv.c b/drivers/isdn/capi/capidrv.c
index cd8e1a6..bb945dd 100644
--- a/drivers/isdn/capi/capidrv.c
+++ b/drivers/isdn/capi/capidrv.c

@@ -471,7 +471,6 @@ static int capidrv_add_ack(struct capidrv_ncci *nccip,
 
 	n = kmalloc(sizeof(struct ncci_datahandle_queue), GFP_ATOMIC);
 	if (!n) {
-		printk(KERN_ERR "capidrv: kmalloc ncci_datahandle failed\n");
 		return -1;
 	}
 	n->next = NULL;

@@ -513,7 +512,6 @@ static void send_message(capidrv_contr *card, _cmsg *cmsg)
 	len = CAPIMSG_LEN(cmsg->buf);
 	skb = alloc_skb(len, GFP_ATOMIC);
 	if (!skb) {
-		printk(KERN_ERR "capidrv::send_message: can't allocate mem\n");
 		return;
 	}
 	memcpy(skb_put(skb, len), cmsg->buf, len);

@@ -2111,8 +2109,6 @@ static int if_sendbuf(int id, int channel, int doack, struct sk_buff *skb)
 	if (skb_headroom(skb) < msglen) {
 		struct sk_buff *nskb = skb_realloc_headroom(skb, msglen);
 		if (!nskb) {
-			printk(KERN_ERR "capidrv-%d: if_sendbuf: no memory\n",
-			       card->contrnr);
 			(void)capidrv_del_ack(nccip, datahandle);
 			return 0;
 		}

@@ -2259,8 +2255,6 @@ static int capidrv_addcontr(u16 contr, struct capi_profile *profp)
 		return -1;
 	}
 	if (!(card = kzalloc(sizeof(capidrv_contr), GFP_ATOMIC))) {
-		printk(KERN_WARNING
-		       "capidrv: (%s) Could not allocate contr-struct.\n", id);
 		return -1;
 	}
 	card->owner = THIS_MODULE;

@@ -2272,8 +2266,6 @@ static int capidrv_addcontr(u16 contr, struct capi_profile *profp)
 				     sizeof(capidrv_bchan),
 				     GFP_ATOMIC);
 	if (!card->bchans) {
-		printk(KERN_WARNING
-		       "capidrv: (%s) Could not allocate bchan-structs.\n", id);
 		module_put(card->owner);
 		kfree(card);
 		return -1;

-- 
2.10.0

[PATCH 3/5] ISDN-CAPI: Adjust 17 function calls together with variable assignments

From: SF Markus Elfring <hidden>
Date: 2016-09-25 11:13:27

From: Markus Elfring <redacted>
Date: Sun, 25 Sep 2016 12:21:37 +0200

The script "checkpatch.pl" can point out that assignments should usually
not be performed within condition checks.
Thus move the assignment for a variable to a separate statement
in four functions.

Signed-off-by: Markus Elfring <redacted>
---
 drivers/isdn/capi/capidrv.c | 59 +++++++++++++++++++++++++++------------------
 1 file changed, 36 insertions(+), 23 deletions(-)

diff --git a/drivers/isdn/capi/capidrv.c b/drivers/isdn/capi/capidrv.c
index bb945dd..bd614e3 100644
--- a/drivers/isdn/capi/capidrv.c
+++ b/drivers/isdn/capi/capidrv.c

@@ -426,7 +426,8 @@ static inline capidrv_ncci *find_ncci(capidrv_contr *card, u32 ncci)
 	capidrv_plci *plcip;
 	capidrv_ncci *p;
 
-	if ((plcip = find_plci_by_ncci(card, ncci)) == NULL)
+	plcip = find_plci_by_ncci(card, ncci);
+	if (!plcip)
 		return NULL;
 
 	for (p = plcip->ncci_list; p; p = p->next)

@@ -441,7 +442,8 @@ static inline capidrv_ncci *find_ncci_by_msgid(capidrv_contr *card,
 	capidrv_plci *plcip;
 	capidrv_ncci *p;
 
-	if ((plcip = find_plci_by_ncci(card, ncci)) == NULL)
+	plcip = find_plci_by_ncci(card, ncci);
+	if (!plcip)
 		return NULL;
 
 	for (p = plcip->ncci_list; p; p = p->next)

@@ -1072,7 +1074,8 @@ static void handle_incoming_call(capidrv_contr *card, _cmsg *cmsg)
 		return;
 	}
 	bchan = &card->bchans[chan];
-	if ((plcip = new_plci(card, chan)) == NULL) {
+	plcip = new_plci(card, chan);
+	if (!plcip) {
 		printk(KERN_ERR "capidrv-%d: incoming call: no memory, sorry.\n", card->contrnr);
 		return;
 	}

@@ -1207,7 +1210,8 @@ static void handle_plci(_cmsg *cmsg)
 			       capi_cmd2str(cmsg->Command, cmsg->Subcommand),
 			       cmsg->Reason, capi_info2str(cmsg->Reason), cmsg->adr.adrPLCI);
 		}
-		if (!(plcip = find_plci_by_plci(card, cmsg->adr.adrPLCI))) {
+		plcip = find_plci_by_plci(card, cmsg->adr.adrPLCI);
+		if (!plcip) {
 			capi_cmsg_answer(cmsg);
 			send_message(card, cmsg);
 			goto notfound;

@@ -1227,7 +1231,8 @@ static void handle_plci(_cmsg *cmsg)
 			       cmsg->Info, capi_info2str(cmsg->Info),
 			       cmsg->adr.adrPLCI);
 		}
-		if (!(plcip = find_plci_by_plci(card, cmsg->adr.adrPLCI)))
+		plcip = find_plci_by_plci(card, cmsg->adr.adrPLCI);
+		if (!plcip)
 			goto notfound;
 
 		card->bchans[plcip->chan].disconnecting = 1;

@@ -1255,7 +1260,8 @@ static void handle_plci(_cmsg *cmsg)
 			       cmsg->Info, capi_info2str(cmsg->Info),
 			       cmsg->adr.adrPLCI);
 		}
-		if (!(plcip = find_plci_by_msgid(card, cmsg->Messagenumber)))
+		plcip = find_plci_by_msgid(card, cmsg->Messagenumber);
+		if (!plcip)
 			goto notfound;
 
 		plcip->plci = cmsg->adr.adrPLCI;

@@ -1267,8 +1273,8 @@ static void handle_plci(_cmsg *cmsg)
 		break;
 
 	case CAPI_CONNECT_ACTIVE_IND:	/* plci */
-
-		if (!(plcip = find_plci_by_plci(card, cmsg->adr.adrPLCI)))
+		plcip = find_plci_by_plci(card, cmsg->adr.adrPLCI);
+		if (!plcip)
 			goto notfound;
 
 		if (card->bchans[plcip->chan].incoming) {

@@ -1305,8 +1311,8 @@ static void handle_plci(_cmsg *cmsg)
 		break;
 
 	case CAPI_INFO_IND:	/* Controller/plci */
-
-		if (!(plcip = find_plci_by_plci(card, cmsg->adr.adrPLCI)))
+		plcip = find_plci_by_plci(card, cmsg->adr.adrPLCI);
+		if (!plcip)
 			goto notfound;
 
 		if (cmsg->InfoNumber == 0x4000) {

@@ -1385,7 +1391,8 @@ static void handle_ncci(_cmsg *cmsg)
 	switch (CAPICMD(cmsg->Command, cmsg->Subcommand)) {
 
 	case CAPI_CONNECT_B3_ACTIVE_IND:	/* ncci */
-		if (!(nccip = find_ncci(card, cmsg->adr.adrNCCI)))
+		nccip = find_ncci(card, cmsg->adr.adrNCCI);
+		if (!nccip)
 			goto notfound;
 
 		capi_cmsg_answer(cmsg);

@@ -1440,10 +1447,10 @@ static void handle_ncci(_cmsg *cmsg)
 		break;
 
 	case CAPI_CONNECT_B3_CONF:	/* ncci */
-
-		if (!(nccip = find_ncci_by_msgid(card,
-						 cmsg->adr.adrNCCI,
-						 cmsg->Messagenumber)))
+		nccip = find_ncci_by_msgid(card,
+					   cmsg->adr.adrNCCI,
+					   cmsg->Messagenumber);
+		if (!nccip)
 			goto notfound;
 
 		nccip->ncci = cmsg->adr.adrNCCI;

@@ -1475,7 +1482,8 @@ static void handle_ncci(_cmsg *cmsg)
 			printk(KERN_WARNING "CAPI_DATA_B3_CONF: Info %x - %s\n",
 			       cmsg->Info, capi_info2str(cmsg->Info));
 		}
-		if (!(nccip = find_ncci(card, cmsg->adr.adrNCCI)))
+		nccip = find_ncci(card, cmsg->adr.adrNCCI);
+		if (!nccip)
 			goto notfound;
 
 		len = capidrv_del_ack(nccip, cmsg->DataHandle);

@@ -1489,7 +1497,8 @@ static void handle_ncci(_cmsg *cmsg)
 		break;
 
 	case CAPI_DISCONNECT_B3_IND:	/* ncci */
-		if (!(nccip = find_ncci(card, cmsg->adr.adrNCCI)))
+		nccip = find_ncci(card, cmsg->adr.adrNCCI);
+		if (!nccip)
 			goto notfound;
 
 		card->bchans[nccip->chan].disconnecting = 1;

@@ -1500,7 +1509,8 @@ static void handle_ncci(_cmsg *cmsg)
 		break;
 
 	case CAPI_DISCONNECT_B3_CONF:	/* ncci */
-		if (!(nccip = find_ncci(card, cmsg->adr.adrNCCI)))
+		nccip = find_ncci(card, cmsg->adr.adrNCCI);
+		if (!nccip)
 			goto notfound;
 		if (cmsg->Info) {
 			printk(KERN_INFO "capidrv-%d: %s info 0x%x (%s) for ncci 0x%x\n",

@@ -1513,7 +1523,8 @@ static void handle_ncci(_cmsg *cmsg)
 		break;
 
 	case CAPI_RESET_B3_IND:	/* ncci */
-		if (!(nccip = find_ncci(card, cmsg->adr.adrNCCI)))
+		nccip = find_ncci(card, cmsg->adr.adrNCCI);
+		if (!nccip)
 			goto notfound;
 		ncci_change_state(card, nccip, EV_NCCI_RESET_B3_IND);
 		capi_cmsg_answer(cmsg);

@@ -1561,7 +1572,8 @@ static void handle_data(_cmsg *cmsg, struct sk_buff *skb)
 		kfree_skb(skb);
 		return;
 	}
-	if (!(nccip = find_ncci(card, cmsg->adr.adrNCCI))) {
+	nccip = find_ncci(card, cmsg->adr.adrNCCI);
+	if (!nccip) {
 		printk(KERN_ERR "capidrv-%d: %s: ncci 0x%x not found\n",
 		       card->contrnr,
 		       capi_cmd2str(cmsg->Command, cmsg->Subcommand),

@@ -1868,7 +1880,8 @@ static int capidrv_command(isdn_ctrl *c, capidrv_contr *card)
 				      NULL,	/* Useruserdata */
 				      NULL	/* Facilitydataarray */
 			);
-		if ((plcip = new_plci(card, (c->arg % card->nbchan))) == NULL) {
+		plcip = new_plci(card, c->arg % card->nbchan);
+		if (!plcip) {
 			cmd.command = ISDN_STAT_DHUP;
 			cmd.driver = card->myid;
 			cmd.arg = (c->arg % card->nbchan);

@@ -2254,9 +2267,9 @@ static int capidrv_addcontr(u16 contr, struct capi_profile *profp)
 		printk(KERN_WARNING "capidrv: (%s) Could not reserve module\n", id);
 		return -1;
 	}
-	if (!(card = kzalloc(sizeof(capidrv_contr), GFP_ATOMIC))) {
+	card = kzalloc(sizeof(capidrv_contr), GFP_ATOMIC);
+	if (!card)
 		return -1;
-	}
 	card->owner = THIS_MODULE;
 	setup_timer(&card->listentimer, listentimerfunc, (unsigned long)card);
 	strcpy(card->name, id);

-- 
2.10.0

[PATCH 4/5] ISDN-CAPI: Adjust checks for null pointers in four functions

From: SF Markus Elfring <hidden>
Date: 2016-09-25 11:14:13

From: Markus Elfring <redacted>
Date: Sun, 25 Sep 2016 12:26:38 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The script "checkpatch.pl" can point information out like the following.

Comparison to NULL could be written !…

Thus fix the affected source code places.

Signed-off-by: Markus Elfring <redacted>
---
 drivers/isdn/capi/capidrv.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/isdn/capi/capidrv.c b/drivers/isdn/capi/capidrv.c
index bd614e3..83f756d 100644
--- a/drivers/isdn/capi/capidrv.c
+++ b/drivers/isdn/capi/capidrv.c

@@ -334,8 +334,7 @@ static capidrv_plci *new_plci(capidrv_contr *card, int chan)
 	capidrv_plci *plcip;
 
 	plcip = kzalloc(sizeof(capidrv_plci), GFP_ATOMIC);
-
-	if (plcip == NULL)
+	if (!plcip)
 		return NULL;
 
 	plcip->state = ST_PLCI_NONE;

@@ -403,8 +402,7 @@ static inline capidrv_ncci *new_ncci(capidrv_contr *card,
 	capidrv_ncci *nccip;
 
 	nccip = kzalloc(sizeof(capidrv_ncci), GFP_ATOMIC);
-
-	if (nccip == NULL)
+	if (!nccip)
 		return NULL;
 
 	nccip->ncci = ncci;

@@ -757,7 +755,7 @@ static inline int new_bchan(capidrv_contr *card)
 {
 	int i;
 	for (i = 0; i < card->nbchan; i++) {
-		if (card->bchans[i].plcip == NULL) {
+		if (!card->bchans[i].plcip) {
 			card->bchans[i].disconnecting = 0;
 			return i;
 		}

@@ -2192,7 +2190,7 @@ static void enable_dchannel_trace(capidrv_contr *card)
 		       card->name, errcode);
 		return;
 	}
-	if (strstr(manufacturer, "AVM") == NULL) {
+	if (!strstr(manufacturer, "AVM")) {
 		printk(KERN_ERR "%s: not from AVM, no d-channel trace possible (%s)\n",
 		       card->name, manufacturer);
 		return;

-- 
2.10.0

[PATCH 5/5] ISDN-CAPI: Delete unnecessary braces

From: SF Markus Elfring <hidden>
Date: 2016-09-25 11:15:16

From: Markus Elfring <redacted>
Date: Sun, 25 Sep 2016 12:50:21 +0200

Do not use curly brackets at eight source code places
where a single statement should be sufficient.

Signed-off-by: Markus Elfring <redacted>
---
 drivers/isdn/capi/capidrv.c | 30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/drivers/isdn/capi/capidrv.c b/drivers/isdn/capi/capidrv.c
index 83f756d..7f58644 100644
--- a/drivers/isdn/capi/capidrv.c
+++ b/drivers/isdn/capi/capidrv.c

@@ -470,9 +470,8 @@ static int capidrv_add_ack(struct capidrv_ncci *nccip,
 	struct ncci_datahandle_queue *n, **pp;
 
 	n = kmalloc(sizeof(struct ncci_datahandle_queue), GFP_ATOMIC);
-	if (!n) {
+	if (!n)
 		return -1;
-	}
 	n->next = NULL;
 	n->datahandle = datahandle;
 	n->len = len;

@@ -511,9 +510,8 @@ static void send_message(capidrv_contr *card, _cmsg *cmsg)
 	}
 	len = CAPIMSG_LEN(cmsg->buf);
 	skb = alloc_skb(len, GFP_ATOMIC);
-	if (!skb) {
+	if (!skb)
 		return;
-	}
 	memcpy(skb_put(skb, len), cmsg->buf, len);
 	if (capi20_put_message(&global.ap, skb) != CAPI_NOERROR)
 		kfree_skb(skb);

@@ -976,13 +974,12 @@ static void handle_controller(_cmsg *cmsg)
 		if (debugmode)
 			printk(KERN_DEBUG "capidrv-%d: listenconf Info=0x%4x (%s) cipmask=0x%x\n",
 			       card->contrnr, cmsg->Info, capi_info2str(cmsg->Info), card->cipmask);
-		if (cmsg->Info) {
+		if (cmsg->Info)
 			listen_change_state(card, EV_LISTEN_CONF_ERROR);
-		} else if (card->cipmask == 0) {
+		else if (card->cipmask == 0)
 			listen_change_state(card, EV_LISTEN_CONF_EMPTY);
-		} else {
+		     else
 			listen_change_state(card, EV_LISTEN_CONF_OK);
-		}
 		break;
 
 	case CAPI_MANUFACTURER_IND:	/* Controller */

@@ -1263,11 +1260,10 @@ static void handle_plci(_cmsg *cmsg)
 			goto notfound;
 
 		plcip->plci = cmsg->adr.adrPLCI;
-		if (cmsg->Info) {
+		if (cmsg->Info)
 			plci_change_state(card, plcip, EV_PLCI_CONNECT_CONF_ERROR);
-		} else {
+		else
 			plci_change_state(card, plcip, EV_PLCI_CONNECT_CONF_OK);
-		}
 		break;
 
 	case CAPI_CONNECT_ACTIVE_IND:	/* plci */

@@ -1476,10 +1472,9 @@ static void handle_ncci(_cmsg *cmsg)
 		goto ignored;
 
 	case CAPI_DATA_B3_CONF:	/* ncci */
-		if (cmsg->Info) {
+		if (cmsg->Info)
 			printk(KERN_WARNING "CAPI_DATA_B3_CONF: Info %x - %s\n",
 			       cmsg->Info, capi_info2str(cmsg->Info));
-		}
 		nccip = find_ncci(card, cmsg->adr.adrNCCI);
 		if (!nccip)
 			goto notfound;

@@ -2319,9 +2314,8 @@ static int capidrv_addcontr(u16 contr, struct capi_profile *profp)
 	}
 	card->myid = card->interface.channels;
 	memset(card->bchans, 0, sizeof(capidrv_bchan) * card->nbchan);
-	for (i = 0; i < card->nbchan; i++) {
+	for (i = 0; i < card->nbchan; i++)
 		card->bchans[i].contr = card;
-	}
 
 	spin_lock_irqsave(&global_lock, flags);
 	card->next = global.contr_list;

@@ -2354,10 +2348,9 @@ static int capidrv_delcontr(u16 contr)
 	isdn_ctrl cmd;
 
 	spin_lock_irqsave(&global_lock, flags);
-	for (card = global.contr_list; card; card = card->next) {
+	for (card = global.contr_list; card; card = card->next)
 		if (card->contrnr == contr)
 			break;
-	}
 	if (!card) {
 		spin_unlock_irqrestore(&global_lock, flags);
 		printk(KERN_ERR "capidrv: delcontr: no contr %u\n", contr);

@@ -2504,9 +2497,8 @@ static int __init capidrv_init(void)
 
 	global.ap.recv_message = capidrv_recv_message;
 	errcode = capi20_register(&global.ap);
-	if (errcode) {
+	if (errcode)
 		return -EIO;
-	}
 
 	register_capictr_notifier(&capictr_nb);

-- 
2.10.0

Re: [PATCH 5/5] ISDN-CAPI: Delete unnecessary braces

From: Sergei Shtylyov <hidden>
Date: 2016-09-25 11:18:54

Hello.

On 9/25/2016 2:15 PM, SF Markus Elfring wrote:

quoted hunk

From: Markus Elfring <redacted>
Date: Sun, 25 Sep 2016 12:50:21 +0200

Do not use curly brackets at eight source code places
where a single statement should be sufficient.

Signed-off-by: Markus Elfring <redacted>
---
 drivers/isdn/capi/capidrv.c | 30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/drivers/isdn/capi/capidrv.c b/drivers/isdn/capi/capidrv.c
index 83f756d..7f58644 100644
--- a/drivers/isdn/capi/capidrv.c
+++ b/drivers/isdn/capi/capidrv.c

[...]

quoted hunk

@@ -976,13 +974,12 @@ static void handle_controller(_cmsg *cmsg)
 		if (debugmode)
 			printk(KERN_DEBUG "capidrv-%d: listenconf Info=0x%4x (%s) cipmask=0x%x\n",
 			       card->contrnr, cmsg->Info, capi_info2str(cmsg->Info), card->cipmask);
-		if (cmsg->Info) {
+		if (cmsg->Info)
 			listen_change_state(card, EV_LISTEN_CONF_ERROR);
-		} else if (card->cipmask == 0) {
+		else if (card->cipmask == 0)
 			listen_change_state(card, EV_LISTEN_CONF_EMPTY);
-		} else {
+		     else

   Indented too much.

[...]

MBR, Sergei

Re: [PATCH 5/5] ISDN-CAPI: Delete unnecessary braces

From: SF Markus Elfring <hidden>
Date: 2016-09-25 12:47:17

quoted

@@ -976,13 +974,12 @@ static void handle_controller(_cmsg *cmsg)
         if (debugmode)
             printk(KERN_DEBUG "capidrv-%d: listenconf Info=0x%4x (%s) cipmask=0x%x\n",
                    card->contrnr, cmsg->Info, capi_info2str(cmsg->Info), card->cipmask);
-        if (cmsg->Info) {
+        if (cmsg->Info)
             listen_change_state(card, EV_LISTEN_CONF_ERROR);
-        } else if (card->cipmask == 0) {
+        else if (card->cipmask == 0)
             listen_change_state(card, EV_LISTEN_CONF_EMPTY);
-        } else {
+             else

   Indented too much.

How do you think about an alignment of this "else"
with the corresponding if statement three lines above?

Regards,
Markus

Re: [PATCH 3/5] ISDN-CAPI: Adjust 17 function calls together with variable assignments

From: Paul Bolle <hidden>
Date: 2016-09-26 09:13:03

On Sun, 2016-09-25 at 13:13 +0200, SF Markus Elfring wrote:

The script "checkpatch.pl" can point out that assignments should usually
not be performed within condition checks.
Thus move the assignment for a variable to a separate statement
in four functions.

Did you recycle this commit explanation? Because git diff tells me you
actually touched about eight functions.

Signed-off-by: Markus Elfring <redacted>
---
 drivers/isdn/capi/capidrv.c | 59 +++++++++++++++++++++++++++------------------
 1 file changed, 36 insertions(+), 23 deletions(-)

So I ran checkpatch on this file, just like you did. Specifically, I
did:
    scripts/checkpatch.pl -f drivers/isdn/capi/capidrv.c | grep "assignment in if condition" | wc -l 

It tells me there are actually 18 instances of this "ERROR". Why did
you ignore one of it in this patch?


Paul Bolle

Re: [PATCH 5/5] ISDN-CAPI: Delete unnecessary braces

From: Paul Bolle <hidden>
Date: 2016-09-26 09:20:56

On Sun, 2016-09-25 at 14:47 +0200, SF Markus Elfring wrote:

quoted

@@ -976,13 +974,12 @@ static void handle_controller(_cmsg *cmsg)

         if (debugmode)
             printk(KERN_DEBUG "capidrv-%d: listenconf Info=0x%4x
(%s) cipmask=0x%x\n",
                    card->contrnr, cmsg->Info,
capi_info2str(cmsg->Info), card->cipmask);
-        if (cmsg->Info) {
+        if (cmsg->Info)
             listen_change_state(card, EV_LISTEN_CONF_ERROR);
-        } else if (card->cipmask == 0) {
+        else if (card->cipmask == 0)
             listen_change_state(card, EV_LISTEN_CONF_EMPTY);
-        } else {
+             else

   Indented too much.

How do you think about an alignment of this "else"
with the corresponding if statement three lines above?

Well, I think it looks silly. checkpatch apparently agrees:
    WARNING: Statements should start on a tabstop
    #51: FILE: drivers/isdn/capi/capidrv.c:981:
    +		     else
    
    total: 0 errors, 1 warnings, 91 lines checked
    
    NOTE: For some of the reported defects, checkpatch may be able to
          mechanically convert to the typical style using --fix or --fix-inplace.
    
    Your patch has style problems, please review.
    
    NOTE: If any of the errors are false positives, please report
          them to the maintainer, see CHECKPATCH in MAINTAINERS.

You use checkpatch a lot, don't you? Didn't you use it to, you know,
check your patch?


Paul Bolle

Re: ISDN-CAPI: Adjust 17 function calls together with variable assignments

From: SF Markus Elfring <hidden>
Date: 2016-09-26 12:29:55

quoted

The script "checkpatch.pl" can point out that assignments should usually
not be performed within condition checks.
Thus move the assignment for a variable to a separate statement
in four functions.

Did you recycle this commit explanation?

Yes. - I am going to use similar commit messages for further changes
in other software modules.

Because git diff tells me you actually touched about eight functions.

You are right. - I'm sorry that I overlooked to update this number somehow.

    scripts/checkpatch.pl -f drivers/isdn/capi/capidrv.c | grep "assignment in if condition" | wc -l 

It tells me there are actually 18 instances of this "ERROR".
Why did you ignore one of it in this patch?

Did I accidentally leave another update candidate over?

* How do you think about to pick such a software update opportunity up?

* Do you expect a resend for the steps 3 - 5 of this small patch series?

Regards,
Markus

Re: [PATCH 5/5] ISDN-CAPI: Delete unnecessary braces

From: SF Markus Elfring <hidden>
Date: 2016-09-26 12:52:56

quoted

@@ -976,13 +974,12 @@ static void handle_controller(_cmsg *cmsg)
         if (debugmode)
             printk(KERN_DEBUG "capidrv-%d: listenconf Info=0x%4x

(%s) cipmask=0x%x\n",
                    card->contrnr, cmsg->Info,
capi_info2str(cmsg->Info), card->cipmask);
-        if (cmsg->Info) {
+        if (cmsg->Info)
             listen_change_state(card, EV_LISTEN_CONF_ERROR);
-        } else if (card->cipmask == 0) {
+        else if (card->cipmask == 0)
             listen_change_state(card, EV_LISTEN_CONF_EMPTY);
-        } else {
+             else

   Indented too much.

How do you think about an alignment of this "else"
with the corresponding if statement three lines above?

Well, I think it looks silly.

Thanks for your feedback.

checkpatch apparently agrees:
    WARNING: Statements should start on a tabstop
    #51: FILE: drivers/isdn/capi/capidrv.c:981:
    +		     else
    
    total: 0 errors, 1 warnings, 91 lines checked

…

You use checkpatch a lot, don't you?

It seems so when I am preparing hundreds of update steps.

Didn't you use it to, you know, check your patch?

This Perl script showed me also the quoted information.

I dared to present a deviation from this general advice
because I got the mood to gather constructive comments on
source code formatting also around an if statement like
in my update suggestion.

Would it eventually make sense to move this "if" (behind an "else")
to a separate line and increase the indentation for the corresponding
code one level then?

Regards,
Markus

Re: [PATCH 5/5] ISDN-CAPI: Delete unnecessary braces

From: Paul Bolle <hidden>
Date: 2016-09-26 19:55:50

On Mon, 2016-09-26 at 14:52 +0200, SF Markus Elfring wrote:

Would it eventually make sense to move this "if" (behind an "else")
to a separate line and increase the indentation for the corresponding
code one level then?

No.


Paul Bolle

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help