[KJ] kj-devel.pl

* [KJ] kj-devel.pl
@ 2005-01-31 19:34 Vicente Feito
  2005-02-07 23:35 ` Randy.Dunlap
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Vicente Feito @ 2005-01-31 19:34 UTC (permalink / raw)
  To: kernel-janitors

[-- Attachment #1: Type: text/plain, Size: 2459 bytes --]

This is the devel script with some modifications/added features, the details:

----------------------------------------------------------------------------------------
Added a chunk of code to handle comments, this kind of comments /*...*/ and 
/*...
....
... */
but no // C++ style comments since I think there's no point in doing that.

Based on the DO's and DONT's
Drivers shouldn't call panic() directly.
Checking the use of module_init and module_exit (assuming if it's using one 
it's using the other)
Adding the GET_USE_COUNT check, cause now it's 'forbidden' too.
Checking the required MODULE_AUTHOR,MODULE_DESCRIPTION,SUPPORTED_DEVICE.
Checking the change from MODULE_PARM to module_param(name, type, perm)
Checking the non-existance of __SMP__ which is non-existant by noChecking the 
non-existance of __SMP__ which is non-existant by now
Warning about the use of racy strtok, and the fact the it should be replaced 
with strsep.
Checking that the driver don't use syscalls (meaning, avoiding int 0x80)
Checking the save_flags/save_flags_cli/restore_flags use that it's now 
deprecated.
Warning about the use of proc_register instead of create_proc_read_entry().
Warning about the use of strlen and sprintf too.
Telling about the better use of const char foo[] = "bla"; vs const char *foo = 
"bla"; (asm code)

----------------------------------------------------------------------------------------

What it should be fix:
----------------------------------------------------------------------------------------
What I've been thinking on doing is checking that the sizeof takes pointers as 
arguments (thinking about that), instead of the types, that way, things can 
be easy to mantain.
And the balance of alloc/free with all types of calls (all of them)
And also checking that functions use __init as recommended(this goes from 
check_init called from the module_init/exit part.
I've been thinking that transforming the array into an array of arrays it 
could be easier to handle all the files, including the header files.
The linenumbers are wrong, the numbers are printed relative to the position in
the array instead of the position in the file, this can be fixed using array 
of array (references or something similar)
----------------------------------------------------------------------------------------

I'm not sure if I can keep adding things to this, I would like to know
if I may or may not do that.
Thank you
Regars
Vicente Feito.

[-- Attachment #2: kj-devel.pl --]
[-- Type: application/octet-stream, Size: 7766 bytes --]

#!/usr/bin/perl -w
#
# An automated kernel janitor.
# (c) 2001, Dave Jones. <davej@suse.de>, with invaluable
# perl assistance from Rob Andrews <nine@impure.org.uk>
# Various code also inspired from hundreds of other perl scripts.
#
# Additional functionality added by The kerneljanitors.org team :)
#
# Licensed under the terms of the GNU GPL License version 2
#
# $Id: kj-devel.pl,v 1.3 2004/04/22 21:44:15 randy Exp $
#
# 1.0 Initial version.
# 1.1 Spinlock balancing checker improved.
#

use strict;

my @lines;
my $linesinfile=0;
my $module;		#flag, mantains the module_init/_exit
my $drvdata = 3; 	#this allow to check MODULE_ macros
my $comment = 0;

foreach my $file (@ARGV) {
	process ($file)
			or warn "Couldn't check file $file: $!";
}

sub check_pci_enable_device {
	my ($used, $filename) = @_;
	my $currentlinenr;
	foreach my $current (@lines) {
		if ($current=~/pci_enable_device/) {
			if ($currentlinenr < $used) {
				print "Should be ok, called on line $currentlinenr\n";
				print "$filename:$currentlinenr:$current\n";
				return;
			}
		}
		$currentlinenr++;
	}
}

sub check_unlock {
	my ($lockname, $filename, $locklinenr, $unlockregexp) = @_;

	foreach my $linenr ($locklinenr..$linesinfile) {

		#This code skips comments.
		next if ($lines[$linenr]=~/(^|\s+)\/\*.*\*\//);
		$comment = 1 if ($lines[$linenr]=~/(^|\s+)\/\*/);
		if ($comment) {
			$comment = 0 if ($lines[$linenr]=~/\*\//);
			next;
		}

		if ($lines[$linenr]=~/$unlockregexp.*$lockname/) {
			print ("Lock closed on line $linenr\n");
			return 1;
		}

		if ($lines[$linenr]=~/return/) {
			return 0;
		}
	}
}

sub process {
	my $filename=shift;

	return undef unless $filename;

	open(INPUT, "<$filename") or return undef;

	push @lines, <INPUT>;
	$linesinfile += @lines;

	close(INPUT);		

#	s!/\*(.*?)\*/!"\n" x (@lines =~ tr/\n//)!esg;

my $spinlockwarn=0;
my $lookspci=0;
my $linenr=0;

	foreach my $line (@lines) {

		$linenr++;

# For later, we may need to preprocess include files.
# read as: Include them into the array before the current file.
#		if ($line=~/^#include ?[<"](.*)[>"]/i) {

		if ($line=~/static const char.*__initdata/) {
			print "const & __initdata in string def. Remove const.\n";
			print "$filename:$linenr:$line\n\n";
		}

		if ($line=~/\(struct netdev_private \*\)/) {
			print "Unnecessary cast.\n";
			print "$filename:$linenr:$line\n\n";
		}

		if ($line=~/netif_rx/) {
			print "Net drivers should set dev->last_rx immediately after netif_rx\n";
			print "Also make sure the skb isn't referenced after giving it to netif_rx\n";
			print "$filename:$linenr:$line\n\n";
		}

		if ($line=~/(GET|MOD_(INC|DEC))_USE_COUNT/) {
			print "MOD_{INC,DEC}_USE_COUNT and GET_USE_COUNT are deprecated for 2.6: see the module-init-tools FAQ\n";
			print "$filename:$linenr:$line\n\n";
		}

		if ($line=~/SET_MODULE_OWNER/) {
			print "SET_MODULE_OWNER is useless for 2.6: see the module-init-tools FAQ\n";
			print "$filename:$linenr:$line\n\n";
		}

		if ($line=~/sleep_on/) {
			print "Using sleep_on derivative, is racy. consider using wait_event instead\n";
			print "$filename:$linenr:$line\n\n";
		}

		if ($line=~/check_region/) {
			print "Using check_region, is racy, use just request_region and check for its return.\n";
			print "$filename:$linenr:$line\n\n";
		}

		if ($line=~/check_mem_region/) {
			print "Using check_mem_region, is racy, use just request_mem_region and check for its return.\n";
			print "$filename:$linenr:$line\n\n";
		}

		if ($line=~/pdev-\>irq/) {
			print "Make sure pci_enable_device before reading irq\n";
			check_pci_enable_device($linenr, $filename);
			print "$filename:$linenr:$line\n\n";
		}

		if (($lookspci==0) and ($line=~/pci_/)) {
			$lookspci=1;
			print "Looks like a PCI driver. Make sure it uses pci_enable_device.\n";
			check_pci_enable_device($linenr, $filename);
			print "$filename:$linenr:$line\n\n";
		}

		if ($line=~/pdev-\>resource/) {
			print "Make sure pci_enable_device --before-- reading resource\n";
			check_pci_enable_device($linenr, $filename);
			print "$filename:$linenr:$line\n\n";
		}

		if ($line=~/pcibios_/) {
			print "Uses obsolete pcibios_xxx functions.\n";
			print "$filename:$linenr:$line\n\n";
		}

		if ($line=~/save_flags_cli/) {
			print "Use local_irq_save instead.\n";
			print "$filename:$linenr:$line\n\n";
		}

		if ($line=~/isa_{read,write}{b,w,l}/) {
			print "Use ioremap instead of isa_read/write functions.\n";
			print "$filename:$linenr:$line\n\n";
		}

		if ($line=~/current-\>state/) {
			print "Bad. Should use set_current_state.\n";
			print "$filename:$linenr:$line\n\n";
		}

		if ($line=~/loops_per_sec/) {
			print "Warning: loops_per_sec may change..\n";
			print "$filename:$linenr:$line\n\n";
		}

		if ($line=~/if.*dev-\>mem_start/) {
			if (($line!~/0xffffffff/) or ($line!~/-1/)) {
				print "Should check for 0xffffffff too.\n";
				print "$filename:$linenr:$line\n\n";
			}
		}

		if (($spinlockwarn==0) and
			(($line=~/cli\(\)/) or
			 ($line=~/sti\(\)/) or
			 ($line=~/lock_kernel/))){
			$spinlockwarn=1;
			print "Consider using spinlocks.\n";
			print "$filename:$linenr:$line\n\n";
		}

		if ($line=~/return E/) {
			print "Should be return -E ?";
			print "$filename:$linenr:$line\n\n";
		}

		if ($line=~/spin_lock\((.*)\)/) {
			if (check_unlock($1, $filename, $linenr, "spin_unlock") == 0) {
				print "Obtained spinlock on line $linenr, but never unlocked.\n";
				print "$filename:$linenr:$line\n\n";
			}
		}
		if ($line=~/spin_lock_irqsave\((.*)\)/) {
			if (check_unlock($1, $filename, $linenr, "spin_unlock_irqrestore") == 0) {
				print "Obtained spinlock on line $linenr, but never unlocked.\n";
				print "$filename:$linenr:$line\n\n";
			}
		}
		if ($line=~/spin_lock_bh\((.*)\)/) {
			if (check_unlock($1, $filename, $linenr, "spin_unlock_bh") == 0) {
				print "Obtained spinlock on line $linenr, but never unlocked.\n";
				print "$filename:$linenr:$line\n\n";
			}
		}

		#Here are the new ones
		if ($line=~/panic\((.*)\)/) {
			print "Device Drivers as a general rule should not call panic().\n";
			print "$filename:$linenr:$line\n\n";
		}
		if ($line=~/save_flags(_cli)?\(\)/ or $line=~/restore_flags\(\)/) {
			print "save_flags_* and restore flags should not be used.\n";
			print "$filename:$linenr:$line\n\n";
		}
		if ($line=~/module_init\((.*)\)/ or $line=~/module_exit\((.*)\)/) {
			$module = 1;
		}
		if ($line=~/MODULE_AUTHOR\(.*\)/ or
			$line=~/MODULE_LICENSE\(.*\)/ or
			$line=~/SUPPORTED_DEVICE\(.*\)/) {
			$drvdata--;
		}
		if ($line=~/MODULE_PARM\(.*\)/) {
			print "MODULE_PARM() has been replaced by module_param(name, type, perm).\n";
			print "$filename:$linenr:$line\n\n";
		}
		if ($line=~/^#.*__SMP__/) {
			print "__SMP__ is no longer used and it's going to dissapear soon.\n";
			print "$filename:$linenr:$line\n\n";
		}
		if ($line=~/strtok\(.*\)/) {
			print "strtok() is racy on smp, use strsep() instead.\n";
			print "$filename:$linenr:$line\n\n";
		}
		if ($line=~/int 0x80/) {
			print "Device Drivers should never use syscalls.\n";
			print "$filename:$linenr:$line\n\n";
		}
		if ($line=~/\bproc_register\(.*\)/) {
			print "proc_register() should be replaced with create_proc_entry().\n";
			print "$filename:$linenr:$line\n\n";
		}
		if ($line=~/strlen\(/ or $line=~/sprintf\(/) {
			print "strlen and sprint should be avoided in drivers.\n";
			print "$filename:$linenr:$line\n\n";
		}
		if ($line=~/(const )?char ?\* ?\w+ ?= ?\".*\"/) {
			print "Using foo[] it's recommended against *foo\n";
			print "$filename:$linenr:$line\n\n";
		}
	}
	if (!defined($module)) {
		print "You must use the init mechanism, module_init() and module_exit().\n";
	}
	if ($drvdata) {
		print "MODULE_AUTHOR/MODULE_LICENSE/MODULE_DESC are now required.\n";
	}

	return 1;
}

[-- Attachment #3: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

^ permalink raw reply	[flat|nested] 4+ messages in thread