[PATCH] fs/eventpoll: error handling micro-cleanup

[PATCH] fs/eventpoll: error handling micro-cleanup

[Jeff Garzik]

While reviewing the 'may be used uninitialized' bogus gcc warnings,
I noticed that an error code assignment was only needed if an error had
actually occured.

Signed-off-by: Jeff Garzik <jeff@garzik.org>

diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 8d54433..557d5b6 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -720,9 +720,10 @@ static int ep_getfd(int *efd, struct ino
 
 	/* Allocates an inode from the eventpoll file system */
 	inode = ep_eventpoll_inode();
-	error = PTR_ERR(inode);
-	if (IS_ERR(inode))
+	if (IS_ERR(inode)) {
+		error = PTR_ERR(inode);
 		goto eexit_2;
+	}
 
 	/* Allocates a free descriptor to plug the file onto */
 	error = get_unused_fd();

Re: [PATCH] fs/eventpoll: error handling micro-cleanup

[Davide Libenzi]

On Sun, 1 Oct 2006, Jeff Garzik wrote:

> 
> While reviewing the 'may be used uninitialized' bogus gcc warnings,
> I noticed that an error code assignment was only needed if an error had
> actually occured.

But that saved one line of code, and there are countless occurences in the 
kernel of such code pattern ;)
In any case, fine by me and not worth further discussion.



> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -720,9 +720,10 @@ static int ep_getfd(int *efd, struct ino
>  
>  	/* Allocates an inode from the eventpoll file system */
>  	inode = ep_eventpoll_inode();
> -	error = PTR_ERR(inode);
> -	if (IS_ERR(inode))
> +	if (IS_ERR(inode)) {
> +		error = PTR_ERR(inode);
>  		goto eexit_2;
> +	}
>  
>  	/* Allocates a free descriptor to plug the file onto */
>  	error = get_unused_fd();



- Davide

Re: [PATCH] fs/eventpoll: error handling micro-cleanup

[Jeff Garzik]

Davide Libenzi wrote:
> On Sun, 1 Oct 2006, Jeff Garzik wrote:
> 
>> While reviewing the 'may be used uninitialized' bogus gcc warnings,
>> I noticed that an error code assignment was only needed if an error had
>> actually occured.
> 
> But that saved one line of code, and there are countless occurences in the 
> kernel of such code pattern ;)

I'm not sure there are countless occurrences with PTR_ERR().  The line 
is incorrect (but harmless) if inode is a valid pointer...


> In any case, fine by me and not worth further discussion.

Thanks :)


>> --- a/fs/eventpoll.c
>> +++ b/fs/eventpoll.c
>> @@ -720,9 +720,10 @@ static int ep_getfd(int *efd, struct ino
>>  
>>  	/* Allocates an inode from the eventpoll file system */
>>  	inode = ep_eventpoll_inode();
>> -	error = PTR_ERR(inode);
>> -	if (IS_ERR(inode))
>> +	if (IS_ERR(inode)) {
>> +		error = PTR_ERR(inode);

Re: [PATCH] fs/eventpoll: error handling micro-cleanup

[Davide Libenzi]

On Sun, 1 Oct 2006, Jeff Garzik wrote:

> Davide Libenzi wrote:
> > On Sun, 1 Oct 2006, Jeff Garzik wrote:
> > 
> > > While reviewing the 'may be used uninitialized' bogus gcc warnings,
> > > I noticed that an error code assignment was only needed if an error had
> > > actually occured.
> > 
> > But that saved one line of code, and there are countless occurences in the
> > kernel of such code pattern ;)
> 
> I'm not sure there are countless occurrences with PTR_ERR().  The line is
> incorrect (but harmless) if inode is a valid pointer...

I just tried a `find /usr/src/linux-2.6.16/ -type f -exec grep -H -C 2 PTR_ERR {} \;`
and looked at the cases where the error variable is assigned in any case 
before the test. Same code pattern as, like:

error = -EFAULT;
if (copy_from_user(...))
	goto kaboom;

Again, expect a big patch if you're gonna fix all those ;)



- Davide

Re: [PATCH] fs/eventpoll: error handling micro-cleanup

[Jeff Garzik]

Davide Libenzi wrote:
> On Sun, 1 Oct 2006, Jeff Garzik wrote:
> 
>> Davide Libenzi wrote:
>>> On Sun, 1 Oct 2006, Jeff Garzik wrote:
>>>
>>>> While reviewing the 'may be used uninitialized' bogus gcc warnings,
>>>> I noticed that an error code assignment was only needed if an error had
>>>> actually occured.
>>> But that saved one line of code, and there are countless occurences in the
>>> kernel of such code pattern ;)
>> I'm not sure there are countless occurrences with PTR_ERR().  The line is
>> incorrect (but harmless) if inode is a valid pointer...
> 
> I just tried a `find /usr/src/linux-2.6.16/ -type f -exec grep -H -C 2 PTR_ERR {} \;`
> and looked at the cases where the error variable is assigned in any case 
> before the test. Same code pattern as, like:
> 
> error = -EFAULT;
> if (copy_from_user(...))
> 	goto kaboom;

No, that's quite different.  I'm talking about

	ptr = get_a_pointer_from_somewhere()
	error = PTR_ERR(ptr)

See the difference?  The error variable is directly assigned from a 
potentially-valid pointer.  Doing that is quite error prone, whereas
	error = -EFOO
is not.

	Jeff

Re: [PATCH] fs/eventpoll: error handling micro-cleanup

[Davide Libenzi]

On Sun, 1 Oct 2006, Jeff Garzik wrote:

> Davide Libenzi wrote:
> 
> > I just tried a `find /usr/src/linux-2.6.16/ -type f -exec grep -H -C 2
> > PTR_ERR {} \;`
> > and looked at the cases where the error variable is assigned in any case
> > before the test. Same code pattern as, like:
> > 
> > error = -EFAULT;
> > if (copy_from_user(...))
> > 	goto kaboom;
> 
> No, that's quite different.  I'm talking about
> 
> 	ptr = get_a_pointer_from_somewhere()
> 	error = PTR_ERR(ptr)
> 
> See the difference?  The error variable is directly assigned from a
> potentially-valid pointer.

So? Is PTR_ERR() defined and documented in a way that, if called with a 
valid pointer, has an unexpected/faulty behaviour?
Again, I don't care either ways, but don't tell me you're not sure about 
the countless occurrences. Take a look at:

`find $LINUXSRC -type f -exec grep -H -C 2 PTR_ERR {} \;`



- Davide

Re: [PATCH] fs/eventpoll: error handling micro-cleanup

[Jeff Garzik]

Davide Libenzi wrote:
> On Sun, 1 Oct 2006, Jeff Garzik wrote:
> 
>> Davide Libenzi wrote:
>>
>>> I just tried a `find /usr/src/linux-2.6.16/ -type f -exec grep -H -C 2
>>> PTR_ERR {} \;`
>>> and looked at the cases where the error variable is assigned in any case
>>> before the test. Same code pattern as, like:
>>>
>>> error = -EFAULT;
>>> if (copy_from_user(...))
>>> 	goto kaboom;
>> No, that's quite different.  I'm talking about
>>
>> 	ptr = get_a_pointer_from_somewhere()
>> 	error = PTR_ERR(ptr)
>>
>> See the difference?  The error variable is directly assigned from a
>> potentially-valid pointer.
> 
> So? Is PTR_ERR() defined and documented in a way that, if called with a 
> valid pointer, has an unexpected/faulty behaviour?

When called with a valid pointer, the value assigned to the return-code 
integer is essentially a random number.


> Again, I don't care either ways, but don't tell me you're not sure about 
> the countless occurrences. Take a look at:
> 
> `find $LINUXSRC -type f -exec grep -H -C 2 PTR_ERR {} \;`

Perhaps 1 out of every 100 or so hits from this find(1) is unprotected 
by IS_ERR().  IOW, what I've been describing here is quite rare.

	Jeff

Re: [PATCH] fs/eventpoll: error handling micro-cleanup

[Davide Libenzi]

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

On Sun, 1 Oct 2006, Jeff Garzik wrote:

> > So? Is PTR_ERR() defined and documented in a way that, if called with a
> > valid pointer, has an unexpected/faulty behaviour?
> 
> When called with a valid pointer, the value assigned to the return-code
> integer is essentially a random number.

That will never be used ...



> > Again, I don't care either ways, but don't tell me you're not sure about the
> > countless occurrences. Take a look at:
> > 
> > `find $LINUXSRC -type f -exec grep -H -C 2 PTR_ERR {} \;`
> 
> Perhaps 1 out of every 100 or so hits from this find(1) is unprotected by
> IS_ERR().  IOW, what I've been describing here is quite rare.

Made a stupid script that try to find such code pattern. Found 193 
instances all around the code. Report and script included for you view 
(I verified about 10 or so of those, and they look correctly captured by 
the script).



- Davide


[-- Attachment #2: Type: TEXT/x-perl, Size: 442 bytes --]

#!/usr/bin/perl -w

use strict;


my $fname = 0;
my $lcnt = 0;
my $state = 0;
my $found = 0;

while (<STDIN>) {
    $lcnt++;
    if (/PTR_ERR/) {
	$state = 1;
	next;
    }
    if ($state == 0 || (/^[\r\n \t]+$/)) {
	next;
    }
    if (/^[ \t]*if[ ]*\(/) {
	if ($fname == 0) {
	    print "File: ", $ARGV[0], "\n";
	    $fname++;
	}
	print "Found at $lcnt\n";
	$found++;
    }
    $state = 0;
}

exit($found);


[-- Attachment #3: Type: TEXT/PLAIN, Size: 6459 bytes --]

File: ./arch/alpha/kernel/osf_sys.c
Found at 328
Found at 348
Found at 377
File: ./arch/alpha/kernel/process.c
Found at 455
File: ./arch/arm/kernel/sys_arm.c
Found at 274
File: ./arch/arm/plat-omap/cpu-omap.c
Found at 108
File: ./arch/arm26/kernel/sys_arm.c
Found at 277
File: ./arch/cris/arch-v10/kernel/process.c
Found at 218
File: ./arch/cris/arch-v32/kernel/process.c
Found at 234
File: ./arch/frv/kernel/process.c
Found at 261
File: ./arch/h8300/kernel/process.c
Found at 224
File: ./arch/i386/kernel/process.c
Found at 747
File: ./arch/ia64/ia32/sys_ia32.c
Found at 99
File: ./arch/ia64/kernel/perfmon.c
Found at 1494
File: ./arch/ia64/kernel/process.c
Found at 664
File: ./arch/m32r/kernel/process.c
Found at 327
File: ./arch/m68k/kernel/process.c
Found at 373
File: ./arch/m68knommu/kernel/process.c
Found at 359
File: ./arch/mips/kernel/linux32.c
Found at 156
File: ./arch/mips/kernel/syscall.c
Found at 219
File: ./arch/mips/kernel/sysirix.c
Found at 802
Found at 821
File: ./arch/parisc/hpux/fs.c
Found at 40
File: ./arch/parisc/kernel/process.c
Found at 356
File: ./arch/parisc/kernel/sys_parisc32.c
Found at 77
File: ./arch/powerpc/kernel/process.c
Found at 810
File: ./arch/powerpc/kernel/sys_ppc32.c
Found at 425
File: ./arch/powerpc/platforms/cell/spufs/inode.c
Found at 313
File: ./arch/powerpc/platforms/cell/spufs/syscalls.c
Found at 84
File: ./arch/s390/hypfs/hypfs_diag.c
Found at 564
File: ./arch/s390/kernel/compat_linux.c
Found at 537
File: ./arch/s390/kernel/process.c
Found at 329
File: ./arch/sh/kernel/process.c
Found at 423
File: ./arch/sh64/kernel/process.c
Found at 830
File: ./arch/sparc/kernel/process.c
Found at 664
File: ./arch/sparc/kernel/sys_sunos.c
Found at 783
Found at 788
Found at 809
File: ./arch/sparc64/kernel/process.c
Found at 798
File: ./arch/sparc64/kernel/sys_sparc32.c
Found at 729
File: ./arch/sparc64/kernel/sys_sunos32.c
Found at 748
Found at 753
Found at 774
File: ./arch/um/kernel/exec.c
Found at 80
File: ./arch/v850/kernel/process.c
Found at 176
File: ./arch/x86_64/ia32/sys_ia32.c
Found at 855
File: ./arch/x86_64/kernel/process.c
Found at 636
File: ./arch/xtensa/kernel/syscalls.c
Found at 119
File: ./block/ll_rw_blk.c
Found at 2463
File: ./drivers/block/nbd.c
Found at 315
File: ./drivers/char/misc.c
Found at 291
File: ./drivers/char/tpm/tpm_tis.c
Found at 661
File: ./drivers/infiniband/core/uverbs_cmd.c
Found at 741
File: ./drivers/message/i2o/i2o_config.c
Found at 263
Found at 338
File: ./drivers/mtd/devices/block2mtd.c
Found at 223
File: ./drivers/s390/block/dasd.c
Found at 2072
File: ./drivers/s390/block/dasd_devmap.c
Found at 836
File: ./drivers/s390/char/monreader.c
Found at 510
File: ./drivers/s390/char/tape_class.c
Found at 80
File: ./drivers/s390/net/qeth_main.c
Found at 8577
File: ./drivers/scsi/scsi_proc.c
Found at 204
File: ./fs/9p/fcall.c
Found at 139
File: ./fs/aio.c
Found at 1273
File: ./fs/autofs4/root.c
Found at 160
File: ./fs/binfmt_elf.c
Found at 684
File: ./fs/binfmt_elf_fdpic.c
Found at 235
File: ./fs/binfmt_flat.c
Found at 801
File: ./fs/binfmt_misc.c
Found at 185
Found at 628
File: ./fs/block_dev.c
Found at 328
File: ./fs/compat.c
Found at 863
Found at 1523
File: ./fs/dcache.c
Found at 1539
File: ./fs/dquot.c
Found at 1542
File: ./fs/eventpoll.c
Found at 773
Found at 1689
File: ./fs/exec.c
Found at 144
Found at 1045
Found at 1143
File: ./fs/exportfs/expfs.c
Found at 127
Found at 361
File: ./fs/ext2/acl.c
Found at 287
Found at 432
File: ./fs/ext2/dir.c
Found at 456
File: ./fs/ext2/namei.c
Found at 110
Found at 138
Found at 162
Found at 220
File: ./fs/ext3/acl.c
Found at 292
Found at 456
File: ./fs/ext3/namei.c
Found at 1662
Found at 1667
Found at 1696
Found at 1701
Found at 1732
Found at 1737
Found at 2080
Found at 2137
Found at 2142
Found at 2191
Found at 2234
File: ./fs/filesystems.c
Found at 130
File: ./fs/fuse/dir.c
Found at 301
File: ./fs/fuse/file.c
Found at 308
Found at 406
File: ./fs/hppfs/hppfs_kern.c
Found at 478
Found at 532
File: ./fs/jffs2/acl.c
Found at 295
Found at 321
Found at 409
Found at 443
File: ./fs/jffs2/scan.c
Found at 349
File: ./fs/jffs2/summary.c
Found at 487
File: ./fs/jfs/acl.c
Found at 162
File: ./fs/minix/dir.c
Found at 212
File: ./fs/namei.c
Found at 588
Found at 1214
Found at 1313
Found at 1638
Found at 1848
Found at 1912
Found at 1921
Found at 2031
Found at 2105
Found at 2180
Found at 2189
Found at 2283
Found at 2483
Found at 2503
Found at 2538
Found at 2556
File: ./fs/namespace.c
Found at 1565
File: ./fs/nfs/dir.c
Found at 1156
File: ./fs/nfs/mount_clnt.c
Found at 67
File: ./fs/nfs/namespace.c
Found at 117
File: ./fs/nfs/nfs3acl.c
Found at 27
Found at 36
File: ./fs/nfs/nfs4proc.c
Found at 3310
File: ./fs/nfs/write.c
Found at 868
File: ./fs/nfsd/nfs2acl.c
Found at 55
Found at 78
File: ./fs/nfsd/nfs3acl.c
Found at 51
Found at 74
File: ./fs/nfsd/nfs4recover.c
Found at 227
File: ./fs/nfsd/vfs.c
Found at 204
Found at 1120
Found at 1250
Found at 1432
Found at 1503
Found at 1575
Found at 1587
Found at 1653
File: ./fs/ntfs/attrib.c
Found at 1015
Found at 2190
Found at 2228
Found at 2241
File: ./fs/ntfs/bitmap.c
Found at 173
File: ./fs/ntfs/file.c
Found at 931
File: ./fs/ntfs/lcnalloc.c
Found at 937
File: ./fs/ntfs/logfile.c
Found at 404
File: ./fs/open.c
Found at 1083
File: ./fs/proc/base.c
Found at 1098
File: ./fs/proc/root.c
Found at 50
File: ./fs/reiserfs/xattr.c
Found at 1092
Found at 1290
File: ./fs/reiserfs/xattr_acl.c
Found at 58
File: ./fs/seq_file.c
Found at 114
Found at 197
File: ./fs/super.c
Found at 801
File: ./fs/sysv/dir.c
Found at 204
File: ./fs/sysv/namei.c
Found at 76
Found at 101
Found at 146
File: ./fs/ufs/dir.c
Found at 334
File: ./fs/ufs/namei.c
Found at 89
Found at 111
Found at 137
Found at 201
File: ./ipc/shm.c
Found at 238
File: ./kernel/acct.c
Found at 214
File: ./kernel/fork.c
Found at 237
File: ./mm/mempolicy.c
Found at 784
File: ./mm/shmem.c
Found at 2350
File: ./mm/swapfile.c
Found at 1154
Found at 1160
Found at 1420
Found at 1426
File: ./mm/tiny-shmem.c
Found at 117
File: ./net/ipv4/arp.c
Found at 1037
File: ./net/sunrpc/auth_gss/auth_gss.c
Found at 567
File: ./net/sunrpc/clnt.c
Found at 91
File: ./net/unix/af_unix.c
Found at 802