[openfabrics-ewg] RE: RDS testing utilities
Roland Dreier
rdreier at cisco.com
Tue Apr 18 21:32:00 PDT 2006
Ranjit> State of Rds in svn:
Ranjit> Rds has been moved into trunk. Latest changes have been
Ranjit> checked in, however, I just tested rds with svn tip and
Ranjit> something appears to be broken. I'm looking into the
Ranjit> issue.
Please budget quite a bit of time for cleaning up the code.
Running the script below on RDS still shows more than 500 lines with
messed up whitespace.
When I start reviewing the code, I see so much stuff before I get very
far into the first file that I lose interest. Some examples:
- grep printk *.c|grep -v KERN|wc shows 95 printks with no log level
- tons of things that should be static that aren't (try "make namespacecheck")
- basic violations of kernel code formatting like
if (cb == NULL)
{
return NULL;
}
which should just be:
if (!cb)
return NULL;
- scary-looking locking. for example doing stuff like
atomic_set(&cb->state, UNSTALLED);
makes me wonder -- atomic_t is not a magic cure for races, and in
fact if you only do atomic_set() and atomic_read() then you should
just use an int, because it amounts to the same thing (read
<asm/atomic.h>) and doesn't fool you into thinking there's some
protection against races.
I haven't taken the time to understand the cmpxchg() usage but I'm
dubious about it on general principles -- it would be better to
have nice plain old locks rather than trying to be very clever like
that. Is cmpxchg() even any faster? Both a spinlock and cmpxchg()
do one instruction with a "lock;" prefix, which is really the
expensive thing, so I would think they're the same.
Thanks,
Roland
#!/usr/bin/env perl
# print messages for misformatted C code
# usage: ws.pl <filenames>
while (<>) {
chomp;
if (/\s$/) {
print $ARGV, "(", $., "): trailing whitespace <", $_, ">\n";
}
if (/^\s* /) {
print $ARGV, "(", $., "): space followed by tab <", $_, ">\n";
}
if (/^ /) {
print $ARGV, "(", $., "): 8 spaces for indent <", $_, ">\n";
}
} continue {
close ARGV if eof;
}
More information about the ewg
mailing list