[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):
General notes as I read. There will likely be a few of these as this is a
big patch. I'm reading though the _squashed branch with git log -p
--reverse, so if I say something now that a later commit in that branch
will make nonsensical, that's probably why.
PART 1:
* Functions like channel_register probably want to note not only what
they do, but when you must call them. Similarly, when there's a field in
a data structure that shouldn't ever be changed except by a particular
group of functions, that field's documentation should say so.
* Your doxygen usage style isn't the one we've been using. Some stuff
(like using @param and @return) is cool and we should consider whether we
want to do that more. But some stuff (like dropping the imperative-
sentence) style shouldn't proliferate. This doesn't need a cleanup before
we can merge this branch, but should get made more uniform sooner or later
once we decide what to do.
* You're doing a printf of global_identifier using the %lu format. That
won't work on systems where long isn't 64 bits, right? To format a uint64
portably, you need to do the dance with U64_FORMAT and U64_PRINTF_ARG.
* channel_remove_from_digest_map is very very complex. Can anything be
done to make it simpler? For instance, could the error-case code get
extracted into a separate function or something?
* Assignments like
"chan->u.cell_chan.prev_with_same_id->u.cell_chan.next_with_same_id =
chan->u.cell_chan.next_with_same_id;" are pretty verbose and make me
wonder whether we've got some lack-of-genericity problem. Not sure if we
need to solve them now or what the right solution is.
* I don't see the point of the rv=tmp dance in
channel_find_by_remote_digest.
* channel_find_by_remote_nickname really shouldn't exist: it shouldn't be
necessary to look up a channel by the peer's nickname, since nicknames are
pretty fragile. (I note that it seems to have no callers.
* Style issue: you seem to like single-point of return a little more than
me. That's fine; but please don't think you need to do it on my account.
For instance, channel_next_with_digest could probably have its body
replaced with the two asserts and "return
chan->u.cell_chan.next_with_same_id"
* Should listener channels be a different type entirely from cell
channels? They seem to have very little in common. IOW, so far in my
reading, it looks like there aren't a lot of the same operations that it
makes sense to invoke on one as on another, or a lot of code that makes
sense to share. There are also a lot of functions that have to assert
that the channel_t they got is really one or another type of channel_t,
which also suggests that using the type system here could be right. I
also get this impression from looking at the data structure.
* By convention, it's always a no-op to pass NULL to any of Tor's
foo_free() functions. channel_free() doesn't follow this convention.
* If channel_force_free() should only be called from channel_free_all(),
should it be static?
* In channel_free, we free the active_circuit_pqueue smartlist, but don't
do anything to its members. Should it be documented what does mark/free
them?
* In channel_force_free(), we tor_free one or more cell_queue_entry_t
items, which is presumably done elsewhere for cases where we would hit
channel_free(). Can/should that be extracted into its own function?
* When we're freeing cell_queue_entry_t items in that function, what
frees the underlying cell_t/var_cell_t/packed_cell_t ? Maybe there should
be a cell_queue_entry_free() function
* Doing a SMARTLIST_DEL_CURRENT() when freeing items in a list that
you're about to smartlist_free() is usually unnecessary.
* re channel_get_listener: For functions that return function pointers, I
strongly prefer to have a typedef for the function type so as to avoid the
Trickiest Syntax In C. Also, we're using the word "listener" here both to
mean a kind of a channel and a function. Maybe the function that returns
the function should be called channel_get_listener_fn() or
channel_get_listener_cb() or channel_get_listener_handler() or something?
* Let's eliminate every function to set the channel cell handlers other
than channel_set_cell_handlers(). That's the only one we currently use,
and I don't see why you'd need the other two. As they stand, they're
mostly duplicated code. If we need them later, we can reimplement them by
calling e.g. channel_set_sell_handlers(chan,
channel_get_old_cell_handler(chan), new_var_cell_handler);
* By convention, channel_request_close() seems like what we would name
channel_mark_for_close. Is it different enough from the existing
mark_for_close() functions that we should keep its current name?
* In channel_closed(), we only call "circuit_n_chan_done(chan, 0);" in
one of the close paths. What tells those circuits that the channel is
closing otherwise? Or am I missing something?
* channel_clear_identity_digest makes me kind of wish that we had a bit
on the channel_t to tell us whether the channel should be in the id-digest
map. Looking at states like this feels a little fragile.
* re channel_write_cell: As I understand it, we have one function here
that'll want to queue a cell, and another that will want to deliver it.
From the documentation, I can't tell which one this function is.
* In channel_write_cell: It looks like there are only two possible return
values from write_cell looking at this function's use of it. I would have
at least expected "I could write it" "I couldn't write it", and "I got an
error." Must investigate later.
* Using a smartlist of cells for an outgoing queue makes me a little
nervous. Popping the first item from this queue is going to be O(N).
* Probably there should be a cell_queue_entry_new() function [or family
of functions] to allocate cell_queue_entry_t objects.
* Does connection_write_cell() potentially double-flush? That is, can it
try to call write_cell, then call channel_flush_cells() on the same cell
even if the first write didn't work? If so, is that a problem?
* The channel_write*cell() functions are mostly duplicate code with each
other. That needs to get fixed; there should be one implementation
function here and two-three wrappers, I think.
* The comment "+ /* That shouldn't happen; bail out */" in
channel_flush_some_cells_from_outgoing_queue seems orphaned?
* The swtich statement in channel_flush_some_cells_from_outgoing_queue
seems like it could stand to be refactored into its own function, maybe
with some of the common code between the different cases eliminated.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/6465#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