[tor-bugs] #13339 [Tor]: Merge GSoC project - Consensus Diffs

[Tor Bug Tracker & Wiki]

#13339: Merge GSoC project - Consensus Diffs
-----------------------------+-------------------------------------------
     Reporter:  mvdan        |      Owner:
         Type:  enhancement  |     Status:  needs_review
     Priority:  major        |  Milestone:  Tor: 0.2.6.x-final
    Component:  Tor          |    Version:
   Resolution:               |   Keywords:  gsoc merge tor-client prop140
Actual Points:               |  Parent ID:
       Points:               |
-----------------------------+-------------------------------------------

Comment (by dgoulet):

 First of all, the test don't compile. Multiple warnings (too many to paste
 here but here is an idea):

 {{{
 src/test/test_consdiff.c:25:3: warning: implicit declaration of function
 ‘test_eq_ptr’ [-Wimplicit-function-declaration]
    test_eq_ptr(sl, sls->list);
 src/test/test_consdiff.c:52:3: warning: implicit declaration of function
 ‘test_eq’ [-Wimplicit-function-declaration]
    test_eq(3, smartlist_slice_string_pos(sls, "a"));
    ^
 src/test/test_consdiff.c:81:3: warning: implicit declaration of function
 ‘test_memeq’ [-Wimplicit-function-declaration]
    test_memeq(e_lengths1, lengths1, sizeof(int) * 6);
 src/test/test_consdiff.c:161:3: warning: implicit declaration of function
 ‘test_assert’ [-Wimplicit-function-declaration]
    test_assert(!bitarray_is_set(changed1, 0));
 src/test/test_consdiff.c:920:12: error: ‘legacy_test_helper’ undeclared
 here (not in a function)
    { #name, legacy_test_helper, 0, &legacy_setup, test_consdiff_ ## name }
 }}}


 Ok that's a big diff chunk of code. There are a lot of commits that could
 be squashed together here to make things easier. For instance, I'm
 thinking of from the older one up to when consdiff.c is created and
 consdiff_client/_server.c are deleted.

 So I see in proposal 140 that multiple consensus hash can be fetched by
 the client but this implementation only uses one single hash. Is it the
 spec that is out dated or there is no use to fetch multiple consensus now?

 I'll skip all coding style issue here for now since you told me you might
 change it. Here some minor thing I've found while going over the code.
 I'ven't yet finish a full review

 '''src/or/consdiff.h'''
 * Clarify what "list" contains.
 * "len" maybe should be renamed to something more meaninful because it
 does NOT represent the len of the list but the range that should be used
 in that list (of what I understand)?

 '''src/or/consdiff.c'''

 * You should set "network-status-diff-version" in a static const variable
 at the begining of the file. You are using it twice and since it's part of
 a public ABI, good practice to have that declared somewhere instead of in
 the middle of the code. Same goes for "hash" in consdiff_get_digests().
 * Same goes for "X-Or-Diff-From-Consensus:" in directory.c

 '''src/or/networkstatus.c'''

 * I would go for switch/case when you check consensus_flavor_t since it's
 an enum and the compiler warns you when you forget one. Just a cool extra
 useful protection. In networkstatus_get_latest_consensus_by_flavor() and
 networkstatus_get_latest_consensus_mmap_by_flavor()

 '''src/or/dirserv.c'''

 * connection_dirserv_add_cons_diff_bytes_to_outbuf(): this one looks
 really like {{{connection_dirserv_add_networkstatus_bytes_to_outbuf}}}
 except the lookup if done somewhere else. I feel like this one could get
 refactor to either take an enum type of what we want or "lookup fct
 pointer" instead of duplication.


 Ok I'll stop for now but I have to say that it's good work :). Will do
 another run at it after your comments/changes.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/13339#comment:4>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online