(Desperate) Plea for multi-person code review

[Mike Perry]

Ok, consensus-bw-weights4 should have fixes for these, with the
exception of the XXX in find_start_of_next_routerstatus() that you
mention. That XXX actually is for the old code, which seemed to
preserve the \n for some reason there.

Nick, do you think you could have a look at that function, and see
what should be done there? It looks like it was possibly a bug or just
bad behavior to me.

In general, I'd also like someone to verify that adding the new
'directory-footer' line and 'bandwidth-weights' line to the consensus
won't break existing clients. It looks to me like the parsing code is
written to handle the addition of arbitrary new lines in the
consensus, but I'd like some confirmation of that. For example, is it
possible that the last routerstatus document in the consensus might
get silently rejected due to the parser finding the extra lines there?
routerstatus_parse_entry_from_string() looks like it might be OK with
that to me, but I'm not familiar with all of the bits of the parsing
code.

Thus spake Karsten Loesing (karsten.loesing at gmx.net):

> 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

-- 
Mike Perry
Mad Computer Scientist
fscked.org evil labs
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.torproject.org/pipermail/tor-dev/attachments/20100220/3c579920/attachment.pgp>