Re: [tor-bugs] #20218 [Core Tor/Tor]: Fix and refactor and redocument routerstatus_has_changed

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#20218: Fix and refactor and redocument routerstatus_has_changed
-------------------------------------------------+-------------------------
 Reporter:  nickm                                |          Owner:  (none)
     Type:  defect                               |         Status:
                                                 |  needs_revision
 Priority:  Medium                               |      Milestone:  Tor:
                                                 |  0.3.4.x-final
Component:  Core Tor/Tor                         |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  ipv6, 029-proposed, tor-control,     |  Actual Points:  0.5
  easy, spec-conformance, review-group-31        |
Parent ID:                                       |         Points:  .1
 Reviewer:  nickm                                |        Sponsor:
-------------------------------------------------+-------------------------
Changes (by nickm):

 * status:  needs_review => needs_revision


Comment:

 teor, I think you have tor_addr_compare backwards. Its documentation says:
 {{{
  * Given two addresses <b>addr1</b> and <b>addr2</b>, return 0 if the two
  * addresses are equivalent under the mask mbits, less than 0 if addr1
  * precedes addr2, and greater than 0 otherwise.
 }}}

 So the original use of tor_addr_compare (without "!") is correct.

 (Unit tests would have caught this bug; that's one reason why unit tests
 are so great. ;) Any chance of writing those?)

 As a smaller issue, it seems that this patch says the same line twice:
 {{{
          a->is_hs_dir != b->is_hs_dir ||
          a->is_hs_dir != b->is_hs_dir ||
 }}}

 ---
 Another question: how did you decide on the list of fields to check?  It
 seems that some but not all of the fields in routerstatus_t are now
 checked for equality, but I don't understand the rationale about why the
 remaining ones (pv, exitsummary) are not.  Is that intentional?

 ---

 Teor asks:
 >  How can we make sure we update functions when we add new members to a
 struct?

 In some cases, using a constructor for a struct instance will work, since
 we have turned on the options that let us know about any uninitialized
 members.  But in cases like this, I don't see an easy way to use a
 constructor.

 One option here would be to stop assuming that we list all relevant the
 members here.  We could instead change the code that we only list the
 ''irrelevant'' members, by:
   1. Making sure that when we construct routerstatus_t, we initialize the
 whole object to 0.
   2. In a function like this, using memcpy() to make a temporary copy of
 the routerstatus_t.
   3. Instead of listing all the relevant members in a comparison, we could
 just use fast_memeq() to compare.  If there are some members we don't want
 to compare, we would set them to 0 before calling fast_memeq().
 I'm not sure if this is actually a good idea, though.

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

_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs