[PATCH 0/6] checkpatch checks for trailing semicolons on conditionals

[PATCH 0/6] checkpatch checks for trailing semicolons on conditionals

[Andy Whitcroft]

A couple of people suggested adding checks to checkpatch for trailing
semicolons on conditionals, where the conditional block may not be
actually conditional:

	if (err);
		return err;

While regression testing the changes, I ran these checks across the
whole of 2.6.23-rc3 and there appear to be 5 places where this is
occurs (above and beyond the IPv6 one which triggered this effort)
and a benign use which could be confused later which it seems safest
to fix.

Following this email are 6 patches for these issues, relevant
maintainers cc'd.  All against 2.6.23-rc3

-apw

drivers/infiniband/mlx/mad.c misplaced ;

[Dave Jones]

Signed-off-by: Dave Jones <davej@redhat.com>

diff --git a/drivers/infiniband/hw/mlx4/mad.c b/drivers/infiniband/hw/mlx4/mad.c
index 3330917..0ed02b7 100644
--- a/drivers/infiniband/hw/mlx4/mad.c
+++ b/drivers/infiniband/hw/mlx4/mad.c
@@ -109,7 +109,7 @@ int mlx4_MAD_IFC(struct mlx4_ib_dev *dev, int ignore_mkey, int ignore_bkey,
 			   in_modifier, op_modifier,
 			   MLX4_CMD_MAD_IFC, MLX4_CMD_TIME_CLASS_C);
 
-	if (!err);
+	if (!err)
 		memcpy(response_mad, outmailbox->buf, 256);
 
 	mlx4_free_cmd_mailbox(dev->dev, inmailbox);

-- 
http://www.codemonkey.org.uk

Re: drivers/infiniband/mlx/mad.c misplaced ;

[Joe Perches]

On Wed, 2007-08-15 at 19:58 -0400, Dave Jones wrote:
> Signed-off-by: Dave Jones <davej@redhat.com>
> 
> diff --git a/drivers/infiniband/hw/mlx4/mad.c b/drivers/infiniband/hw/mlx4/mad.c
> index 3330917..0ed02b7 100644
> --- a/drivers/infiniband/hw/mlx4/mad.c
> +++ b/drivers/infiniband/hw/mlx4/mad.c
> @@ -109,7 +109,7 @@ int mlx4_MAD_IFC(struct mlx4_ib_dev *dev, int ignore_mkey, int ignore_bkey,
>  			   in_modifier, op_modifier,
>  			   MLX4_CMD_MAD_IFC, MLX4_CMD_TIME_CLASS_C);
>  
> -	if (!err);
> +	if (!err)

There's more than a few of these (not inspected).

$ egrep -r --include=*.c "\bif[[:space:]]*\([^\)]*\)[[:space:]]*\;" * 
arch/sh/boards/se/7343/io.c:    if (0) ;
drivers/atm/iphase.c:     if (!desc1) ;
drivers/infiniband/hw/mlx4/mad.c:       if (!err);
drivers/isdn/capi/capiutil.c:   else if (c <= 0x0f);
drivers/net/tokenring/ibmtr.c:  else if (ti->shared_ram_paging == 0xf);  /* No paging in adapter */
drivers/s390/scsi/zfcp_erp.c:           if (status == ZFCP_ERP_SUCCEEDED) ;     /* no further action */
fs/hostfs/hostfs_user.c:        if(attrs->ia_valid & HOSTFS_ATTR_CTIME) ;
net/netfilter/xt_u32.c:         if (skb->len < 4 || pos > skb->len - 4);
sound/pci/au88x0/au88x0_synth.c:                                if (eax == 0) ;

Re: drivers/infiniband/mlx/mad.c misplaced ;

[Kok, Auke]

Joe Perches wrote:
> On Wed, 2007-08-15 at 19:58 -0400, Dave Jones wrote:
>> Signed-off-by: Dave Jones <davej@redhat.com>
>>
>> diff --git a/drivers/infiniband/hw/mlx4/mad.c b/drivers/infiniband/hw/mlx4/mad.c
>> index 3330917..0ed02b7 100644
>> --- a/drivers/infiniband/hw/mlx4/mad.c
>> +++ b/drivers/infiniband/hw/mlx4/mad.c
>> @@ -109,7 +109,7 @@ int mlx4_MAD_IFC(struct mlx4_ib_dev *dev, int ignore_mkey, int ignore_bkey,
>>  			   in_modifier, op_modifier,
>>  			   MLX4_CMD_MAD_IFC, MLX4_CMD_TIME_CLASS_C);
>>  
>> -	if (!err);
>> +	if (!err)
> 
> There's more than a few of these (not inspected).
> 
> $ egrep -r --include=*.c "\bif[[:space:]]*\([^\)]*\)[[:space:]]*\;" * 
> arch/sh/boards/se/7343/io.c:    if (0) ;
> drivers/atm/iphase.c:     if (!desc1) ;
> drivers/infiniband/hw/mlx4/mad.c:       if (!err);
> drivers/isdn/capi/capiutil.c:   else if (c <= 0x0f);
> drivers/net/tokenring/ibmtr.c:  else if (ti->shared_ram_paging == 0xf);  /* No paging in adapter */
> drivers/s390/scsi/zfcp_erp.c:           if (status == ZFCP_ERP_SUCCEEDED) ;     /* no further action */
> fs/hostfs/hostfs_user.c:        if(attrs->ia_valid & HOSTFS_ATTR_CTIME) ;
> net/netfilter/xt_u32.c:         if (skb->len < 4 || pos > skb->len - 4);
> sound/pci/au88x0/au88x0_synth.c:                                if (eax == 0) ;

sounds like an excellent candidate check for checkpatch.pl !!!

Cheers,

Auke

Re: drivers/infiniband/mlx/mad.c misplaced ;

[Joe Perches]

On Wed, 2007-08-15 at 19:19 -0700, Kok, Auke wrote:
> Joe Perches wrote:
> > On Wed, 2007-08-15 at 19:58 -0400, Dave Jones wrote:
> > There's more than a few of these (not inspected).
> > $ egrep -r --include=*.c "\bif[[:space:]]*\([^\)]*\)[[:space:]]*\;" * 
> > arch/sh/boards/se/7343/io.c:    if (0) ;
> > drivers/atm/iphase.c:     if (!desc1) ;
> > drivers/infiniband/hw/mlx4/mad.c:       if (!err);
> > drivers/isdn/capi/capiutil.c:   else if (c <= 0x0f);
> > drivers/net/tokenring/ibmtr.c:  else if (ti->shared_ram_paging == 0xf);  /* No paging in adapter */
> > drivers/s390/scsi/zfcp_erp.c:           if (status == ZFCP_ERP_SUCCEEDED) ;     /* no further action */
> > fs/hostfs/hostfs_user.c:        if(attrs->ia_valid & HOSTFS_ATTR_CTIME) ;
> > net/netfilter/xt_u32.c:         if (skb->len < 4 || pos > skb->len - 4);
> > sound/pci/au88x0/au88x0_synth.c:                                if (eax == 0) ;
> 
> sounds like an excellent candidate check for checkpatch.pl !!!

I think it's poor style and all of these should go away.

the netfilter one appears to be a real error too.

Signed-off-by: Joe Perches <joe@perches.com>

diff --git a/net/netfilter/xt_u32.c b/net/netfilter/xt_u32.c
index 74f9b14..bec4279 100644
--- a/net/netfilter/xt_u32.c
+++ b/net/netfilter/xt_u32.c
@@ -36,7 +36,7 @@ static bool u32_match_it(const struct xt_u32 *data,
 		at  = 0;
 		pos = ct->location[0].number;
 
-		if (skb->len < 4 || pos > skb->len - 4);
+		if (skb->len < 4 || pos > skb->len - 4)
 			return false;
 
 		ret   = skb_copy_bits(skb, pos, &n, sizeof(n));

Re: drivers/infiniband/mlx/mad.c misplaced ;

[Kok, Auke]

Joe Perches wrote:
> On Wed, 2007-08-15 at 19:19 -0700, Kok, Auke wrote:
>> Joe Perches wrote:
>>> On Wed, 2007-08-15 at 19:58 -0400, Dave Jones wrote:
>>> There's more than a few of these (not inspected).
>>> $ egrep -r --include=*.c "\bif[[:space:]]*\([^\)]*\)[[:space:]]*\;" * 
>>> arch/sh/boards/se/7343/io.c:    if (0) ;
>>> drivers/atm/iphase.c:     if (!desc1) ;
>>> drivers/infiniband/hw/mlx4/mad.c:       if (!err);
>>> drivers/isdn/capi/capiutil.c:   else if (c <= 0x0f);
>>> drivers/net/tokenring/ibmtr.c:  else if (ti->shared_ram_paging == 0xf);  /* No paging in adapter */
>>> drivers/s390/scsi/zfcp_erp.c:           if (status == ZFCP_ERP_SUCCEEDED) ;     /* no further action */
>>> fs/hostfs/hostfs_user.c:        if(attrs->ia_valid & HOSTFS_ATTR_CTIME) ;
>>> net/netfilter/xt_u32.c:         if (skb->len < 4 || pos > skb->len - 4);
>>> sound/pci/au88x0/au88x0_synth.c:                                if (eax == 0) ;
>> sounds like an excellent candidate check for checkpatch.pl !!!
> 
> I think it's poor style and all of these should go away.
> 
> the netfilter one appears to be a real error too.

I was more thinking of making sure that none of these slip back in...

Auke

Re: [netfilter-core] Re: drivers/infiniband/mlx/mad.c misplaced ;

[Patrick McHardy]

Joe Perches wrote:
> diff --git a/net/netfilter/xt_u32.c b/net/netfilter/xt_u32.c
> index 74f9b14..bec4279 100644
> --- a/net/netfilter/xt_u32.c
> +++ b/net/netfilter/xt_u32.c
> @@ -36,7 +36,7 @@ static bool u32_match_it(const struct xt_u32 *data,
>  		at  = 0;
>  		pos = ct->location[0].number;
>  
> -		if (skb->len < 4 || pos > skb->len - 4);
> +		if (skb->len < 4 || pos > skb->len - 4)
>  			return false;
>  

Thanks, I already sent the same patch upstream one or two days ago.

Re: drivers/infiniband/mlx/mad.c misplaced ;

[Andy Whitcroft]

Kok, Auke wrote:
> Joe Perches wrote:
>> On Wed, 2007-08-15 at 19:58 -0400, Dave Jones wrote:
>>> Signed-off-by: Dave Jones <davej@redhat.com>
>>>
>>> diff --git a/drivers/infiniband/hw/mlx4/mad.c
>>> b/drivers/infiniband/hw/mlx4/mad.c
>>> index 3330917..0ed02b7 100644
>>> --- a/drivers/infiniband/hw/mlx4/mad.c
>>> +++ b/drivers/infiniband/hw/mlx4/mad.c
>>> @@ -109,7 +109,7 @@ int mlx4_MAD_IFC(struct mlx4_ib_dev *dev, int
>>> ignore_mkey, int ignore_bkey,
>>>                 in_modifier, op_modifier,
>>>                 MLX4_CMD_MAD_IFC, MLX4_CMD_TIME_CLASS_C);
>>>  
>>> -    if (!err);
>>> +    if (!err)
>>
>> There's more than a few of these (not inspected).
>>
>> $ egrep -r --include=*.c "\bif[[:space:]]*\([^\)]*\)[[:space:]]*\;" *
>> arch/sh/boards/se/7343/io.c:    if (0) ;
>> drivers/atm/iphase.c:     if (!desc1) ;
>> drivers/infiniband/hw/mlx4/mad.c:       if (!err);
>> drivers/isdn/capi/capiutil.c:   else if (c <= 0x0f);
>> drivers/net/tokenring/ibmtr.c:  else if (ti->shared_ram_paging ==
>> 0xf);  /* No paging in adapter */
>> drivers/s390/scsi/zfcp_erp.c:           if (status ==
>> ZFCP_ERP_SUCCEEDED) ;     /* no further action */
>> fs/hostfs/hostfs_user.c:        if(attrs->ia_valid & HOSTFS_ATTR_CTIME) ;
>> net/netfilter/xt_u32.c:         if (skb->len < 4 || pos > skb->len - 4);
>> sound/pci/au88x0/au88x0_synth.c:                                if
>> (eax == 0) ;
> 
> sounds like an excellent candidate check for checkpatch.pl !!!

Yep.  My scan of 2.6.23-rc3 with a checkpatch with this new test added,
found 6 which seemed wrong.

-apw

Re: drivers/infiniband/mlx/mad.c misplaced ;

[Heiko Carstens]

On Wed, Aug 15, 2007 at 05:40:11PM -0700, Joe Perches wrote:
> On Wed, 2007-08-15 at 19:58 -0400, Dave Jones wrote:
> > Signed-off-by: Dave Jones <davej@redhat.com>
> > 
> > diff --git a/drivers/infiniband/hw/mlx4/mad.c b/drivers/infiniband/hw/mlx4/mad.c
> > index 3330917..0ed02b7 100644
> > --- a/drivers/infiniband/hw/mlx4/mad.c
> > +++ b/drivers/infiniband/hw/mlx4/mad.c
> > @@ -109,7 +109,7 @@ int mlx4_MAD_IFC(struct mlx4_ib_dev *dev, int ignore_mkey, int ignore_bkey,
> >  			   in_modifier, op_modifier,
> >  			   MLX4_CMD_MAD_IFC, MLX4_CMD_TIME_CLASS_C);
> >  
> > -	if (!err);
> > +	if (!err)
> 
> There's more than a few of these (not inspected).
> 
> $ egrep -r --include=*.c "\bif[[:space:]]*\([^\)]*\)[[:space:]]*\;" * 

[...]

> drivers/s390/scsi/zfcp_erp.c:           if (status == ZFCP_ERP_SUCCEEDED) ;     /* no further action */

At least this one is not a bug. But I'm going to add a "break" there, so it
doesn't look that strange anymore. Thanks!

Re: drivers/infiniband/mlx/mad.c misplaced ;

[Ilpo Järvinen]

...I guess those guys hunting for broken busyloops in the other thread 
could also benefit from similar searching commands introduced in this 
thread... ...Ccing Satyam to caught their attention too.


> On Wed, Aug 15, 2007 at 05:40:11PM -0700, Joe Perches wrote:
> > 
> > There's more than a few of these (not inspected).
> > 
> > $ egrep -r --include=*.c "\bif[[:space:]]*\([^\)]*\)[[:space:]]*\;" * 

...Hmm, I plugged in "a preprocessor" too to manage with non compliant 
coding styles :-). Please understand that the line numbers are not an 
exact match due to preprocessor changes:

$ for i in `find . -name '*.[ch]'`; do echo $i; indent -npro -kr -i8 -ts8 
-sob -l8000 -ss -ncs -cp1 -nhnl -st $i | egrep -n "[[:space:]]if [(].*[)] ;$";
done | grep -B1 "^[^.]"

./arch/arm/mach-omap1/leds-innovator.c
97:	if (led_state & LED_STATE_ENABLED) ;
--
./arch/mips/sibyte/cfe/console.c
23:				if (written < 0) ;
32:			if (written < 0) ;
--
./arch/powerpc/kernel/legacy_serial.c
524:	if (0) ;
--
./arch/powerpc/xmon/ppc-opc.c
938:	else if (value == 0) ;
--
./arch/sh/boards/se/7343/io.c
137:	if (0) ;
--
./arch/um/kernel/tt/tracer.c
254:		if (WIFEXITED(status)) ;
--
./arch/x86_64/ia32/ptrace32.c
363:			if (__copy_from_user(&child->thread.i387.fxsave, u, sizeof(*u))) ;
--
./arch/x86_64/kernel/traps.c
801:	if (eregs == (struct pt_regs *)eregs->rsp) ;
--
./drivers/atm/iphase.c
159:		if (!desc1) ;
--
./drivers/isdn/capi/capiutil.c
456:	else if (c <= 0x0f) ;
--
./drivers/isdn/hisax/hfc_pci.c
125:	if (Read_hfc(cs, HFCPCI_INT_S1)) ;
155:	if (Read_hfc(cs, HFCPCI_INT_S1)) ;
1483:					if (Read_hfc(cs, HFCPCI_INT_S1)) ;
--
./drivers/isdn/hisax/hfc_sx.c
377:	if (Read_hfc(cs, HFCSX_INT_S1)) ;
407:	if (Read_hfc(cs, HFCSX_INT_S2)) ;
1246:					if (Read_hfc(cs, HFCSX_INT_S1)) ;
--
./drivers/media/video/video-buf.c
1141:			if (q->bufs[i]) ;
--
./drivers/net/lp486e.c
777:	if (lp->scb.command && i596_timeout(dev, "i596_cleanup_cmd", 100)) ;
785:	if (lp->scb.command && i596_timeout(dev, "i596_reset", 100)) ;
794:	if (lp->scb.command && i596_timeout(dev, "i596_reset(2)", 400)) ;
820:		if (lp->scb.command && i596_timeout(dev, "i596_add_cmd", 100)) ;
1146:	if (lp->scb.command && i596_timeout(dev, "interrupt", 40)) ;
1192:	if (lp->scb.command && i596_timeout(dev, "i596 interrupt", 100)) ;
1217:	if (lp->scb.command && i596_timeout(dev, "i596_close", 200)) ;
--
./drivers/net/ni5010.c
273:	if (dev->irq == 0xff) ;
--
./drivers/net/ni52.c
648:		if (result & TDR_LNK_OK) ;
--
./drivers/net/sun3_82586.c
498:		if (result & TDR_LNK_OK) ;
--
./drivers/pci/hotplug/ibmphp_core.c
418:					else if (mode == BUS_MODE_PCI) ;
636:		else if (mode == BUS_MODE_PCI) ;
--
./drivers/usb/gadget/file_storage.c
2480:	if (protocol_is_scsi()) ;
--
./drivers/usb/host/uhci-debug.c
416:		if (i <= SKEL_ISO) ;
419:		else if (!uhci->fsbr_is_on) ;
--
./drivers/usb/host/uhci-q.c
541:	if (qh->skel == SKEL_ISO) ;
--
./drivers/usb/misc/usbtest.c
1401:	if (status != 0) ;
--
./drivers/video/intelfb/intelfbdrv.c
337:		if (get_opt_bool(this_opt, "accel", &accel)) ;
338:		else if (get_opt_int(this_opt, "vram", &vram)) ;
339:		else if (get_opt_bool(this_opt, "hwcursor", &hwcursor)) ;
340:		else if (get_opt_bool(this_opt, "mtrr", &mtrr)) ;
341:		else if (get_opt_bool(this_opt, "fixed", &fixed)) ;
--
./drivers/video/matrox/matroxfb_DAC1064.c
46:	if (fvco <= 100000) ;
--
./drivers/video/matrox/matroxfb_maven.c
298:	if (fvco <= 100000000) ;
316:	if (fvco <= 100000) ;
--
./fs/hfs/inode.c
72:		if (!node) ;
--
./fs/hfsplus/inode.c
67:		if (!node) ;
--
./fs/hostfs/hostfs_user.c
300:	if (attrs->ia_valid & HOSTFS_ATTR_CTIME) ;
--
./fs/xfs/xfs_bmap.c
2287:	if (nullfb || XFS_FSB_TO_AGNO(mp, ap->rval) == fb_agno) ;
--
./fs/xfs/xfs_dir2.c
281:	else if ((rval = xfs_dir2_isblock(tp, dp, &v))) ;
--
./fs/xfs/xfs_iomap.c
248:	if (io->io_flags & XFS_IOCORE_RT) ;
--
./include/asm-cris/uaccess.h
255:	if (n == 0) ;
303:	if (n == 0) ;
351:	if (n == 0) ;
--
./mm/swapfile.c
791:				if (swcount <= 1) ;
--
./net/core/pktgen.c
2256:		if (pkt_dev->min_in6_daddr.s6_addr32[0] == 0 && pkt_dev->min_in6_daddr.s6_addr32[1] == 0 && pkt_dev->min_in6_daddr.s6_addr32[2] == 0 && pkt_dev->min_in6_daddr.s6_addr32[3] == 0) ;
--
./net/irda/af_irda.c
1357:			if (ret) ;
1358:			else if (sk->sk_shutdown & RCV_SHUTDOWN) ;
--
./net/irda/irnetlink.c
105:	if (nla_put_string(msg, IRDA_NL_ATTR_IFNAME, dev->name)) ;
--
./net/netfilter/xt_u32.c
38:		if (skb->len < 4 || pos > skb->len - 4) ;
--
./sound/pci/au88x0/au88x0_core.c
2076:				if (vortex_adbdma_bufshift(vortex, i)) ;
2085:				if (vortex_wtdma_bufshift(vortex, i)) ;
--
./sound/pci/au88x0/au88x0_synth.c
352:				if (eax == 0) ;
--
./sound/pci/ice1712/ice1724.c
596:	if (!ptr) ;
636:	if (!ptr) ;
--
./sound/usb/usbmixer.c
1296:		if (check_mapped_name(state, unitid, cval->control, kctl->id.name, sizeof(kctl->id.name))) ;
1500:	if (len) ;


...some of these are false positives due to constructs like this (not 
sure if there's some better alternative):

if (!ptr)
	;
else if (ptr->something)
	do_it();
else
	do_other();

...plus there might be some #ifdefs in that construct too.


-- 
 i.

Re: drivers/infiniband/mlx/mad.c misplaced ;

[Karsten Keil]

On Thu, Aug 16, 2007 at 01:22:04PM +0300, Ilpo Järvinen wrote:
> 
> ...I guess those guys hunting for broken busyloops in the other thread 
> could also benefit from similar searching commands introduced in this 
> thread... ...Ccing Satyam to caught their attention too.
> 
> 
> ./drivers/isdn/hisax/hfc_pci.c
> 125:	if (Read_hfc(cs, HFCPCI_INT_S1)) ;
> 155:	if (Read_hfc(cs, HFCPCI_INT_S1)) ;
> 1483:					if (Read_hfc(cs, HFCPCI_INT_S1)) ;
> --
> ./drivers/isdn/hisax/hfc_sx.c
> 377:	if (Read_hfc(cs, HFCSX_INT_S1)) ;
> 407:	if (Read_hfc(cs, HFCSX_INT_S2)) ;
> 1246:					if (Read_hfc(cs, HFCSX_INT_S1)) ;
> --

These are workaround to not get compiler warnings about ignored return
values I got some time ago under some architecture.


-- 
Karsten Keil
SuSE Labs
ISDN and VOIP development
SUSE LINUX Products GmbH, Maxfeldstr.5 90409 Nuernberg, GF: Markus Rex, HRB 16746 (AG Nuernberg)

Re: drivers/infiniband/mlx/mad.c misplaced ;

[Daniel Schaffrath]

On 2007/08/16  , at 13:01, Karsten Keil wrote:

> On Thu, Aug 16, 2007 at 01:22:04PM +0300, Ilpo Järvinen wrote:
>>
>> ...I guess those guys hunting for broken busyloops in the other  
>> thread
>> could also benefit from similar searching commands introduced in this
>> thread... ...Ccing Satyam to caught their attention too.
>>
>>
>> ./drivers/isdn/hisax/hfc_pci.c
>> 125:	if (Read_hfc(cs, HFCPCI_INT_S1)) ;
>> 155:	if (Read_hfc(cs, HFCPCI_INT_S1)) ;
>> 1483:					if (Read_hfc(cs, HFCPCI_INT_S1)) ;
>> --
>> ./drivers/isdn/hisax/hfc_sx.c
>> 377:	if (Read_hfc(cs, HFCSX_INT_S1)) ;
>> 407:	if (Read_hfc(cs, HFCSX_INT_S2)) ;
>> 1246:					if (Read_hfc(cs, HFCSX_INT_S1)) ;
>> --
>
> These are workaround to not get compiler warnings about ignored return
> values I got some time ago under some architecture.
Maybe '(void) Read_hfc(cs, HFCSX_INT_S1)' is a better option to get  
rid of the warnings.

Re: drivers/infiniband/mlx/mad.c misplaced ;

[Satyam Sharma]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 914 bytes --]

Hi Ilpo,


On Thu, 16 Aug 2007, Ilpo Järvinen wrote:

> 
> ...I guess those guys hunting for broken busyloops in the other thread 
> could also benefit from similar searching commands introduced in this 
> thread... ...Ccing Satyam to caught their attention too.
> 
> 
> > On Wed, Aug 15, 2007 at 05:40:11PM -0700, Joe Perches wrote:
> > > 
> > > There's more than a few of these (not inspected).
> > > 
> > > $ egrep -r --include=*.c "\bif[[:space:]]*\([^\)]*\)[[:space:]]*\;" * 
> 
> ...Hmm, I plugged in "a preprocessor" too to manage with non compliant 
> coding styles :-). Please understand that the line numbers are not an 
> exact match due to preprocessor changes:
> 
> $ for i in `find . -name '*.[ch]'`; do echo $i; indent -npro -kr -i8 -ts8 
> -sob -l8000 -ss -ncs -cp1 -nhnl -st $i | egrep -n "[[:space:]]if [(].*[)] ;$";
> done | grep -B1 "^[^.]"

Thanks, looks useful, will check with this.


Satyam

Re: drivers/infiniband/mlx/mad.c misplaced ;

[Jeff Dike]

On Wed, Aug 15, 2007 at 05:40:11PM -0700, Joe Perches wrote:
> fs/hostfs/hostfs_user.c:        if(attrs->ia_valid & HOSTFS_ATTR_CTIME) ;

This one can be deleted, but I think I did it for documentation
reasons, to make it clear that ctime handling wasn't left out by
mistake.

				Jeff

-- 
Work email - jdike at linux dot intel dot com

[PATCH] hostfs: Remove pointless if statement

[Satyam Sharma]

[ Cc: list heavily trimmed. ]


On Thu, 16 Aug 2007, Jeff Dike wrote:

> On Wed, Aug 15, 2007 at 05:40:11PM -0700, Joe Perches wrote:
> > fs/hostfs/hostfs_user.c:        if(attrs->ia_valid & HOSTFS_ATTR_CTIME) ;
> 
> This one can be deleted, but I think I did it for documentation
> reasons, to make it clear that ctime handling wasn't left out by
> mistake.


We normally use "comments" for that, not dead code that a compiler
then elids ;-)


[PATCH] hostfs: Remove pointless if statement

And replace with comment saying we don't handle ctime.
Also some codingstyle while I was there.

Signed-off-by: Satyam Sharma <satyam@infradead.org>

---

 fs/hostfs/hostfs_user.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/hostfs/hostfs_user.c b/fs/hostfs/hostfs_user.c
index 5625e24..a554a0a 100644
--- a/fs/hostfs/hostfs_user.c
+++ b/fs/hostfs/hostfs_user.c
@@ -283,12 +283,12 @@ int set_attr(const char *file, struct hostfs_iattr *attrs, int fd)
 		}
 	}
 
-	if(attrs->ia_valid & HOSTFS_ATTR_CTIME) ;
-	if(attrs->ia_valid & (HOSTFS_ATTR_ATIME | HOSTFS_ATTR_MTIME)){
+	/* ctime is not handled */
+	if (attrs->ia_valid & (HOSTFS_ATTR_ATIME | HOSTFS_ATTR_MTIME)){
 		err = stat_file(file, NULL, NULL, NULL, NULL, NULL, NULL,
 				&attrs->ia_atime, &attrs->ia_mtime, NULL,
 				NULL, NULL, fd);
-		if(err != 0)
+		if (err != 0)
 			return err;
 	}
 	return 0;

Re: [PATCH] hostfs: Remove pointless if statement

[Jeff Dike]

On Fri, Aug 17, 2007 at 12:24:23PM +0530, Satyam Sharma wrote:
> We normally use "comments" for that, not dead code that a compiler
> then elids ;-)

I'd argue that comments are for when you can't make the code
self-explanatory.

> [PATCH] hostfs: Remove pointless if statement
> 
> And replace with comment saying we don't handle ctime.
> Also some codingstyle while I was there.

I'll forward this upstream.

				Jeff

-- 
Work email - jdike at linux dot intel dot com

Re: drivers/infiniband/mlx/mad.c misplaced ;

[Paul Mundt]

On Wed, Aug 15, 2007 at 05:40:11PM -0700, Joe Perches wrote:
> $ egrep -r --include=*.c "\bif[[:space:]]*\([^\)]*\)[[:space:]]*\;" * 
> arch/sh/boards/se/7343/io.c:    if (0) ;
> drivers/atm/iphase.c:     if (!desc1) ;
> drivers/infiniband/hw/mlx4/mad.c:       if (!err);
> drivers/isdn/capi/capiutil.c:   else if (c <= 0x0f);
> drivers/net/tokenring/ibmtr.c:  else if (ti->shared_ram_paging == 0xf);  /* No paging in adapter */
> drivers/s390/scsi/zfcp_erp.c:           if (status == ZFCP_ERP_SUCCEEDED) ;     /* no further action */
> fs/hostfs/hostfs_user.c:        if(attrs->ia_valid & HOSTFS_ATTR_CTIME) ;
> net/netfilter/xt_u32.c:         if (skb->len < 4 || pos > skb->len - 4);
> sound/pci/au88x0/au88x0_synth.c:                                if (eax == 0) ;

On Thu, Aug 16, 2007 at 02:18:40PM +0100, Andy Whitcroft wrote:
> A couple of people suggested adding checks to checkpatch for trailing
> semicolons on conditionals, where the conditional block may not be
> actually conditional:
> 
> 	if (err);
> 		return err;
> 
> While regression testing the changes, I ran these checks across the
> whole of 2.6.23-rc3 and there appear to be 5 places where this is
> occurs (above and beyond the IPv6 one which triggered this effort)
> and a benign use which could be confused later which it seems safest
> to fix.
> 
> Following this email are 6 patches for these issues, relevant
> maintainers cc'd.  All against 2.6.23-rc3
> 
It looks like you may want to refine your search parameters to match the
above, as there are at least a few cases where the space exists.

Re: drivers/infiniband/mlx/mad.c misplaced ;

[Roland Dreier]

Thanks!  Seems like checking for this is in the air, I just applied an
identical patch from Ilpo Järvinen.

[PATCH 1/6] mips: irix_getcontext will always fail EFAULT

[Andy Whitcroft]

Seems that a trailing ';' has slipped onto the end of the access_ok
check here, such that we will always return -EFAULT.

Signed-off-by: Andy Whitcroft <apw@shadowen.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: linux-mips@linux-mips.org
---
 arch/mips/kernel/irixsig.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/mips/kernel/irixsig.c b/arch/mips/kernel/irixsig.c
index 6980deb..28b2a8f 100644
--- a/arch/mips/kernel/irixsig.c
+++ b/arch/mips/kernel/irixsig.c
@@ -725,7 +725,7 @@ asmlinkage int irix_getcontext(struct pt_regs *regs)
 	       current->comm, current->pid, ctx);
 #endif
 
-	if (!access_ok(VERIFY_WRITE, ctx, sizeof(*ctx)));
+	if (!access_ok(VERIFY_WRITE, ctx, sizeof(*ctx)))
 		return -EFAULT;
 
 	error = __put_user(current->thread.irix_oldctx, &ctx->link);

Re: [PATCH 1/6] mips: irix_getcontext will always fail EFAULT

[Ralf Baechle]

On Thu, Aug 16, 2007 at 02:18:45PM +0100, Andy Whitcroft wrote:

> Seems that a trailing ';' has slipped onto the end of the access_ok
> check here, such that we will always return -EFAULT.

Ilpo Järvinen has sent an identical patch earlier which I have already
applied.  Thanks anyway,

  Ralf

[PATCH 2/6] powerpc: hash_preload fails to preload under CONFIG_PPC_MM_SLICES

[Andy Whitcroft]

Seems that a trailing ';' has slipped onto the end of the
get_slice_psize checks under CONFIG_PPC_MM_SLICES causing us to
return unconditionally and never preload.

Signed-off-by: Andy Whitcroft <apw@shadowen.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: linuxppc-dev@ozlabs.org
---
 arch/powerpc/mm/hash_utils_64.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c
index f178957..a47151e 100644
--- a/arch/powerpc/mm/hash_utils_64.c
+++ b/arch/powerpc/mm/hash_utils_64.c
@@ -795,7 +795,7 @@ void hash_preload(struct mm_struct *mm, unsigned long ea,
 
 #ifdef CONFIG_PPC_MM_SLICES
 	/* We only prefault standard pages for now */
-	if (unlikely(get_slice_psize(mm, ea) != mm->context.user_psize));
+	if (unlikely(get_slice_psize(mm, ea) != mm->context.user_psize))
 		return;
 #endif
 

[PATCH 3/6] sh: remove extraneous ; on scif_sercon_putc wait loop

[Andy Whitcroft]

It seems we have gained an extraneous trailing ';' on one of the
wait loops in scif_sercon_putc().  Although this is completely
benign as the apparent payload is also the empty statement, it
invites error in the future.  Clean it up now.

Signed-off-by: Andy Whitcroft <apw@shadowen.org>
Cc: Paul Mundt <lethal@linux-sh.org>
Cc: linuxsh-dev@lists.sourceforge.net
---
 arch/sh/kernel/early_printk.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/sh/kernel/early_printk.c b/arch/sh/kernel/early_printk.c
index 9833493..80b637c 100644
--- a/arch/sh/kernel/early_printk.c
+++ b/arch/sh/kernel/early_printk.c
@@ -76,7 +76,7 @@ static void scif_sercon_putc(int c)
 	sci_in(&scif_port, SCxSR);
 	sci_out(&scif_port, SCxSR, 0xf3 & ~(0x20 | 0x40));
 
-	while ((sci_in(&scif_port, SCxSR) & 0x40) == 0);
+	while ((sci_in(&scif_port, SCxSR) & 0x40) == 0)
 		;
 
 	if (c == '\n')

Re: [PATCH 3/6] sh: remove extraneous ; on scif_sercon_putc wait loop

[Paul Mundt]

On Thu, Aug 16, 2007 at 02:18:56PM +0100, Andy Whitcroft wrote:
> It seems we have gained an extraneous trailing ';' on one of the
> wait loops in scif_sercon_putc().  Although this is completely
> benign as the apparent payload is also the empty statement, it
> invites error in the future.  Clean it up now.
> 
> Signed-off-by: Andy Whitcroft <apw@shadowen.org>

Applied, thanks.

[PATCH 4/6] infiniband: mlx4_MAD_IFC copies out response unconditionally

[Andy Whitcroft]

It seems a trailing ';' has slipped into mlx4_MAD_IFC() which causes
the response to be copied out unconditionally.

Signed-off-by: Andy Whitcroft <apw@shadowen.org>
Cc: Roland Dreier <rolandd@cisco.com>
Cc: Sean Hefty <mshefty@ichips.intel.com>
Cc: Hal Rosenstock <hal.rosenstock@gmail.com>
Cc: general@lists.openfabrics.org
---
 drivers/infiniband/hw/mlx4/mad.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/infiniband/hw/mlx4/mad.c b/drivers/infiniband/hw/mlx4/mad.c
index 3330917..0ed02b7 100644
--- a/drivers/infiniband/hw/mlx4/mad.c
+++ b/drivers/infiniband/hw/mlx4/mad.c
@@ -109,7 +109,7 @@ int mlx4_MAD_IFC(struct mlx4_ib_dev *dev, int ignore_mkey, int ignore_bkey,
 			   in_modifier, op_modifier,
 			   MLX4_CMD_MAD_IFC, MLX4_CMD_TIME_CLASS_C);
 
-	if (!err);
+	if (!err)
 		memcpy(response_mad, outmailbox->buf, 256);
 
 	mlx4_free_cmd_mailbox(dev->dev, inmailbox);

Re: [PATCH 4/6] infiniband: mlx4_MAD_IFC copies out response unconditionally

[Roland Dreier]

 > It seems a trailing ';' has slipped into mlx4_MAD_IFC() which causes
 > the response to be copied out unconditionally.

Thanks -- this is the third time someone has sent me this patch in the
last day ;)

 - R.

[PATCH 5/6] irda_nl_get_mode: always results in failure

[Andy Whitcroft]

It seems an extraneous trailing ';' has slipped in to the error
handling for a name registration failure causing the error path to
trigger unconditionally.

Signed-off-by: Andy Whitcroft <apw@shadowen.org>
Cc: Samuel Ortiz <samuel@sortiz.org>
Cc: irda-users@lists.sourceforge.net
---
 net/irda/irnetlink.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/net/irda/irnetlink.c b/net/irda/irnetlink.c
index 694ea4d..1e429c9 100644
--- a/net/irda/irnetlink.c
+++ b/net/irda/irnetlink.c
@@ -106,7 +106,7 @@ static int irda_nl_get_mode(struct sk_buff *skb, struct genl_info *info)
 	}
 
 	if(nla_put_string(msg, IRDA_NL_ATTR_IFNAME,
-			  dev->name));
+			  dev->name))
 		goto err_out;
 
 	if(nla_put_u32(msg, IRDA_NL_ATTR_MODE, irlap->mode))

[PATCH 6/6] netfilter: xt_u32: fix length checks in u32_match_it

[Andy Whitcroft]

It seems an extraneous trailing ';' has slipped into the skb length
checks in u32_match_it() triggering an unconditional missmatch.

Signed-off-by: Andy Whitcroft <apw@shadowen.org>
Cc: netfilter-devel@lists.netfilter.org
---
 net/netfilter/xt_u32.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/net/netfilter/xt_u32.c b/net/netfilter/xt_u32.c
index 74f9b14..bec4279 100644
--- a/net/netfilter/xt_u32.c
+++ b/net/netfilter/xt_u32.c
@@ -36,7 +36,7 @@ static bool u32_match_it(const struct xt_u32 *data,
 		at  = 0;
 		pos = ct->location[0].number;
 
-		if (skb->len < 4 || pos > skb->len - 4);
+		if (skb->len < 4 || pos > skb->len - 4)
 			return false;
 
 		ret   = skb_copy_bits(skb, pos, &n, sizeof(n));

Re: [PATCH 6/6] netfilter: xt_u32: fix length checks in u32_match_it

[Patrick McHardy]

Andy Whitcroft wrote:
> It seems an extraneous trailing ';' has slipped into the skb length
> checks in u32_match_it() triggering an unconditional missmatch.


Thanks, this already fixed in net-2.6 and should hit Linus' tree soon.

Re: [PATCH 0/6] checkpatch checks for trailing semicolons on conditionals

[Satyam Sharma]

On Thu, 16 Aug 2007, Andy Whitcroft wrote:

> A couple of people suggested adding checks to checkpatch for trailing
> semicolons on conditionals, where the conditional block may not be
> actually conditional:
> 
> 	if (err);
> 		return err;
> 
> While regression testing the changes, I ran these checks across the
> whole of 2.6.23-rc3 and there appear to be 5 places where this is
> occurs (above and beyond the IPv6 one which triggered this effort)
> and a benign use which could be confused later which it seems safest
> to fix.
> 
> Following this email are 6 patches for these issues, relevant
> maintainers cc'd.  All against 2.6.23-rc3

Amazing :-) All 6 obviously correct, and one of those 5 bugs was around
for more than ~2 years! But most of these made one wonder (sadly) about
the (lack of) testing code got before getting merged, considering some
(#2 and 6, at least) should've been obvious immediately on basic testing
of the new feature, I think.