[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #12538 [Tor]: Make all relays automatically be dir caches
#12538: Make all relays automatically be dir caches
-------------------------+-------------------------------------------------
Reporter: | Owner:
cypherpunks | Status: needs_review
Type: task | Milestone: Tor: 0.2.6.x-final
Priority: normal | Version: Tor: unspecified
Component: Tor | Keywords: tor-guard, tor-relay, prop237
Resolution: | 026-triaged-1
Actual Points: | Parent ID:
Points: |
-------------------------+-------------------------------------------------
Comment (by sysrqb):
Pushed bug12538_prop_squash_1, adding function rename and unit test.
Replying to [comment:19 dgoulet]:
> I'm wondering if it would be useful to have "is_directory_server()"
function that could take a routerinfo_t and test that object to see if
it's a directory instead of the "options" object parsing?
>
> It could then be used with "desc_routerinfo" object representing our
router information thus fixing the semantic "issue".
I considered taking a routerinfo_t as a parameter, but I decided against
it because 1) I wanted to match the signature of server_mode() as closely
as possible, and 2) this function is only called when a router must decide
if it serves dir requests, so an or_options_t should be sufficient.
Until this changes in the future, simply checking the is_v2_dir field of
another router's routerinfo_t is all that is needed to determine if it is
a dir server. I think it would be unnecessarily complicated otherwise, so
I think it's okay that we have different interfaces for answering the
questions "am I a directory server?" verses "is she a directory server?".
>
> Also, I'm a big fan of namespacing function with the file name so this
function is in "router.c" and public thus maybe should be called
"router_is_directory_server()" ? (unless there is a coding rules I'm not
aware of)
I agree, but I'll follow Nick's suggestions below in this case.
Replying to [comment:20 nickm]:
> - In set_routerstatus_from_routerinfo, shouldn't it also set is_v2_dir
to true if ri->dir_port is nonzero? That's what the proposal says, I
think.
{{{
@@ -2090,6 +2090,7 @@ set_routerstatus_from_routerinfo(routerstatus_t *rs,
strlcpy(rs->nickname, ri->nickname, sizeof(rs->nickname));
rs->or_port = ri->or_port;
rs->dir_port = ri->dir_port;
+ rs->is_v2_dir = ri->supports_tunnelled_dir_requests;
}}}
I don't think it's necessary because supports_tunnelled_dir_requests is
only set when the descriptor is parsed in router_parse_entry_from_string
and we set it true if either the dirport is set or "tunnelled-dir-server"
was in the descriptor. Is there a case I'm missing?
{{{
+ /* This router accepts tunnelled directory requests via begindir if it
has
+ * an open dirport or it included "tunnelled-dir-server". */
+ if (find_opt_by_keyword(tokens, K_DIR_TUNNELLED) || router->dir_port >
0) {
+ router->supports_tunnelled_dir_requests = 1;
+ }
}}}
> - dir_server_mode() or running_as_dir_server() would be a better name
for is_directory_server()
Agreed, let's use the former so it matches server_mode() and
public_server_mode().
> - Do we have a test for router_pick_directory_server_impl()? If not,
should we get one somehow?
No, and yes.
It's not a very elegant solution (directly including routerlist.c).
Alternatively I can make router_pick_directory_server_impl and
routerlist_insert STATIC or create TOR_UNIT_TESTS-specific wrappers around
them. The existing non-static wrappers added unnecessary complexity
considering I'm trying to test a single function, but if using a wrapper
is more mergeable then I can expand the unit test so that will work. Which
do you prefer?
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/12538#comment:21>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs