[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #2149 [Tor Client]: new 'extra dormant' mode for people who never use their tor
#2149: new 'extra dormant' mode for people who never use their tor
---------------------------------+------------------------------------------
Reporter: arma | Owner:
Type: enhancement | Status: needs_revision
Priority: major | Milestone: Tor: 0.2.4.x-final
Component: Tor Client | Version:
Keywords: performance scaling | Parent:
Points: | Actualpoints:
---------------------------------+------------------------------------------
Comment(by sysrqb):
Replying to [comment:14 nickm]:
> Thanks for the code! Just pulled the branch -- The "Using wrong idle
checker" patch doesn't seem to be there yet?
That's odd. I'll take a look.
>
> Quick review:
> * IDLE_TIME_STAGE isn't used.
I forgot to remove this, fixed.
> * stopped_fetching_descriptors isn't documented.
Missed that. Fixed.
> * needs a changelog entry in the changes directory.
I added one in Using-wrong-idle-checker commit, I wasn't really sure what
to say, so it'll probably be revised.
> * or_options_t is for stuff parsed from torrc and the command line --
it isn't a catch-all for persistent state. That would be or_state_t.
Making this change will also let you back out lots of the
get_options_mutable() calls, which ought to be a red flag.
Ah! That makes more sense. I'll make the necessary changes.
> * I don't see the point of having DormantStage be global: why can't
it be a local variable in directory_too_idle_to_fetch_descriptors() ?
This relates to arma's comment about clients that are consistently idle
but they don't run continuously for the entire "IDLE_TIME_BEFORE_DORMANT"
time period. My thinking was that if we add it to the state file then when
the client starts again it won't necessarily start from time=0. It really
shouldn't be global, but it needs to be set when the state file is read
and I wasn't sure how else to accomplish this.
> * I don't understand the logic of the changes in conn_read_callback
and do_main_loop. In any case, it feels like
"directory_info_has_arrived()" might be the wrong function to be calling
here -- after all, you are calling it *not* in response to directory info
arriving.[*]
a) Ok, I misunderstood conn_read_callback's use. I'll move the changes.
b) I seem to have created a bit of duplicate code, I'll fix that. But what
I was actually trying to do is check that the client is not idle and if it
is then not call directory_info_has_arrived because it potentially
downloads new descriptors and such. I toyed with skipping the reload of
the saved networkstatuses and consensus but decided against it. On
rethinking this, if we don't call directory_info_has_arrived there's not
much reason to load the cached info.
c) I agree. Bad habit. I mostly wanted to make sure I was on the correct
track with this patch.
> * The comment on 59c8df8b59bf04f25 seems wrong; conn_read_callback
isn't the function that gets called on "an incoming connection" -- it gets
called every time any socket is readable. If you want to have something
happen on incoming connections, that would belong in
connection_handle_listener_read, I think.
Same as a) above. I did a lot of manual backtracing and I guess I got a
little confused along the way.
>
> [*] Functions should be called as documented, and not because they have
some desirable side-effect. Let's say there is a function called
computer_is_on_fire() that floods the server room with carbon dioxide. It
is IMO wrong to call computer_is_on_fire() when the computer is not on
fire. If we sometimes want to flood the server room with CO2 when there
is no fire, then we should make an flood_server_room() function. That
makes our code more maintainable and less confusing.
Thanks for the feedback
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/2149#comment:15>
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