Skip to contentSkip to conversation

kj-devel.pl

PATCH In review Vicente Feito 2 participants 2 messages Mar 22, 2005 – Mar 27, 2005

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 kj-devel.pl

This patch goes against the original file here:
http://www.kerneljanitors.org/scripts/kj-devel.pl

Randy: The comment you've made about spinlocks, you can easily let them out of the checking by using --nospinlocks, but, I don't understand the comment on using the nr line at the begining instead of using it at the end, I mean, any special reason? (Also, I'm not a native english speaker so I really appreciate the corrections, since I've learned english from HBO, not kidding :).

I've added checks for the use of memset with backward parameters, because ultimately a lot of these appeared.

I was about to rewrite some part of the code last night to make it more user friendly in order to allow other people add their regexes easily, but I don't know if someone is even using this, so I've avoided that by now, if I get some feedback on new things to check (besides the ones I already got to add) I'll rewrite that part, I think it would help.

I'm still wondering wether or not to check for the use of task queues, I think people still use them, so suggestions are more than welcome about this (and anything else).

Now that ldd3 is out, people can even send more suggestions in order to check for other things, but that's up to the janitors team members who hang in the channel 24/7 hardly speaking (how is this possible?they're always on profound meditation states, shhh ;)

kj-devel.pl net/*/*.c - it's plagued of return ESOMETHING (it's plagued of everything) instead of return -ESOMETHING;  (of course there are some false positives because of the existance of i.e. return ETH_HLEN which is correct) but some are wrong.

Vicente.

256,257c256,257
< print "Unlocking a lock that hasn't been aquired ".
< "yet(could be a false positive) - Line:$linenr\n\n";
---

Quoted 2 lines
					print "Unlocking a lock that hasn't been acquired ".
						"yet (could be a false positive) - Line:$linenr\n\n";

272,273c272,273
< print "Unlocking a lock that hasn't been aquired ".
< "yet(could be a false positive) - Line:$linenr\n\n";
---

Quoted 2 lines
					print "Unlocking a lock that hasn't been acquired ".
						"yet (could be a false positive) - Line:$linenr\n\n";

287,288c287,288
< print "Unlocking a lock that hasn't been aquired ".
< "yet(could be a false positive) - Line:$linenr\n\n";
---

Quoted 2 lines
					print "Unlocking a lock that hasn't been acquired ".
						"yet (could be a false positive) - Line:$linenr\n\n";

302c302
< "yet(could be a false positive) - Line:$linenr\n\n";
---

Quoted 1 line
						"yet (could be a false positive) - Line:$linenr\n\n";

347c347,351
< print "Using foo[] it's recommended over *foo: saves memory references & code\n";
---

Quoted 5 lines
				print "Using foo[] is recommended over *foo: saves memory references & code\n";
				print "$filename:$linenr:$line\n\n";
			}
			if ($line=~/memset\((.*),( *sizeof.*),( *\d+)\)/) {
				print "Your memset parameters appears to be backwards\n";

359c363
< print "If you're building a module, be concious that you must use the".
---

Quoted 1 line

print "If you're building a module, note that you must use the".

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

#1 lore raw
Quoted 8 lines · Vicente Feito

This patch goes against the original file here:
http://www.kerneljanitors.org/scripts/kj-devel.pl

Randy: The comment you've made about spinlocks, you can easily let them out of the checking by using --nospinlocks, but, I don't understand the comment on using the nr line at the begining instead of using it at the end, I mean, any special reason? (Also, I'm not a native english speaker so I really appreciate the corrections, since I've learned english from HBO, not kidding :).

Wow, that's cool. :)

Line nrs at beginning are just more common in some other tools, like gcc and sparse.

Quoted 2 lines

I've added checks for the use of memset with backward parameters, because ultimately a lot of these appeared.

Yes, that's good.
Here's something else that I've seen a few patches for recently:

	if (condition);
or
	for (a; b; c);
or
	while (condition);

Of course, any of these may be valid, but several of them probably are not. Here's a patch from today that you can use as an example:

drivers/usb/net/zd1201.c +5 -5
@@ -673,7 +673,7 @@
  		return 0;
 
  	err = zd1201_docmd(zd, ZD1201_CMDCODE_ENABLE, 0, 0, 0);
-	if (!err);
+	if (!err)
  		zd->mac_enabled = 1;
 
  	if (zd->monitor)
@@ -690,7 +690,7 @@
  		return 0;
  	if (zd->monitor) {
  		err = zd1201_setconfig16(zd, ZD1201_RID_PROMISCUOUSMODE, 0);
-		if (err);Signed-off-by: Colin Leroy <[email protected]>
--- a/drivers/usb/net/zd1201.c	2005-03-24 12:24:23.000000000 +0100
+++ b/drivers/usb/net/zd1201.c	2005-03-24 12:24:41.000000000 +0100
@@ -673,7 +673,7 @@
  		return 0;
 
  	err = zd1201_docmd(zd, ZD1201_CMDCODE_ENABLE, 0, 0, 0);
-	if (!err);
+	if (!err)
  		zd->mac_enabled = 1;
 
  	if (zd->monitor)
@@ -690,7 +690,7 @@
  		return 0;
  	if (zd->monitor) {
  		err = zd1201_setconfig16(zd, ZD1201_RID_PROMISCUOUSMODE, 0);
-		if (err);
+		if (err)
  			return err;
  	}
+		if (err)

return err;
}

Quoted 21 lines

I was about to rewrite some part of the code last night to make it more user friendly in order to allow other people add their regexes easily, but I don't know if someone is even using this, so I've avoided that by now, if I get some feedback on new things to check (besides the ones I already got to add) I'll rewrite that part, I think it would help.

I'm still wondering wether or not to check for the use of task queues, I think people still use them, so suggestions are more than welcome about this (and anything else).

Now that ldd3 is out, people can even send more suggestions in order to check for other things, but that's up to the janitors team members who hang in the channel 24/7 hardly speaking (how is this possible?they're always on profound meditation states, shhh ;)

kj-devel.pl net/*/*.c - it's plagued of return ESOMETHING (it's plagued of everything) instead of return -ESOMETHING; (of course there are some false positives because of the existance of i.e. return ETH_HLEN which is correct) but some are wrong.

Vicente.

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