#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
_______________________________________________ tor-bugs mailing list tor-bugs@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs