* [PATCH 1/5] ISDN-CAPI: Use kmalloc_array() in capidrv_addcontr()
2016-09-25 11:10 [PATCH 0/5] ISDN-CAPI: Fine-tuning for several function implementations SF Markus Elfring
@ 2016-09-25 11:11 ` SF Markus Elfring
2016-09-25 11:12 ` [PATCH 2/5] ISDN-CAPI: Delete error messages for a failed memory allocation in four functions SF Markus Elfring
` (3 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: SF Markus Elfring @ 2016-09-25 11:11 UTC (permalink / raw)
To: netdev, Karsten Keil; +Cc: LKML, kernel-janitors, Julia Lawall
From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 2/5] ISDN-CAPI: Delete error messages for a failed memory allocation in four functions
2016-09-25 11:10 [PATCH 0/5] ISDN-CAPI: Fine-tuning for several function implementations SF Markus Elfring
2016-09-25 11:11 ` [PATCH 1/5] ISDN-CAPI: Use kmalloc_array() in capidrv_addcontr() SF Markus Elfring
@ 2016-09-25 11:12 ` SF Markus Elfring
2016-09-25 11:13 ` [PATCH 3/5] ISDN-CAPI: Adjust 17 function calls together with variable assignments SF Markus Elfring
` (2 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: SF Markus Elfring @ 2016-09-25 11:12 UTC (permalink / raw)
To: netdev, Karsten Keil; +Cc: LKML, kernel-janitors, Julia Lawall, Wolfram Sang
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 3/5] ISDN-CAPI: Adjust 17 function calls together with variable assignments
2016-09-25 11:10 [PATCH 0/5] ISDN-CAPI: Fine-tuning for several function implementations SF Markus Elfring
2016-09-25 11:11 ` [PATCH 1/5] ISDN-CAPI: Use kmalloc_array() in capidrv_addcontr() SF Markus Elfring
2016-09-25 11:12 ` [PATCH 2/5] ISDN-CAPI: Delete error messages for a failed memory allocation in four functions SF Markus Elfring
@ 2016-09-25 11:13 ` SF Markus Elfring
2016-09-26 9:12 ` Paul Bolle
2016-09-25 11:14 ` [PATCH 4/5] ISDN-CAPI: Adjust checks for null pointers in four functions SF Markus Elfring
2016-09-25 11:15 ` [PATCH 5/5] ISDN-CAPI: Delete unnecessary braces SF Markus Elfring
4 siblings, 1 reply; 13+ messages in thread
From: SF Markus Elfring @ 2016-09-25 11:13 UTC (permalink / raw)
To: netdev, Karsten Keil; +Cc: LKML, kernel-janitors, Julia Lawall
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 3/5] ISDN-CAPI: Adjust 17 function calls together with variable assignments
2016-09-25 11:13 ` [PATCH 3/5] ISDN-CAPI: Adjust 17 function calls together with variable assignments SF Markus Elfring
@ 2016-09-26 9:12 ` Paul Bolle
2016-09-26 12:28 ` SF Markus Elfring
0 siblings, 1 reply; 13+ messages in thread
From: Paul Bolle @ 2016-09-26 9:12 UTC (permalink / raw)
To: SF Markus Elfring, netdev, Karsten Keil
Cc: LKML, kernel-janitors, Julia Lawall
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: ISDN-CAPI: Adjust 17 function calls together with variable assignments
2016-09-26 9:12 ` Paul Bolle
@ 2016-09-26 12:28 ` SF Markus Elfring
0 siblings, 0 replies; 13+ messages in thread
From: SF Markus Elfring @ 2016-09-26 12:28 UTC (permalink / raw)
To: Paul Bolle; +Cc: netdev, Karsten Keil, LKML, kernel-janitors, Julia Lawall
>> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 4/5] ISDN-CAPI: Adjust checks for null pointers in four functions
2016-09-25 11:10 [PATCH 0/5] ISDN-CAPI: Fine-tuning for several function implementations SF Markus Elfring
` (2 preceding siblings ...)
2016-09-25 11:13 ` [PATCH 3/5] ISDN-CAPI: Adjust 17 function calls together with variable assignments SF Markus Elfring
@ 2016-09-25 11:14 ` SF Markus Elfring
2016-09-25 11:15 ` [PATCH 5/5] ISDN-CAPI: Delete unnecessary braces SF Markus Elfring
4 siblings, 0 replies; 13+ messages in thread
From: SF Markus Elfring @ 2016-09-25 11:14 UTC (permalink / raw)
To: netdev, Karsten Keil; +Cc: LKML, kernel-janitors, Julia Lawall
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 5/5] ISDN-CAPI: Delete unnecessary braces
2016-09-25 11:10 [PATCH 0/5] ISDN-CAPI: Fine-tuning for several function implementations SF Markus Elfring
` (3 preceding siblings ...)
2016-09-25 11:14 ` [PATCH 4/5] ISDN-CAPI: Adjust checks for null pointers in four functions SF Markus Elfring
@ 2016-09-25 11:15 ` SF Markus Elfring
2016-09-25 11:18 ` Sergei Shtylyov
4 siblings, 1 reply; 13+ messages in thread
From: SF Markus Elfring @ 2016-09-25 11:15 UTC (permalink / raw)
To: netdev, Karsten Keil; +Cc: LKML, kernel-janitors, Julia Lawall
From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 5/5] ISDN-CAPI: Delete unnecessary braces
2016-09-25 11:15 ` [PATCH 5/5] ISDN-CAPI: Delete unnecessary braces SF Markus Elfring
@ 2016-09-25 11:18 ` Sergei Shtylyov
2016-09-25 12:47 ` SF Markus Elfring
0 siblings, 1 reply; 13+ messages in thread
From: Sergei Shtylyov @ 2016-09-25 11:18 UTC (permalink / raw)
To: SF Markus Elfring, netdev, Karsten Keil
Cc: LKML, kernel-janitors, Julia Lawall
Hello.
On 9/25/2016 2:15 PM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> 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 <elfring@users.sourceforge.net>
> ---
> 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
[...]
> @@ -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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 5/5] ISDN-CAPI: Delete unnecessary braces
2016-09-25 11:18 ` Sergei Shtylyov
@ 2016-09-25 12:47 ` SF Markus Elfring
2016-09-26 9:20 ` Paul Bolle
0 siblings, 1 reply; 13+ messages in thread
From: SF Markus Elfring @ 2016-09-25 12:47 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: netdev, Karsten Keil, LKML, kernel-janitors, Julia Lawall
>> @@ -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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 5/5] ISDN-CAPI: Delete unnecessary braces
2016-09-25 12:47 ` SF Markus Elfring
@ 2016-09-26 9:20 ` Paul Bolle
2016-09-26 12:52 ` SF Markus Elfring
0 siblings, 1 reply; 13+ messages in thread
From: Paul Bolle @ 2016-09-26 9:20 UTC (permalink / raw)
To: SF Markus Elfring, Sergei Shtylyov
Cc: netdev, Karsten Keil, LKML, kernel-janitors, Julia Lawall
On Sun, 2016-09-25 at 14:47 +0200, SF Markus Elfring wrote:
> > > @@ -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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 5/5] ISDN-CAPI: Delete unnecessary braces
2016-09-26 9:20 ` Paul Bolle
@ 2016-09-26 12:52 ` SF Markus Elfring
2016-09-26 19:55 ` Paul Bolle
0 siblings, 1 reply; 13+ messages in thread
From: SF Markus Elfring @ 2016-09-26 12:52 UTC (permalink / raw)
To: Paul Bolle
Cc: Sergei Shtylyov, netdev, Karsten Keil, LKML, kernel-janitors,
Julia Lawall
>>>> @@ -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
^ permalink raw reply [flat|nested] 13+ messages in thread