[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