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

[Tor Bug Tracker & Wiki]

#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:
-------------------------------------------------+-------------------------

Comment (by teor):

 Replying to [comment:31 nickm]:
 > 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.

 You're right. Oops!

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

 I think we should have unit tests.

 > 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?

 We only check the fields that are output via the control port.
 We should revise this like to say "only" not "also":
 {{{
 /* Given two router status entries for the same router identity, return 1
 if
  * if the contents have changed between them. Otherwise, return 0.
  * It also checks for changes for that are output by control port. */
 }}}

 https://github.com/ArunaMaurya221B/tor/commit/e29292dda5ef68e441f267864357a7568b29588e
 #diff-fcb162b79906521706b54d280b939d57R1540

 > ---
 >
 > 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.

 I think it is enough to comment the control port printing function, and
 this function, and say that they must be kept in sync.

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