Fix and refactor and redocument routerstatus_has_changed
The routerstatus_has_changed() function is used by controllers to to tell which rs entries are new. But it only looks at a fraction of the fields which might change in a routerstatus. Also, it only checks for semantic changes that tor cares about (though this is not documented).
Activity
changed milestone to %Tor: 0.4.3.x-final
added 029-proposed 034-removed-20180328 034-triage-20180328 043-can actualpoints::0.6 component::core tor/tor easy ipv6 milestone::Tor: 0.4.3.x-final owner::nickm points::.1 priority::medium resolution::fixed review-group-31 reviewer::teor severity::normal spec-conformance status::closed tor-control type::defect labels
(do not change this till #19958 (moved) is merged)
There are a few different issues:
- the function comment needs to say that the function checks for semantic changes
- the function is used by two different parts of the code, and the controller part needs it to check for more changes
The controller part of the code wants all new descriptors: https://gitweb.torproject.org/torspec.git/tree/control-spec.txt#n2144
The other part of the code just wants descriptors that are different enough to matter.
This means adding an argument to the function that makes it check for different kinds of changes. Or making two functions. But we try not to copy and paste code, we get functions to call other functions instead.
routerstatus_has_changed() checks for some differences in descriptors. This is the right thing to do when the function is called from some code. But it hides some changed descriptors from controllers.
When called from control port code, the function should check if the timestamp has changed. That will give the controller all the changed descriptors.
Trac:
Username: aruna12340003-Bug-20218-Function-added-to-check-for-new-file-descr.patch
Your new function should have the same arguments and return type as routerstatus_has_changed():
static int routerstatus_has_changed(const routerstatus_t *a, const routerstatus_t *b)https://gitweb.torproject.org/tor.git/tree/src/or/networkstatus.c#n1541
So now that you have two routerstatus objects, what can you do?
Oh, I'm sorry, I've been giving you bad advice. I must have searched for the wrong function name.
We don't need a new function. We need to change the existing function to look at more fields.
Here's what we need to do:
The routerstatus_t struct has a lot of fields. routerstatus_has_changed() checks a few of them. But it needs to check every field that is output by networkstatus_getinfo_helper_single().
These missing fields can be compared using !=:
- published
- ipv6_orport
- is_v2_dir
- bandwidth_kb
This missing field must be compared using tor_addr_compare(..., ..., CMP_EXACT):
- ipv6_addr
We don't need to worry about the summarised IPv4 exit policy output by networkstatus_getinfo_helper_single(). The descriptor_digest check will find changes in this field.
Then we need to add comments to routerstatus_has_changed() saying that it checks for changes that are output by the control port. And we need to add a comment to routerstatus_format_entry() saying that any extra control port fields need to be added to routerstatus_has_changed() as well.
Trac:
Keywords: N/A deleted, ipv6 added
Status: new to needs_revision
Milestone: Tor: unspecified to Tor: 0.3.4.x-final-
Trac:
Username: aruna1234 -
Here's it. I have put it in a separate branch: [https://github.com/ArunaMaurya221B/tor/commit/e29292dda5ef68e441f267864357a7568b29588e]
Trac:
Username: aruna1234 Hi, this branch doesn't compile, it has extra "+" at the start of some lines. Please run "make check" before submitting patches,
Also, please remove this comment, because this patch fixes it:
// XXXX this function needs a huge refactoring; it has gotten out // XXXX of sync with routerstatus_t, and it will do so again.-
Oh !!My bad:( I have done the final changes:[https://github.com/ArunaMaurya221B/tor/commits/Bug-20218]
Trac:
Username: aruna1234 -
Corrected! [https://github.com/ArunaMaurya221B/tor/commits/Bug-20218]
Trac:
Username: aruna1234 -
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:
- Making sure that when we construct routerstatus_t, we initialize the whole object to 0.
- In a function like this, using memcpy() to make a temporary copy of the routerstatus_t.
- 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.
Trac:
Status: needs_review to needs_revision Replying to 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.-
First patch on tor, please tell me if anything is wrong. See https://github.com/vnepveu/tor/tree/ticket20218. edit : change branch name
Trac:
Username: maurice_pibouin
Status: needs_revision to needs_review -
Created pull request as suggested : https://github.com/torproject/tor/pull/1549
Trac:
Username: maurice_pibouin -
I would actually love some help ! I am a complete beginner in C, so I don't know how to go around with writing the makefile for my test (just to see if my tests work). I have follwed tutorials on how to write makefiles, but I don't see how to handle the multiple depth of dependencies. What steps should I take to write my test makefile ? Since I don't want to go through "making" the whole Tor project everytime I want to see if my test file works. Also, is this the right place to ask for help ? It feels weird posting questions on an issue. Thanks !
Trac:
Username: maurice_pibouin -
I've rebased on master (to sort out the merge history) and added a unit test, as
ticket20218_rebasedwith PR at https://github.com/torproject/tor/pull/1670 .I have some questions in the unit test about particular fields; please let me know what you think there, and I'll revise the test.
Trac:
Status: needs_revision to needs_review The implementation of routerstatus_has_changed() is correct, it's only designed to detect changes in the control port routerstatus format. Let's rename routerstatus_has_changed() to a name that contains "control"?
If you'd like to remove the named and unnamed code, let's do it in another ticket, and try to remove as much as we can.
Trac:
Status: needs_review to needs_revision-
Okay -- I've changed the comments, updated the tests to verify that routerstatus_format_entry() actually changed (or hasn't), and renamed the identifier. I went with "visibly" instead of "control", but I'm open to suggestions for other identifiers that aren't even longer.
Trac:
Status: needs_revision to needs_review There's a conflict with #32695 (moved), which removed networkstatus_consensus_has_ipv6() above routerstatus_has_visibly_clanged(). The conflict is stopping CI from running,
But the code looks good.
I'll resolve it, run CI, then merge.
Trac:
Status: needs_review to merge_readyHere's the PR merged into the current master, with conflicts resolved:
Waiting for CI to pass before merging.
(I also had to remove ROUTERSTATUS_FORMAT_NO_CONSENSUS_METHOD from the tests, also due to #32695 (moved).)
Merged to master.
Merged #32962 (moved), #32984 (moved), and #20218 (moved) together.
Trac:
Actualpoints: 0.5 to 0.6
Resolution: N/A to fixed
Status: merge_ready to closedmentioned in issue #32962 (moved)
mentioned in issue #32984 (moved)
moved to tpo/core/tor#20218 (closed)
