Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#4620 closed defect (fixed)

Move ipv6_preferred from routerinfo_t to node_t

Reported by: ln5 Owned by: ln5
Priority: Low Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: ipv6 tor-relay
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by ln5)

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.

Child Tickets

Change History (12)

comment:1 Changed 9 years ago by nickm

Milestone: Tor: 0.2.3.x-final

comment:2 Changed 8 years ago by ln5

Owner: set to ln5
Status: newassigned

comment:3 Changed 8 years ago by ln5

Description: modified (diff)
Summary: Move the prefer_ipv6 to node_tMove ipv6_preferred from routerinfo_t to node_t

comment:4 Changed 8 years ago by ln5

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

comment:5 Changed 8 years ago by nickm

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.

comment:6 Changed 8 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: 0.2.4.x-final

I'm going to move this to 0.2.4.x; it's worth doing, but 0.2.3 is fine how it is.

comment:7 in reply to:  5 ; Changed 8 years ago by 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.
  • 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?

comment:8 in reply to:  7 Changed 8 years ago by nickm

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

comment:9 Changed 8 years ago by ln5

Status: assignedneeds_review

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.

comment:10 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

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!

comment:11 Changed 8 years ago by nickm

Keywords: tor-relay added

comment:12 Changed 8 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.