Re: (Desperate) Plea for multi-person code review

[Karsten Loesing <karsten.loesing@xxxxxxx>]

On Feb 15, 2010, at 3:09 AM, Mike Perry wrote:

> Thus spake Mike Perry (mikeperry@xxxxxxxxxx):
> 
>> My past few posts to this list have been about developing new formulas
>> for load balancing Tor requests. I've since implemented these formulas
>> in my git branch mikeperry/consensus-bw-weights3. You can get it with:
>> 
>> git clone git://git.torproject.org/git/tor.git tor.git
>> cd tor.git
>> git remote add mikeperry git://git.torproject.org/mikeperry/tor
>> git fetch mikeperry
>> git branch --track mp-consensus-bw-weights3 mikeperry/consensus-bw-weights3
>> git checkout mp-consensus-bw-weights3
>> 
>> X11 users can see the change sets easily with 'gitk'.
> 
> Sebastian requested that I should utilize git to engage in some
> revisionist history and make my branch look a little less
> schizophrenic.
> 
> I've now rebased, split, and squashed all the commits into 8
> logically distinct patches in mikeperry/consensus-bw-weights4.
> Should be easier to review for those who prefer to look at things
> one commit at a time.

I had a quick look at your branch to find similar mistakes as I would make them. I should say that I didn't check any of your calculations which are quite hard to get started with. So, don't consider this a real code review.

Your last commit breaks test_dir.c which is probably okay, because that commit will go away anyway.

I found a comment 'XXX: Should this be+1 for the \n?'. Do you want to make sure you're doing the right thing and take this comment out?

In smartlist_choose_by_bandwidth_weights() (and maybe some other places), you might write 'weight = is_dir ? Wbd*Wd : Wd;' instead of using if-else. Makes your code shorter and easier to read, IMO.

There are some, IMO, unnecessary newlines before closing }'s. I extended the check-spaces script as pasted below. I figured it's probably easier for you to put it in rather than pulling these few lines from my branch and doing the git black magic to remove your last commit before applying mine.

diff --git a/contrib/checkSpace.pl b/contrib/checkSpace.pl
index db061a0..074fb6d 100755
--- a/contrib/checkSpace.pl
+++ b/contrib/checkSpace.pl
@@ -33,6 +33,10 @@ for $fn (@ARGV) {
             print " #else#if:$fn:$.\n";
        }
        $lastline = $_;
+        ## Warn about unnecessary empty lines.
+        if ($lastnil && /^\s*}\n/) {
+            print "  UnnecNL:$fn:$.\n";
+        }
         ## Warn about multiple empty lines.
         if ($lastnil && /^$/) {
             print " DoubleNL:$fn:$.\n";

Would it make sense to update the list of consensus methods in dir-spec.txt, section 3.4.1? It seems that nobody did that for the past few methods, though.

Sometimes you're writing 'if () foo;' in one line which is, IMO, not-so-good coding style. I don't know how to tell Perl to detect that, though.

In networkstatus_verify_bw_weights, you have lines like 'if (fabs(Wem - Wee) > 1) {'. You are comparing double to int here, which is probably okay, but somewhat bad style, IMO. More importantly, if Wem and Wee differ by exactly 1, your condition doesn't detect that difference; did you mean '> 0.00001' or something?


These are just minor issues, as you can see. I think the most promising approaches to find more bugs would be: 1) write some tests, 2) run the code in a private Tor network, 3) ask more people to review your code, and/or 4) just put it in 0.2.2.x and hope to find the bugs before 0.2.2.x becomes the new stable. 1) and 2) are probably rather painful. If you want to do 2) and need support, please let me know.

--Karsten