[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #6465 [Tor Relay]: Build abstraction layer around TLS
#6465: Build abstraction layer around TLS
-----------------------+----------------------------------------------------
Reporter: andrea | Owner: andrea
Type: project | Status: needs_review
Priority: major | Milestone: Tor: 0.2.4.x-final
Component: Tor Relay | Version: Tor: unspecified
Keywords: | Parent:
Points: | Actualpoints:
-----------------------+----------------------------------------------------
Comment(by nickm):
PART 2:
A general thing: this branch doesn't actually compile for me. I get lots
of warnings like this:
{{{
src/or/channel.c:1595: warning: format â%luâ expects type âlong unsigned
intâ, but argument 7 has type âuint64_tâ
}}}
Please try building on a more recent GCC and/or a 64-bit platform and/or a
32-bit platform and/or with a recent clang -- whatever you haven't tried
yet.
Another general thing: I'm not done reading yet, but I'm a little nervous
about queues and buffers here. As I understand it, there will be 5 layers
where cells get queued:
* The circuit's queue of cells
* The channel's queue of cells [*]
* The or_connection_t's queue of data to encrypt (conn->outbuf)
* SSL's write buffer. (ssl->wbuf)
* The kernel's write buffer.
The one marked with [*] above is new; what does it give us that
conn->outbuf doesn't? And what are we doing to make sure it doesn't get
too big? We don't want it getting large, since we want to use the circuit
pqueue mechanism to choose an active circuit late. Filling buffers
aggressively here means that loud circuits fill the queue, and quiet low-
latency circuits don't get heard.
Last general thing: Some of these functions should probably take const
channel_t*, not channel_t*.
Other stuff, going on with my reading:
* In channel_process_incoming, do we want to process that queue in-order?
It seems that doing the DEL_CURRENT approach is likely to have weird
consequences. Perhaps it would be better to extract all the members into a
separate list (smartlist_add_all(), smartlist_clear()), then process the
separate list, then free it.
* Should channel_more_to_flush() check outgoing_queue rather than
cell_queue?
* Should cell_queue be renamed incoming_queue for consistency? I think it
should.
* In channel_process_cells, I feel like maybe we're going to hit trouble
if there isn't a way to break out in the middle of processing all those
cells? I guess that might be happening in the middle of the processing
functions, where they note if there's an error and don't process anything
if an error has occurred.
* It seems like channel_process_cells, channel_queue_cell, and
channel_queue_var_cell have some common code that should be extracted.
* In channel_send_destroy, I am confused about why we're calling
channel_write_cell instead of channel_queue_cell. In fact, it's likely
that I'm going to have this confusion about every
channel_write_cell/channel_queue_cell instance. Hmm. I wonder if there's
any way we can make those more distinct/obvious/safe?
* I'm confused about why channel_send_destroy() is here, but there aren't
other channel_send_foo() functions.
* The first part of channel_free_all() seems redundant with
channel_run_cleanup().
* Typo in comment in channel.h : "see thw channel_t"
* Crazy idea: How badly, if at all, would we break things by making the
channel_s definition internal so that only channel.c (and later others if
needed) was allowed to look at it? It's not how we do most stuff right
now, but I bet it would make us happier in the long run.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/6465#comment:22>
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