kj-devel.pl

KJ Vicente Feito 2 participants 4 messages Jan 31, 2005 – Feb 10, 2005 Kernel janitors

Share & tools

Vicente Feito

author

2005-01-31 19:34 UTC #0 lore raw

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.

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Randy.Dunlap

2005-02-07 23:35 UTC #1 lore raw

Quoted 1 line · Vicente Feito

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

You read this part of the TODO list, right?
"NOTE: scripts/kj.pl was a weekend hack. Don't take it too seriously."

However, kj.pl _could_ become a great tool if you and others are willing to spend some time on it. In some cases it might be better to make additions to sparse instead, e.g., Linus has recently begun adding lock/unlock annotation support to sparse so that it can check for missing lock or unlock calls.

Quoted 8 lines

----------------------------------------------------------------------------------------
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

As I think you found out, these are recommendations, not hard rules.

Quoted 5 lines

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.

MODULE_AUTHOR() is now required? where is that written?

Quoted 3 lines

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

Yes, they should all be gone. I don't find any, but it's inexpensive to make sure that they don't reappear.

Quoted 10 lines

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)

Quoted 7 lines

----------------------------------------------------------------------------------------
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.

I expect that would produce quite a few false warnings on things like sizeof(pid_t) (just a quick example).

Quoted 1 line

And the balance of alloc/free with all types of calls (all of them)

Good.

Quoted 11 lines

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.

It will take some time and work, but if you are willing, it could be a very useful tool. I'll test your second script patch soon.

Signature · 5 lines

~Randy
_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Vicente Feito

author

2005-02-08 07:57 UTC #2 lore raw

In reply to Vicente Feito

Hello.

Quoted 4 lines · you

Quoted 2 lines · Vicente Feito
This is the devel script with some modifications/added features, the details:

Quoted 8 lines

You read this part of the TODO list, right?
"NOTE: scripts/kj.pl was a weekend hack. Don't take it too seriously."
However, kj.pl _could_ become a great tool if you and others are willing to spend some time on it. In some cases it might be better to make additions to sparse instead, e.g., Linus has recently begun adding lock/unlock annotation support to sparse so that it can check for missing lock or unlock calls.

Yes, I have to be honest, I've started adding some things before reading the 'weekend hack thing', and about 3 hours later I've found that line, so I've added 1 more hour of thinking and decided to send it anyway.
I think the good part about this script it's that it can be a lightweight tool for those who don't want to complicate themselves by using sparse at the beginning, I think they can just start using this for little things, what do you think?

Quoted 13 lines

Quoted 10 lines
-------------------------------------------------------------------------
--------------- 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
As I think you found out, these are recommendations, not hard rules.

Quoted 7 lines

Quoted 5 lines
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.
MODULE_AUTHOR() is now required? where is that written?

Section 3.2.1 appears as Module Interface Changes :

-It is now required to have all module information in you code:
MODULE_AUTHOR,MODULE_DESCRIPTION,SUPPORTED_DEVICE, PARM_DESC
I'm not checking for MODULE_DESC, I'll should add that one and PARM_DESC.

Quoted 22 lines

Quoted 4 lines
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
Yes, they should all be gone. I don't find any, but it's inexpensive to make sure that they don't reappear.
Quoted 14 lines
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)
-------------------------------------------------------------------------
---------------

Quoted 8 lines

Quoted 5 lines
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.
I expect that would produce quite a few false warnings on things like sizeof(pid_t) (just a quick example).

I'll need to think about it more, but there's always a way, the bad part it's that sometimes the way involves reading the file twice, of course, not literarily the file, but the data.

Quoted 15 lines

Quoted 2 lines
And the balance of alloc/free with all types of calls (all of them)
Good.
Quoted 10 lines
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)
-------------------------------------------------------------------------
---------------

Quoted 5 lines

Quoted 2 lines
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.
It will take some time and work, but if you are willing, it could be a very useful tool. I'll test your second script patch soon.

The second script it's just the removal of strlen and sprintf checks, I'll be adding some more things to this.

Vicente.

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Randy.Dunlap

2005-02-10 21:34 UTC #3 lore raw

In reply to Vicente Feito

Quoted 3 lines

The second script it's just the removal of strlen and sprintf checks, I'll be adding some more things to this.

Here are a few more false reports, if you are interested, from the linux/ipc/ directory:

Should be return -E ?mqueue.c:1453: return ERR_PTR(-EFAULT);
Should be return -E ?mqueue.c:1455: return ERR_PTR(-EINVAL);
Should be return -E ?mqueue.c:1463: return ERR_PTR(ret);
Should be return -E ?mqueue.c:1480: return ERR_PTR(-EINVAL);
Should be return -E ?mqueue.c:1483: return ERR_PTR(-EACCES);
Should be return -E ?msg.c:1453: return ERR_PTR(-EFAULT);
Should be return -E ?msg.c:1455: return ERR_PTR(-EINVAL);
Should be return -E ?msg.c:1463: return ERR_PTR(ret);
Should be return -E ?msg.c:1480: return ERR_PTR(-EINVAL);
Should be return -E ?msg.c:1483: return ERR_PTR(-EACCES);

and some good ones:

Bad. Should use set_current_state.
msg.c:2230: current->state=TASK_INTERRUPTIBLE;

Bad. Should use set_current_state.
msg.c:2836: current->state = TASK_INTERRUPTIBLE;

Any idea what this message means?

Unmatched ) in regex; marked by <-- HERE in m/spin_unlock.*) <-- HERE
inside ipc_lock_by_ptr(/ at /home/rddunlap/scripts/kj-devel-vic2.pl
line 61.

From the linux/lib/ directory, I am seeing comment lines not being
skipped:

strtok() is racy on smp, use strsep() instead. string.c:7440: * - Added strsep() which will replace strtok() soon (because strsep() is

strtok() is racy on smp, use strsep() instead. string.c:7445: * - Kissed strtok() goodbye

strtok() is racy on smp, use strsep() instead. vsprintf.c:7440: * - Added strsep() which will replace strtok() soon (because strsep() is

strtok() is racy on smp, use strsep() instead. vsprintf.c:7445: * - Kissed strtok() goodbye

Signature · 5 lines

~Randy
_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors