Move ipv6_preferred from routerinfo_t to node_t
The ipv6_preferred flag in routerinfo_t should not be there since that data structure is supposed to be immutable. Move it to node_t.
Activity
changed milestone to %Tor: 0.2.4.x-final
Trac:
Summary: Move the prefer_ipv6 to node_t to Move ipv6_preferred from routerinfo_t to node_t
Description: The prefer_ipv6 flag in routerinfo_t should not be there since that data structure is supposed to be immutable. Move it to node_t.to
The ipv6_preferred flag in routerinfo_t should not be there since that data structure is supposed to be immutable. Move it to node_t.
When is code like this safe?
node = node_get_mutable_by_id(router->cache_info.identity_digest); tor_assert(node);
For example, dirserv_set_router_is_running() is doing that (and #5529 (moved) mimics that in dirserv_single_reachability_test()).
node_get_mutable_by_id() searches nodelist_map (nodelist.c) which is populated by node_get_or_create(), invoked by nodelist_add_routerinfo() and nodelist_set_consensus().
So it looks like if ROUTER is in consensus or has been added to nodelist_map by other means (like getting a descriptor directly from a bridge?) node_get_mutable_by_id() will succeed.
I'm wondering since I'm trying to figure out if it's worth it or not to move the ipv6_preferred flag to node_t if the risk of not having a node_t at the time we might need the flag exists. Maybe this should be dealt with in a more general overhaul of routerinfo_t vs. node_t?
Whenever we have a routerinfo_t in the routerlist, there should be a node_t for it. That's one of the things that the routerinfo_* maintenance functions handle (by calling nodelist_add_routerinfo).
That said, I'm fine with letting this wait for 0.2.4.x if you think it's safer that way. If you agree, feel free to change the milestone.
Please comment on bug4620_hackery in my repo. Still not ready for merging -- lacks a changes file and good committ message(s) if nothing else. I'd like to know if this is what we want first.
Addressing Nicks comments he gave on IRC yesterday:
-
Making node_assert_ok() it into a function would require that func and such would be passed to it, wouldn't it? Keeping it a macro and using the tor_assert() macro seems risky, f.ex. because 'n' would be evaluated twice. Moved it to nodelist.h as is.
-
Kept extend_info_from_router(), made it static and removed the now unused second arg. Does this make more sense?
-
Complemented get_configured_bridge_by_addr_port_digest() with get_configured_bridge_by_orports_digest() rather than replacing it.
-
Moved node_* to nodelist.[ch]. Should node_describe(), format_node_description() and node_get_description() move too?
-
-
Replying to ln5:
Please comment on bug4620_hackery in my repo. Still not ready for merging -- lacks a changes file and good committ message(s) if nothing else. I'd like to know if this is what we want first.
Addressing Nicks comments he gave on IRC yesterday:
- Making node_assert_ok() it into a function would require that func and such would be passed to it, wouldn't it? Keeping it a macro and using the tor_assert() macro seems risky, f.ex. because 'n' would be evaluated twice. Moved it to nodelist.h as is.
ok, will fix
- Kept extend_info_from_router(), made it static and removed the now unused second arg. Does this make more sense?
sure
- Complemented get_configured_bridge_by_addr_port_digest() with get_configured_bridge_by_orports_digest() rather than replacing it.
seems good
- Moved node_* to nodelist.[ch]. Should node_describe(), format_node_description() and node_get_description() move too?
eventually; not in this patch
Thanks for the review. Please take a look at bug4620 in my repo.
The commit message contains a question about code duplication that I'd like a comment on before merge.
Otherwise, this patch Works For Me, as we say. I hope it won't break anything critical. Like Tor.
Trac:
Status: assigned to needs_review-
Reviewing one last time; the issues here are for me to fix as I merge:
- the documentation for get_configured_bridge_* should be a little more explicit about "if possible...if necessary" mean.
- fix node_assert_ok to not duplicate code; I don't want a zillion reimplementations of tor_assert.
Tweaked those and pushed to master; thanks!
Trac:
Status: needs_review to closed
Resolution: N/A to fixed mentioned in issue #5535 (moved)
moved to tpo/core/tor#4620 (closed)