by Alan

[permalink] [raw]

Subject: Re: [PATCH 6/9] ser_gigaset: checkpatch cleanup

On Sun, 25 Oct 2009 20:30:27 +0100 (CET)
Tilman Schmidt <[email protected]> wrote:

> Duly uglified as demanded by checkpatch.pl.
>
> Impact: cosmeti

Umm ??

> - if (!(bcs->tx_skb = skb_dequeue(&bcs->squeue)))
> + nextskb = skb_dequeue(&bcs->squeue);
> + if (!nextskb)
> /* no skb either, nothing to do */
> return;
> + bcs->tx_skb = nextskb;

This does not do the same thing as before

Previously the NULL case assigned to bcs->tx_skb, now it does not.

2009-10-25 23:37:08

Am 26.10.2009 01:54 schrieb Joe Perches:
> On Sun, 2009-10-25 at 20:30 +0100, Tilman Schmidt wrote:
>> Duly uglified as demanded by checkpatch.pl.
>> diff --git a/drivers/isdn/gigaset/ser-gigaset.c b/drivers/isdn/gigaset/ser-gigaset.c
>> index 3071a52..ac3409e 100644
>> --- a/drivers/isdn/gigaset/ser-gigaset.c
>> +++ b/drivers/isdn/gigaset/ser-gigaset.c
>> @@ -164,9 +164,15 @@ static void gigaset_modem_fill(unsigned long data)
>> {
>> struct cardstate *cs = (struct cardstate *) data;
>> struct bc_state *bcs;
>> + struct sk_buff *nextskb;
>> int sent = 0;
>>
>> - if (!cs || !(bcs = cs->bcs)) {
>> + if (!cs) {
>> + gig_dbg(DEBUG_OUTPUT, "%s: no cardstate", __func__);
>> + return;
>> + }
>> + bcs = cs->bcs;
>> + if (!bcs) {
>> gig_dbg(DEBUG_OUTPUT, "%s: no cardstate", __func__);
>> return;
>> }
>
> perhaps:
> if (!cs || !cs->bcs) {
> gig_dbg(DEBUG_OUTPUT, "%s: no cardstate", __func__);
> return;
> }
> bcs = cs->bcs;

That would evaluate cs->bcs twice, and is also, in my experience,
significantly more prone to easily overlooked typos which result in
checking a different pointer in the if statement than the one that's
actually used in the subsequent assignment.

>> @@ -404,16 +412,20 @@ static void gigaset_device_release(struct device *dev)
>> static int gigaset_initcshw(struct cardstate *cs)
>> {
>> int rc;
>> + struct ser_cardstate *scs;
>>
>> - if (!(cs->hw.ser = kzalloc(sizeof(struct ser_cardstate), GFP_KERNEL))) {
>> + scs = kzalloc(sizeof(struct ser_cardstate), GFP_KERNEL);
>> + if (!scs) {
>> pr_err("out of memory\n");
>> return 0;
>> }
>> + cs->hw.ser = scs;
>
> Why not no temporary and just:
>
> cs->hw.ser = kzalloc...
> if (!cs->hw.ser)

For the same reasons as above.

Thanks,
Tilman

--
Tilman Schmidt E-Mail: [email protected]
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)

Attachments:

signature.asc (254.00 B)
OpenPGP digital signature

2009-10-27 00:14:52

by Joe Perches

[permalink] [raw]

Subject: Re: [PATCH 6/9] ser_gigaset: checkpatch cleanup

On Tue, 2009-10-27 at 00:59 +0100, Tilman Schmidt wrote:
> Am 26.10.2009 01:54 schrieb Joe Perches:
> > On Sun, 2009-10-25 at 20:30 +0100, Tilman Schmidt wrote:
> >> Duly uglified as demanded by checkpatch.pl.
> >> diff --git a/drivers/isdn/gigaset/ser-gigaset.c b/drivers/isdn/gigaset/ser-gigaset.c
> >> index 3071a52..ac3409e 100644
> >> --- a/drivers/isdn/gigaset/ser-gigaset.c
> >> +++ b/drivers/isdn/gigaset/ser-gigaset.c
> >> @@ -164,9 +164,15 @@ static void gigaset_modem_fill(unsigned long data)
> >> {
> >> struct cardstate *cs = (struct cardstate *) data;
> >> struct bc_state *bcs;
> >> + struct sk_buff *nextskb;
> >> int sent = 0;
> >>
> >> - if (!cs || !(bcs = cs->bcs)) {
> >> + if (!cs) {
> >> + gig_dbg(DEBUG_OUTPUT, "%s: no cardstate", __func__);
> >> + return;
> >> + }
> >> + bcs = cs->bcs;
> >> + if (!bcs) {
> >> gig_dbg(DEBUG_OUTPUT, "%s: no cardstate", __func__);
> >> return;
> >> }
> >
> > perhaps:
> > if (!cs || !cs->bcs) {
> > gig_dbg(DEBUG_OUTPUT, "%s: no cardstate", __func__);
> > return;
> > }
> > bcs = cs->bcs;
>
> That would evaluate cs->bcs twice, and is also, in my experience,
> significantly more prone to easily overlooked typos which result in
> checking a different pointer in the if statement than the one that's
> actually used in the subsequent assignment.

The other is to duplicate the gig_dbg function as you've done.
Also prone to typos and more code as well.

> >> @@ -404,16 +412,20 @@ static void gigaset_device_release(struct device *dev)
> >> static int gigaset_initcshw(struct cardstate *cs)
> >> {
> >> int rc;
> >> + struct ser_cardstate *scs;
> >>
> >> - if (!(cs->hw.ser = kzalloc(sizeof(struct ser_cardstate), GFP_KERNEL))) {
> >> + scs = kzalloc(sizeof(struct ser_cardstate), GFP_KERNEL);
> >> + if (!scs) {
> >> pr_err("out of memory\n");
> >> return 0;
> >> }
> >> + cs->hw.ser = scs;
> >
> > Why not no temporary and just:
> >
> > cs->hw.ser = kzalloc...
> > if (!cs->hw.ser)
>
> For the same reasons as above.

I believe the checkpatch recommended form is:

foo = func();
if ([!]foo) {
handle_error()...
}

as you've used in all the other conversions.

No big deal or difference, but I think what I
suggested is more kernel style normal.

cheers, Joe

2009-10-27 11:35:31

by Karsten Keil

[permalink] [raw]

Subject: Re: [PATCH 6/9] ser_gigaset: checkpatch cleanup

On Dienstag, 27. Oktober 2009 00:59:20 Tilman Schmidt wrote:
> Am 26.10.2009 01:54 schrieb Joe Perches:
> > On Sun, 2009-10-25 at 20:30 +0100, Tilman Schmidt wrote:
> >> Duly uglified as demanded by checkpatch.pl.
> >> diff --git a/drivers/isdn/gigaset/ser-gigaset.c
> >> b/drivers/isdn/gigaset/ser-gigaset.c index 3071a52..ac3409e 100644
> >> --- a/drivers/isdn/gigaset/ser-gigaset.c
> >> +++ b/drivers/isdn/gigaset/ser-gigaset.c
> >> @@ -164,9 +164,15 @@ static void gigaset_modem_fill(unsigned long data)
> >> {
> >> struct cardstate *cs = (struct cardstate *) data;
> >> struct bc_state *bcs;
> >> + struct sk_buff *nextskb;
> >> int sent = 0;
> >>
> >> - if (!cs || !(bcs = cs->bcs)) {
> >> + if (!cs) {
> >> + gig_dbg(DEBUG_OUTPUT, "%s: no cardstate", __func__);
> >> + return;
> >> + }
> >> + bcs = cs->bcs;
> >> + if (!bcs) {
> >> gig_dbg(DEBUG_OUTPUT, "%s: no cardstate", __func__);
> >> return;
> >> }
> >
> > perhaps:
> > if (!cs || !cs->bcs) {
> > gig_dbg(DEBUG_OUTPUT, "%s: no cardstate", __func__);
> > return;
> > }
> > bcs = cs->bcs;
>
> That would evaluate cs->bcs twice, and is also, in my experience,

gcc should handle this subsequent double evaluation well enough.

> significantly more prone to easily overlooked typos which result in
> checking a different pointer in the if statement than the one that's
> actually used in the subsequent assignment.
>

Yes this may happen, but more often a = in if statements should be a ==.

The kernel code style says only one statement per line, which implies no
assignments in if statements, so we should follow this.
The checkpatch.pl script complain about these issues.

Yes sometimes in the past (while preparing some old code for kernel submit)
I was not very happy about all these rules, it take lot time to reach zero
reports from checkpatch.
But it make lot of sense in the long term, currently I have to debug code
written without any code style, it is really, really painful and I need 5-10
times more time for simple understanding the basic function of the code.

Karsten

2009-10-29 08:37:16

by David Miller

[permalink] [raw]

Subject: Re: [PATCH 0/9] Gigaset driver patches for 2.6.33

From: Tilman Schmidt <[email protected]>
Date: Sun, 25 Oct 2009 20:29:27 +0100 (CET)

> David, Karsten,
>
> here's a series of patches that should be applied on top of those
> currently in net-next. I'm sending them now to leave you enough time
> before the next merge window to decide who wants to handle them.
> In particular, patch 3 provides an important bugfix to the new CAPI
> interface which is necessary for using it with pppd capiplugin.

All of these patches look reasonable, so I've applied them
to net-next-2.6

Thanks!