Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1490874 > unrolled thread

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

Started bySF Markus Elfring <elfring@users.sourceforge.net>
First post2016-09-25 13:20 +0200
Last post2016-09-25 13:20 +0200
Articles 6 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/5] ISDN-CAPI: Fine-tuning for several function  implementations SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-25 13:20 +0200
    [PATCH 3/5] ISDN-CAPI: Adjust 17 function calls together with  variable assignments SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-25 13:20 +0200
      Re: [PATCH 3/5] ISDN-CAPI: Adjust 17 function calls together with  variable assignments Paul Bolle <pebolle@tiscali.nl> - 2016-09-26 11:20 +0200
        Re: ISDN-CAPI: Adjust 17 function calls together with variable  assignments SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-26 14:40 +0200
    [PATCH 4/5] ISDN-CAPI: Adjust checks for null pointers in four  functions SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-25 13:20 +0200
    [PATCH 2/5] ISDN-CAPI: Delete error messages for a failed memory  allocation in four functions SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-25 13:20 +0200

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

FromSF Markus Elfring <elfring@users.sourceforge.net>
Date2016-09-25 13:20 +0200
Subject[PATCH 0/5] ISDN-CAPI: Fine-tuning for several function implementations
Message-ID<slfDr-5Uj-5@gated-at.bofh.it>
From: Markus Elfring <elfring@users.sourceforge.net>
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

[toc] | [next] | [standalone]

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

FromSF Markus Elfring <elfring@users.sourceforge.net>
Date2016-09-25 13:20 +0200
Subject[PATCH 3/5] ISDN-CAPI: Adjust 17 function calls together with variable assignments
Message-ID<slfDs-5Uj-17@gated-at.bofh.it>
In reply to#1490874
From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

[toc] | [prev] | [next] | [standalone]

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

FromPaul Bolle <pebolle@tiscali.nl>
Date2016-09-26 11:20 +0200
SubjectRe: [PATCH 3/5] ISDN-CAPI: Adjust 17 function calls together with variable assignments
Message-ID<slAeR-20r-7@gated-at.bofh.it>
In reply to#1490875
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 <elfring@users.sourceforge.net>
> ---
>  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

[toc] | [prev] | [next] | [standalone]

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

FromSF Markus Elfring <elfring@users.sourceforge.net>
Date2016-09-26 14:40 +0200
SubjectRe: ISDN-CAPI: Adjust 17 function calls together with variable assignments
Message-ID<slDmp-3Pp-23@gated-at.bofh.it>
In reply to#1491173
>> 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

[toc] | [prev] | [next] | [standalone]

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

FromSF Markus Elfring <elfring@users.sourceforge.net>
Date2016-09-25 13:20 +0200
Subject[PATCH 4/5] ISDN-CAPI: Adjust checks for null pointers in four functions
Message-ID<slfDs-5Uj-25@gated-at.bofh.it>
In reply to#1490874
From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

[toc] | [prev] | [next] | [standalone]

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

FromSF Markus Elfring <elfring@users.sourceforge.net>
Date2016-09-25 13:20 +0200
Subject[PATCH 2/5] ISDN-CAPI: Delete error messages for a failed memory allocation in four functions
Message-ID<slfDs-5Uj-19@gated-at.bofh.it>
In reply to#1490874
From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web