December 2017 - tor-commits - lists.torproject.org

[tor/master] relay: Improve comment in append_cell_to_circuit_queue()
by nickm＠torproject.org 08 Dec '17

08 Dec '17

commit d165f0fd30bc9823152dad6769afab0afae2ea6d Author: David Goulet <dgoulet(a)torproject.org> Date: Mon Nov 20 15:53:25 2017 -0500 relay: Improve comment in append_cell_to_circuit_queue() This function is part of the tor fast path so this commit adds more documentation to it as it is critical. Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/or/relay.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/or/relay.c b/src/or/relay.c index eb9d03080..d6c103c14 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -2859,7 +2859,12 @@ channel_flush_from_first_active_circuit, (channel_t *chan, int max)) } /** Add cell to the queue of circ writing to chan - * transmitting in direction. */ + * transmitting in direction. + * + * The given cell is copied over the circuit queue so the caller must + * cleanup the memory. + * + * This function is part of the fast path. */ void append_cell_to_circuit_queue(circuit_t *circ, channel_t *chan, cell_t *cell, cell_direction_t direction, @@ -2882,11 +2887,14 @@ append_cell_to_circuit_queue(circuit_t *circ, channel_t *chan, streams_blocked = circ->streams_blocked_on_p_chan; } + /* Very important that we copy to the circuit queue because all calls to + * this function use the stack for the cell memory. */ cell_queue_append_packed_copy(circ, queue, exitward, cell, chan->wide_circ_ids, 1); + /* Check and run the OOM if needed. */ if (PREDICT_UNLIKELY(cell_queues_check_size())) { - /* We ran the OOM handler */ + /* We ran the OOM handler which might have closed this circuit. */ if (circ->marked_for_close) return; }

1 0

[tor/master] channel: Remove incoming/outgoing queue
by nickm＠torproject.org 08 Dec '17

08 Dec '17

commit 46a0709261471fe7730b1c01351e216a2dbaf785 Author: David Goulet <dgoulet(a)torproject.org> Date: Mon Nov 20 16:44:16 2017 -0500 channel: Remove incoming/outgoing queue For the rationale, see ticket #23709. This is a pretty massive commit. Those queues were everywhere in channel.c and it turns out that it was used by lots of dead code. The channel subsystem *never* handles variable size cell (var_cell_t) or unpacked cells (cell_t). The variable ones are only handled in channeltls and outbound cells are always packed from the circuit queue so this commit removes code related to variable and unpacked cells. However, inbound cells are unpacked (cell_t), that is untouched and is handled via channel_process_cell() function. In order to make the commit compile, test have been modified but not passing at this commit. Also, many tests have been removed but better improved ones get added in future commits. This commit also adds a XXX: which indicates that the handling process of outbound cells isn't fully working. This as well is fixed in a future commit. Finally, at this commit, more dead code remains, it will be cleanup in future commits. Fixes #23709 Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/or/channel.c | 678 ++-------------------------------------------- src/or/channel.h | 13 +- src/or/channeltls.c | 2 +- src/test/test_channel.c | 489 --------------------------------- src/test/test_scheduler.c | 4 - 5 files changed, 22 insertions(+), 1164 deletions(-) diff --git a/src/or/channel.c b/src/or/channel.c index 0b5a7fde9..6f6bd8a7e 100644 --- a/src/or/channel.c +++ b/src/or/channel.c @@ -201,14 +201,6 @@ HT_PROTOTYPE(channel_idmap, channel_idmap_entry_s, node, channel_idmap_hash, HT_GENERATE2(channel_idmap, channel_idmap_entry_s, node, channel_idmap_hash, channel_idmap_eq, 0.5, tor_reallocarray_, tor_free_) -static cell_queue_entry_t * cell_queue_entry_dup(cell_queue_entry_t *q); -#if 0 -static int cell_queue_entry_is_padding(cell_queue_entry_t *q); -#endif -static cell_queue_entry_t * -cell_queue_entry_new_fixed(cell_t *cell); -static cell_queue_entry_t * -cell_queue_entry_new_var(var_cell_t *var_cell); static int is_destroy_cell(channel_t *chan, const cell_queue_entry_t *q, circid_t *circid_out); @@ -218,13 +210,6 @@ static void channel_assert_counter_consistency(void); static void channel_add_to_digest_map(channel_t *chan); static void channel_remove_from_digest_map(channel_t *chan); -/* - * Flush cells from just the outgoing queue without trying to get them - * from circuits; used internall by channel_flush_some_cells(). - */ -static ssize_t -channel_flush_some_cells_from_outgoing_queue(channel_t *chan, - ssize_t num_cells); static void channel_force_free(channel_t *chan); static void channel_free_list(smartlist_t *channels, int mark_for_close); @@ -936,10 +921,6 @@ channel_init(channel_t *chan) /* Warn about exhausted circuit IDs no more than hourly. */ chan->last_warned_circ_ids_exhausted.rate = 3600; - /* Initialize queues. */ - TOR_SIMPLEQ_INIT(&chan->incoming_queue); - TOR_SIMPLEQ_INIT(&chan->outgoing_queue); - /* Initialize list entries. */ memset(&chan->next_with_same_id, 0, sizeof(chan->next_with_same_id)); @@ -1069,7 +1050,6 @@ channel_listener_free(channel_listener_t *chan_l) static void channel_force_free(channel_t *chan) { - cell_queue_entry_t *cell, *cell_tmp; tor_assert(chan); log_debug(LD_CHANNEL, @@ -1103,18 +1083,6 @@ channel_force_free(channel_t *chan) chan->cmux = NULL; } - /* We might still have a cell queue; kill it */ - TOR_SIMPLEQ_FOREACH_SAFE(cell, &chan->incoming_queue, next, cell_tmp) { - cell_queue_entry_free(cell, 0); - } - TOR_SIMPLEQ_INIT(&chan->incoming_queue); - - /* Outgoing cell queue is similar, but we can have to free packed cells */ - TOR_SIMPLEQ_FOREACH_SAFE(cell, &chan->outgoing_queue, next, cell_tmp) { - cell_queue_entry_free(cell, 0); - } - TOR_SIMPLEQ_INIT(&chan->outgoing_queue); - tor_free(chan); } @@ -1247,8 +1215,6 @@ channel_set_cell_handlers(channel_t *chan, channel_var_cell_handler_fn_ptr var_cell_handler) { - int try_again = 0; - tor_assert(chan); tor_assert(CHANNEL_CAN_HANDLE_CELLS(chan)); @@ -1259,21 +1225,9 @@ channel_set_cell_handlers(channel_t *chan, "Setting var_cell_handler callback for channel %p to %p", chan, var_cell_handler); - /* Should we try the queue? */ - if (cell_handler && - cell_handler != chan->cell_handler) try_again = 1; - if (var_cell_handler && - var_cell_handler != chan->var_cell_handler) try_again = 1; - /* Change them */ chan->cell_handler = cell_handler; chan->var_cell_handler = var_cell_handler; - - /* Re-run the queue if we have one and there's any reason to */ - if (! TOR_SIMPLEQ_EMPTY(&chan->incoming_queue) && - try_again && - (chan->cell_handler || - chan->var_cell_handler)) channel_process_cells(chan); } /* @@ -1730,147 +1684,6 @@ channel_set_remote_end(channel_t *chan, } /** - * Duplicate a cell queue entry; this is a shallow copy intended for use - * in channel_write_cell_queue_entry(). - */ - -static cell_queue_entry_t * -cell_queue_entry_dup(cell_queue_entry_t *q) -{ - cell_queue_entry_t *rv = NULL; - - tor_assert(q); - - rv = tor_malloc(sizeof(*rv)); - memcpy(rv, q, sizeof(*rv)); - - return rv; -} - -/** - * Free a cell_queue_entry_t; the handed_off parameter indicates whether - * the contents were passed to the lower layer (it is responsible for - * them) or not (we should free). - */ - -STATIC void -cell_queue_entry_free(cell_queue_entry_t *q, int handed_off) -{ - if (!q) return; - - if (!handed_off) { - /* - * If we handed it off, the recipient becomes responsible (or - * with packed cells the channel_t subclass calls packed_cell - * free after writing out its contents; see, e.g., - * channel_tls_write_packed_cell_method(). Otherwise, we have - * to take care of it here if possible. - */ - switch (q->type) { - case CELL_QUEUE_FIXED: - if (q->u.fixed.cell) { - /* - * There doesn't seem to be a cell_free() function anywhere in the - * pre-channel code; just use tor_free() - */ - tor_free(q->u.fixed.cell); - } - break; - case CELL_QUEUE_PACKED: - if (q->u.packed.packed_cell) { - packed_cell_free(q->u.packed.packed_cell); - } - break; - case CELL_QUEUE_VAR: - if (q->u.var.var_cell) { - /* - * This one's in connection_or.c; it'd be nice to figure out the - * whole flow of cells from one end to the other and factor the - * cell memory management functions like this out of the specific - * TLS lower layer. - */ - var_cell_free(q->u.var.var_cell); - } - break; - default: - /* - * Nothing we can do if we don't know the type; this will - * have been warned about elsewhere. - */ - break; - } - } - tor_free(q); -} - -#if 0 -/** - * Check whether a cell queue entry is padding; this is a helper function - * for channel_write_cell_queue_entry() - */ - -static int -cell_queue_entry_is_padding(cell_queue_entry_t *q) -{ - tor_assert(q); - - if (q->type == CELL_QUEUE_FIXED) { - if (q->u.fixed.cell) { - if (q->u.fixed.cell->command == CELL_PADDING || - q->u.fixed.cell->command == CELL_VPADDING) { - return 1; - } - } - } else if (q->type == CELL_QUEUE_VAR) { - if (q->u.var.var_cell) { - if (q->u.var.var_cell->command == CELL_PADDING || - q->u.var.var_cell->command == CELL_VPADDING) { - return 1; - } - } - } - - return 0; -} -#endif /* 0 */ - -/** - * Allocate a new cell queue entry for a fixed-size cell - */ - -static cell_queue_entry_t * -cell_queue_entry_new_fixed(cell_t *cell) -{ - cell_queue_entry_t *q = NULL; - - tor_assert(cell); - - q = tor_malloc(sizeof(*q)); - q->type = CELL_QUEUE_FIXED; - q->u.fixed.cell = cell; - - return q; -} - -/** - * Allocate a new cell queue entry for a variable-size cell - */ - -static cell_queue_entry_t * -cell_queue_entry_new_var(var_cell_t *var_cell) -{ - cell_queue_entry_t *q = NULL; - - tor_assert(var_cell); - - q = tor_malloc(sizeof(*q)); - q->type = CELL_QUEUE_VAR; - q->u.var.var_cell = var_cell; - - return q; -} - -/** * Ask how big the cell contained in a cell_queue_entry_t is */ @@ -1910,8 +1723,7 @@ channel_get_cell_queue_entry_size(channel_t *chan, cell_queue_entry_t *q) static void channel_write_cell_queue_entry(channel_t *chan, cell_queue_entry_t *q) { - int result = 0, sent = 0; - cell_queue_entry_t *tmp = NULL; + int result = 0; size_t cell_bytes; tor_assert(chan); @@ -1931,8 +1743,7 @@ channel_write_cell_queue_entry(channel_t *chan, cell_queue_entry_t *q) cell_bytes = channel_get_cell_queue_entry_size(chan, q); /* Can we send it right out? If so, try */ - if (TOR_SIMPLEQ_EMPTY(&chan->outgoing_queue) && - CHANNEL_IS_OPEN(chan)) { + if (CHANNEL_IS_OPEN(chan)) { /* Pick the right write function for this cell type and save the result */ switch (q->type) { case CELL_QUEUE_FIXED: @@ -1956,7 +1767,6 @@ channel_write_cell_queue_entry(channel_t *chan, cell_queue_entry_t *q) /* Check if we got it out */ if (result > 0) { - sent = 1; /* Timestamp for transmission */ channel_timestamp_xmit(chan); /* If we're here the queue is empty, so it's drained too */ @@ -1973,25 +1783,8 @@ channel_write_cell_queue_entry(channel_t *chan, cell_queue_entry_t *q) } } - if (!sent) { - /* Not sent, queue it */ - /* - * We have to copy the queue entry passed in, since the caller probably - * used the stack. - */ - tmp = cell_queue_entry_dup(q); - TOR_SIMPLEQ_INSERT_TAIL(&chan->outgoing_queue, tmp, next); - /* Update global counters */ - ++n_channel_cells_queued; - ++n_channel_cells_in_queues; - n_channel_bytes_queued += cell_bytes; - n_channel_bytes_in_queues += cell_bytes; - channel_assert_counter_consistency(); - /* Update channel queue size */ - chan->bytes_in_queue += cell_bytes; - /* Try to process the queue? */ - if (CHANNEL_IS_OPEN(chan)) channel_flush_cells(chan); - } + /* XXX: If the cell wasn't sent, we need to propagate the error back so we + * can put it back in the circuit queue. */ } /** Write a generic cell type to a channel @@ -2207,13 +2000,6 @@ channel_change_state_(channel_t *chan, channel_state_t to_state) from_state == CHANNEL_STATE_MAINT)) { estimated_total_queue_size += chan->bytes_in_queue; } - - if (to_state == CHANNEL_STATE_CLOSED || - to_state == CHANNEL_STATE_ERROR) { - /* Assert that all queues are empty */ - tor_assert(TOR_SIMPLEQ_EMPTY(&chan->incoming_queue)); - tor_assert(TOR_SIMPLEQ_EMPTY(&chan->outgoing_queue)); - } } /** @@ -2237,12 +2023,6 @@ channel_change_state_open(channel_t *chan) /* Tell circuits if we opened and stuff */ channel_do_open_actions(chan); chan->has_been_open = 1; - - /* Check for queued cells to process */ - if (! TOR_SIMPLEQ_EMPTY(&chan->incoming_queue)) - channel_process_cells(chan); - if (! TOR_SIMPLEQ_EMPTY(&chan->outgoing_queue)) - channel_flush_cells(chan); } /** @@ -2347,8 +2127,7 @@ channel_flush_some_cells, (channel_t *chan, ssize_t num_cells)) { unsigned int unlimited = 0; ssize_t flushed = 0; - int num_cells_from_circs, clamped_num_cells; - int q_len_before, q_len_after; + int clamped_num_cells; tor_assert(chan); @@ -2357,11 +2136,6 @@ channel_flush_some_cells, (channel_t *chan, ssize_t num_cells)) /* If we aren't in CHANNEL_STATE_OPEN, nothing goes through */ if (CHANNEL_IS_OPEN(chan)) { - /* Try to flush as much as we can that's already queued */ - flushed += channel_flush_some_cells_from_outgoing_queue(chan, - (unlimited ? -1 : num_cells - flushed)); - if (!unlimited && num_cells <= flushed) goto done; - if (circuitmux_num_cells(chan->cmux) > 0) { /* Calculate number of cells, including clamp */ if (unlimited) { @@ -2375,45 +2149,9 @@ channel_flush_some_cells, (channel_t *chan, ssize_t num_cells)) } } - /* - * Keep track of the change in queue size; we have to count cells - * channel_flush_from_first_active_circuit() writes out directly, - * but not double-count ones we might get later in - * channel_flush_some_cells_from_outgoing_queue() - */ - q_len_before = chan_cell_queue_len(&(chan->outgoing_queue)); - /* Try to get more cells from any active circuits */ - num_cells_from_circs = channel_flush_from_first_active_circuit( + flushed = channel_flush_from_first_active_circuit( chan, clamped_num_cells); - - q_len_after = chan_cell_queue_len(&(chan->outgoing_queue)); - - /* - * If it claims we got some, adjust the flushed counter and consider - * processing the queue again - */ - if (num_cells_from_circs > 0) { - /* - * Adjust flushed by the number of cells counted in - * num_cells_from_circs that didn't go to the cell queue. - */ - - if (q_len_after > q_len_before) { - num_cells_from_circs -= (q_len_after - q_len_before); - if (num_cells_from_circs < 0) num_cells_from_circs = 0; - } - - flushed += num_cells_from_circs; - - /* Now process the queue if necessary */ - - if ((q_len_after > q_len_before) && - (unlimited || (flushed < num_cells))) { - flushed += channel_flush_some_cells_from_outgoing_queue(chan, - (unlimited ? -1 : num_cells - flushed)); - } - } } } @@ -2422,181 +2160,6 @@ channel_flush_some_cells, (channel_t *chan, ssize_t num_cells)) } /** - * Flush cells from just the channel's outgoing cell queue - * - * This gets called from channel_flush_some_cells() above to flush cells - * just from the queue without trying for active_circuits. - */ - -static ssize_t -channel_flush_some_cells_from_outgoing_queue(channel_t *chan, - ssize_t num_cells) -{ - unsigned int unlimited = 0; - ssize_t flushed = 0; - cell_queue_entry_t *q = NULL; - size_t cell_size; - int free_q = 0, handed_off = 0; - - tor_assert(chan); - tor_assert(chan->write_cell); - tor_assert(chan->write_packed_cell); - tor_assert(chan->write_var_cell); - - if (num_cells < 0) unlimited = 1; - if (!unlimited && num_cells <= flushed) return 0; - - /* If we aren't in CHANNEL_STATE_OPEN, nothing goes through */ - if (CHANNEL_IS_OPEN(chan)) { - while ((unlimited || num_cells > flushed) && - NULL != (q = TOR_SIMPLEQ_FIRST(&chan->outgoing_queue))) { - free_q = 0; - handed_off = 0; - - /* Figure out how big it is for statistical purposes */ - cell_size = channel_get_cell_queue_entry_size(chan, q); - /* - * Okay, we have a good queue entry, try to give it to the lower - * layer. - */ - switch (q->type) { - case CELL_QUEUE_FIXED: - if (q->u.fixed.cell) { - if (chan->write_cell(chan, - q->u.fixed.cell)) { - ++flushed; - channel_timestamp_xmit(chan); - ++(chan->n_cells_xmitted); - chan->n_bytes_xmitted += cell_size; - free_q = 1; - handed_off = 1; - } - /* Else couldn't write it; leave it on the queue */ - } else { - /* This shouldn't happen */ - log_info(LD_CHANNEL, - "Saw broken cell queue entry of type CELL_QUEUE_FIXED " - "with no cell on channel %p " - "(global ID " U64_FORMAT ").", - chan, U64_PRINTF_ARG(chan->global_identifier)); - /* Throw it away */ - free_q = 1; - handed_off = 0; - } - break; - case CELL_QUEUE_PACKED: - if (q->u.packed.packed_cell) { - if (chan->write_packed_cell(chan, - q->u.packed.packed_cell)) { - ++flushed; - channel_timestamp_xmit(chan); - ++(chan->n_cells_xmitted); - chan->n_bytes_xmitted += cell_size; - free_q = 1; - handed_off = 1; - } - /* Else couldn't write it; leave it on the queue */ - } else { - /* This shouldn't happen */ - log_info(LD_CHANNEL, - "Saw broken cell queue entry of type CELL_QUEUE_PACKED " - "with no cell on channel %p " - "(global ID " U64_FORMAT ").", - chan, U64_PRINTF_ARG(chan->global_identifier)); - /* Throw it away */ - free_q = 1; - handed_off = 0; - } - break; - case CELL_QUEUE_VAR: - if (q->u.var.var_cell) { - if (chan->write_var_cell(chan, - q->u.var.var_cell)) { - ++flushed; - channel_timestamp_xmit(chan); - ++(chan->n_cells_xmitted); - chan->n_bytes_xmitted += cell_size; - free_q = 1; - handed_off = 1; - } - /* Else couldn't write it; leave it on the queue */ - } else { - /* This shouldn't happen */ - log_info(LD_CHANNEL, - "Saw broken cell queue entry of type CELL_QUEUE_VAR " - "with no cell on channel %p " - "(global ID " U64_FORMAT ").", - chan, U64_PRINTF_ARG(chan->global_identifier)); - /* Throw it away */ - free_q = 1; - handed_off = 0; - } - break; - default: - /* Unknown type, log and free it */ - log_info(LD_CHANNEL, - "Saw an unknown cell queue entry type %d on channel %p " - "(global ID " U64_FORMAT "; ignoring it." - " Someone should fix this.", - q->type, chan, U64_PRINTF_ARG(chan->global_identifier)); - free_q = 1; - handed_off = 0; - } - - /* - * if free_q is set, we used it and should remove the queue entry; - * we have to do the free down here so TOR_SIMPLEQ_REMOVE_HEAD isn't - * accessing freed memory - */ - if (free_q) { - TOR_SIMPLEQ_REMOVE_HEAD(&chan->outgoing_queue, next); - /* - * ...and we handed a cell off to the lower layer, so we should - * update the counters. - */ - ++n_channel_cells_passed_to_lower_layer; - --n_channel_cells_in_queues; - n_channel_bytes_passed_to_lower_layer += cell_size; - n_channel_bytes_in_queues -= cell_size; - channel_assert_counter_consistency(); - /* Update the channel's queue size too */ - chan->bytes_in_queue -= cell_size; - /* Finally, free q */ - cell_queue_entry_free(q, handed_off); - q = NULL; - } else { - /* No cell removed from list, so we can't go on any further */ - break; - } - } - } - - /* Did we drain the queue? */ - if (TOR_SIMPLEQ_EMPTY(&chan->outgoing_queue)) { - channel_timestamp_drained(chan); - } - - /* Update the estimate queue size */ - channel_update_xmit_queue_size(chan); - - return flushed; -} - -/** - * Flush as many cells as we possibly can from the queue - * - * This tries to flush as many cells from the queue as the lower layer - * will take. It just calls channel_flush_some_cells_from_outgoing_queue() - * in unlimited mode. - */ - -void -channel_flush_cells(channel_t *chan) -{ - channel_flush_some_cells_from_outgoing_queue(chan, -1); -} - -/** * Check if any cells are available * * This gets used from the lower layer to check if any more cells are @@ -2608,10 +2171,6 @@ channel_more_to_flush, (channel_t *chan)) { tor_assert(chan); - /* Check if we have any queued */ - if (! TOR_SIMPLEQ_EMPTY(&chan->incoming_queue)) - return 1; - /* Check if any circuits would like to queue some */ if (circuitmux_num_cells(chan->cmux) > 0) return 1; @@ -2823,200 +2382,37 @@ channel_listener_queue_incoming(channel_listener_t *listener, */ void -channel_process_cells(channel_t *chan) +channel_process_cell(channel_t *chan, cell_t *cell) { - cell_queue_entry_t *q; tor_assert(chan); tor_assert(CHANNEL_IS_CLOSING(chan) || CHANNEL_IS_MAINT(chan) || CHANNEL_IS_OPEN(chan)); - - log_debug(LD_CHANNEL, - "Processing as many incoming cells as we can for channel %p", - chan); + tor_assert(cell); /* Nothing we can do if we have no registered cell handlers */ - if (!(chan->cell_handler || - chan->var_cell_handler)) return; - /* Nothing we can do if we have no cells */ - if (TOR_SIMPLEQ_EMPTY(&chan->incoming_queue)) return; + if (!chan->cell_handler) + return; /* - * Process cells until we're done or find one we have no current handler - * for. + * Process the given cell * * We must free the cells here after calling the handler, since custody * of the buffer was given to the channel layer when they were queued; * see comments on memory management in channel_queue_cell() and in * channel_queue_var_cell() below. */ - while (NULL != (q = TOR_SIMPLEQ_FIRST(&chan->incoming_queue))) { - tor_assert(q); - tor_assert(q->type == CELL_QUEUE_FIXED || - q->type == CELL_QUEUE_VAR); - - if (q->type == CELL_QUEUE_FIXED && - chan->cell_handler) { - /* Handle a fixed-length cell */ - TOR_SIMPLEQ_REMOVE_HEAD(&chan->incoming_queue, next); - tor_assert(q->u.fixed.cell); - log_debug(LD_CHANNEL, - "Processing incoming cell_t %p for channel %p (global ID " - U64_FORMAT ")", - q->u.fixed.cell, chan, - U64_PRINTF_ARG(chan->global_identifier)); - chan->cell_handler(chan, q->u.fixed.cell); - tor_free(q->u.fixed.cell); - tor_free(q); - } else if (q->type == CELL_QUEUE_VAR && - chan->var_cell_handler) { - /* Handle a variable-length cell */ - TOR_SIMPLEQ_REMOVE_HEAD(&chan->incoming_queue, next); - tor_assert(q->u.var.var_cell); - log_debug(LD_CHANNEL, - "Processing incoming var_cell_t %p for channel %p (global ID " - U64_FORMAT ")", - q->u.var.var_cell, chan, - U64_PRINTF_ARG(chan->global_identifier)); - chan->var_cell_handler(chan, q->u.var.var_cell); - tor_free(q->u.var.var_cell); - tor_free(q); - } else { - /* Can't handle this one */ - break; - } - } -} - -/** - * Queue incoming cell - * - * This should be called by a channel_t subclass to queue an incoming fixed- - * length cell for processing, and process it if possible. - */ - -void -channel_queue_cell(channel_t *chan, cell_t *cell) -{ - int need_to_queue = 0; - cell_queue_entry_t *q; - cell_t *cell_copy = NULL; - - tor_assert(chan); - tor_assert(cell); - tor_assert(CHANNEL_IS_OPEN(chan)); - - /* Do we need to queue it, or can we just call the handler right away? */ - if (!(chan->cell_handler)) need_to_queue = 1; - if (! TOR_SIMPLEQ_EMPTY(&chan->incoming_queue)) - need_to_queue = 1; /* Timestamp for receiving */ channel_timestamp_recv(chan); - - /* Update the counters */ + /* Update received counter. */ ++(chan->n_cells_recved); chan->n_bytes_recved += get_cell_network_size(chan->wide_circ_ids); - /* If we don't need to queue we can just call cell_handler */ - if (!need_to_queue) { - tor_assert(chan->cell_handler); - log_debug(LD_CHANNEL, - "Directly handling incoming cell_t %p for channel %p " - "(global ID " U64_FORMAT ")", - cell, chan, - U64_PRINTF_ARG(chan->global_identifier)); - chan->cell_handler(chan, cell); - } else { - /* - * Otherwise queue it and then process the queue if possible. - * - * We queue a copy, not the original pointer - it might have been on the - * stack in connection_or_process_cells_from_inbuf() (or another caller - * if we ever have a subclass other than channel_tls_t), or be freed - * there after we return. This is the uncommon case; the non-copying - * fast path occurs in the if (!need_to_queue) case above when the - * upper layer has installed cell handlers. - */ - cell_copy = tor_malloc_zero(sizeof(cell_t)); - memcpy(cell_copy, cell, sizeof(cell_t)); - q = cell_queue_entry_new_fixed(cell_copy); - log_debug(LD_CHANNEL, - "Queueing incoming cell_t %p for channel %p " - "(global ID " U64_FORMAT ")", - cell, chan, - U64_PRINTF_ARG(chan->global_identifier)); - TOR_SIMPLEQ_INSERT_TAIL(&chan->incoming_queue, q, next); - if (chan->cell_handler || - chan->var_cell_handler) { - channel_process_cells(chan); - } - } -} - -/** - * Queue incoming variable-length cell - * - * This should be called by a channel_t subclass to queue an incoming - * variable-length cell for processing, and process it if possible. - */ - -void -channel_queue_var_cell(channel_t *chan, var_cell_t *var_cell) -{ - int need_to_queue = 0; - cell_queue_entry_t *q; - var_cell_t *cell_copy = NULL; - - tor_assert(chan); - tor_assert(var_cell); - tor_assert(CHANNEL_IS_OPEN(chan)); - - /* Do we need to queue it, or can we just call the handler right away? */ - if (!(chan->var_cell_handler)) need_to_queue = 1; - if (! TOR_SIMPLEQ_EMPTY(&chan->incoming_queue)) - need_to_queue = 1; - - /* Timestamp for receiving */ - channel_timestamp_recv(chan); - - /* Update the counter */ - ++(chan->n_cells_recved); - chan->n_bytes_recved += get_var_cell_header_size(chan->wide_circ_ids) + - var_cell->payload_len; - - /* If we don't need to queue we can just call cell_handler */ - if (!need_to_queue) { - tor_assert(chan->var_cell_handler); - log_debug(LD_CHANNEL, - "Directly handling incoming var_cell_t %p for channel %p " - "(global ID " U64_FORMAT ")", - var_cell, chan, - U64_PRINTF_ARG(chan->global_identifier)); - chan->var_cell_handler(chan, var_cell); - } else { - /* - * Otherwise queue it and then process the queue if possible. - * - * We queue a copy, not the original pointer - it might have been on the - * stack in connection_or_process_cells_from_inbuf() (or another caller - * if we ever have a subclass other than channel_tls_t), or be freed - * there after we return. This is the uncommon case; the non-copying - * fast path occurs in the if (!need_to_queue) case above when the - * upper layer has installed cell handlers. - */ - cell_copy = var_cell_copy(var_cell); - q = cell_queue_entry_new_var(cell_copy); - log_debug(LD_CHANNEL, - "Queueing incoming var_cell_t %p for channel %p " - "(global ID " U64_FORMAT ")", - var_cell, chan, - U64_PRINTF_ARG(chan->global_identifier)); - TOR_SIMPLEQ_INSERT_TAIL(&chan->incoming_queue, q, next); - if (chan->cell_handler || - chan->var_cell_handler) { - channel_process_cells(chan); - } - } + log_debug(LD_CHANNEL, + "Processing incoming cell_t %p for channel %p (global ID " + U64_FORMAT ")", cell, chan, + U64_PRINTF_ARG(chan->global_identifier)); + chan->cell_handler(chan, cell); } /** If packed_cell on chan is a destroy cell, then set @@ -3629,19 +3025,6 @@ channel_listener_describe_transport(channel_listener_t *chan_l) } /** - * Return the number of entries in queue - */ -STATIC int -chan_cell_queue_len(const chan_cell_queue_t *queue) -{ - int r = 0; - cell_queue_entry_t *cell; - TOR_SIMPLEQ_FOREACH(cell, queue, next) - ++r; - return r; -} - -/** * Dump channel statistics * * Dump statistics for one channel to the log @@ -3753,14 +3136,6 @@ channel_dump_statistics, (channel_t *chan, int severity)) channel_is_incoming(chan) ? "incoming" : "outgoing"); - /* Describe queues */ - tor_log(severity, LD_GENERAL, - " * Channel " U64_FORMAT " has %d queued incoming cells" - " and %d queued outgoing cells", - U64_PRINTF_ARG(chan->global_identifier), - chan_cell_queue_len(&chan->incoming_queue), - chan_cell_queue_len(&chan->outgoing_queue)); - /* Describe circuits */ tor_log(severity, LD_GENERAL, " * Channel " U64_FORMAT " has %d active circuits out of" @@ -4037,19 +3412,11 @@ channel_get_addr_if_possible(channel_t *chan, tor_addr_t *addr_out) int channel_has_queued_writes(channel_t *chan) { - int has_writes = 0; - tor_assert(chan); tor_assert(chan->has_queued_writes); - if (! TOR_SIMPLEQ_EMPTY(&chan->outgoing_queue)) { - has_writes = 1; - } else { - /* Check with the lower layer */ - has_writes = chan->has_queued_writes(chan); - } - - return has_writes; + /* Check with the lower layer */ + return chan->has_queued_writes(chan); } /** @@ -4286,11 +3653,8 @@ channel_get_global_queue_estimate(void) /* * Estimate the number of writeable cells * - * Ask the lower layer for an estimate of how many cells it can accept, and - * then subtract the length of our outgoing_queue, if any, to produce an - * estimate of the number of cells this channel can accept for writes. + * Ask the lower layer for an estimate of how many cells it can accept. */ - int channel_num_cells_writeable(channel_t *chan) { @@ -4302,8 +3666,6 @@ channel_num_cells_writeable(channel_t *chan) if (chan->state == CHANNEL_STATE_OPEN) { /* Query lower layer */ result = chan->num_cells_writeable(chan); - /* Subtract cell queue length, if any */ - result -= chan_cell_queue_len(&chan->outgoing_queue); if (result < 0) result = 0; } else { /* No cells are writeable in any other state */ diff --git a/src/or/channel.h b/src/or/channel.h index 32336fe1d..d72f232bc 100644 --- a/src/or/channel.h +++ b/src/or/channel.h @@ -268,12 +268,6 @@ struct channel_s { */ TOR_LIST_ENTRY(channel_s) next_with_same_id; - /** List of incoming cells to handle */ - chan_cell_queue_t incoming_queue; - - /** List of queued outgoing cells */ - chan_cell_queue_t outgoing_queue; - /** Circuit mux for circuits sending on this channel */ circuitmux_t *cmux; @@ -480,11 +474,6 @@ struct cell_queue_entry_s { } u; }; -/* Cell queue functions for benefit of test suite */ -STATIC int chan_cell_queue_len(const chan_cell_queue_t *queue); - -STATIC void cell_queue_entry_free(cell_queue_entry_t *q, int handed_off); - void channel_write_cell_generic_(channel_t *chan, const char *cell_type, void *cell, cell_queue_entry_t *q); #endif /* defined(CHANNEL_PRIVATE_) */ @@ -556,7 +545,7 @@ void channel_listener_queue_incoming(channel_listener_t *listener, channel_t *incoming); /* Incoming cell handling */ -void channel_process_cells(channel_t *chan); +void channel_process_cell(channel_t *chan, cell_t *cell); void channel_queue_cell(channel_t *chan, cell_t *cell); void channel_queue_var_cell(channel_t *chan, var_cell_t *var_cell); diff --git a/src/or/channeltls.c b/src/or/channeltls.c index 827781318..e6ecc1538 100644 --- a/src/or/channeltls.c +++ b/src/or/channeltls.c @@ -1149,7 +1149,7 @@ channel_tls_handle_cell(cell_t *cell, or_connection_t *conn) * These are all transport independent and we pass them up through the * channel_t mechanism. They are ultimately handled in command.c. */ - channel_queue_cell(TLS_CHAN_TO_BASE(chan), cell); + channel_process_cell(TLS_CHAN_TO_BASE(chan), cell); break; default: log_fn(LOG_INFO, LD_PROTOCOL, diff --git a/src/test/test_channel.c b/src/test/test_channel.c index 023c2950c..28f84825c 100644 --- a/src/test/test_channel.c +++ b/src/test/test_channel.c @@ -32,16 +32,11 @@ static int test_dumpstats_calls = 0; static int test_has_waiting_cells_count = 0; static double test_overhead_estimate = 1.0; static int test_releases_count = 0; -static circuitmux_t *test_target_cmux = NULL; -static unsigned int test_cmux_cells = 0; static channel_t *dump_statistics_mock_target = NULL; static int dump_statistics_mock_matches = 0; static void chan_test_channel_dump_statistics_mock( channel_t *chan, int severity); -static int chan_test_channel_flush_from_first_active_circuit_mock( - channel_t *chan, int max); -static unsigned int chan_test_circuitmux_num_cells_mock(circuitmux_t *cmux); static void channel_note_destroy_not_pending_mock(channel_t *ch, circid_t circid); static void chan_test_cell_handler(channel_t *ch, @@ -64,12 +59,9 @@ static int chan_test_write_var_cell(channel_t *ch, var_cell_t *var_cell); static void scheduler_channel_doesnt_want_writes_mock(channel_t *ch); static void test_channel_dumpstats(void *arg); -static void test_channel_flush(void *arg); -static void test_channel_flushmux(void *arg); static void test_channel_incoming(void *arg); static void test_channel_lifecycle(void *arg); static void test_channel_multi(void *arg); -static void test_channel_queue_incoming(void *arg); static void test_channel_queue_size(void *arg); static void test_channel_write(void *arg); @@ -112,62 +104,6 @@ chan_test_channel_dump_statistics_mock(channel_t *chan, int severity) return; } -/** - * If the target cmux is the cmux for chan, make fake cells up to the - * target number of cells and write them to chan. Otherwise, invoke - * the real channel_flush_from_first_active_circuit(). - */ - -static int -chan_test_channel_flush_from_first_active_circuit_mock(channel_t *chan, - int max) -{ - int result = 0, c = 0; - packed_cell_t *cell = NULL; - - tt_ptr_op(chan, OP_NE, NULL); - if (test_target_cmux != NULL && - test_target_cmux == chan->cmux) { - while (c <= max && test_cmux_cells > 0) { - cell = packed_cell_new(); - channel_write_packed_cell(chan, cell); - ++c; - --test_cmux_cells; - } - result = c; - } else { - result = channel_flush_from_first_active_circuit__real(chan, max); - } - - done: - return result; -} - -/** - * If we have a target cmux set and this matches it, lie about how - * many cells we have according to the number indicated; otherwise - * pass to the real circuitmux_num_cells(). - */ - -static unsigned int -chan_test_circuitmux_num_cells_mock(circuitmux_t *cmux) -{ - unsigned int result = 0; - - tt_ptr_op(cmux, OP_NE, NULL); - if (cmux != NULL) { - if (cmux == test_target_cmux) { - result = test_cmux_cells; - } else { - result = circuitmux_num_cells__real(cmux); - } - } - - done: - - return result; -} - /* * Handle an incoming fixed-size cell for unit tests */ @@ -434,21 +370,12 @@ new_fake_channel(void) void free_fake_channel(channel_t *chan) { - cell_queue_entry_t *cell, *cell_tmp; - if (! chan) return; if (chan->cmux) circuitmux_free(chan->cmux); - TOR_SIMPLEQ_FOREACH_SAFE(cell, &chan->incoming_queue, next, cell_tmp) { - cell_queue_entry_free(cell, 0); - } - TOR_SIMPLEQ_FOREACH_SAFE(cell, &chan->outgoing_queue, next, cell_tmp) { - cell_queue_entry_free(cell, 0); - } - tor_free(chan); } @@ -608,7 +535,6 @@ test_channel_dumpstats(void *arg) cell = tor_malloc_zero(sizeof(cell_t)); make_fake_cell(cell); old_count = test_chan_fixed_cells_recved; - channel_queue_cell(ch, cell); tor_free(cell); tt_int_op(test_chan_fixed_cells_recved, OP_EQ, old_count + 1); tt_assert(ch->n_bytes_recved > 0); @@ -641,137 +567,6 @@ test_channel_dumpstats(void *arg) } static void -test_channel_flush(void *arg) -{ - channel_t *ch = NULL; - cell_t *cell = NULL; - packed_cell_t *p_cell = NULL; - var_cell_t *v_cell = NULL; - int init_count; - - (void)arg; - - ch = new_fake_channel(); - tt_assert(ch); - - /* Cache the original count */ - init_count = test_cells_written; - - /* Stop accepting so we can queue some */ - test_chan_accept_cells = 0; - - /* Queue a regular cell */ - cell = tor_malloc_zero(sizeof(cell_t)); - make_fake_cell(cell); - channel_write_cell(ch, cell); - /* It should be queued, so assert that we didn't write it */ - tt_int_op(test_cells_written, OP_EQ, init_count); - - /* Queue a var cell */ - v_cell = tor_malloc_zero(sizeof(var_cell_t) + CELL_PAYLOAD_SIZE); - make_fake_var_cell(v_cell); - channel_write_var_cell(ch, v_cell); - /* It should be queued, so assert that we didn't write it */ - tt_int_op(test_cells_written, OP_EQ, init_count); - - /* Try a packed cell now */ - p_cell = packed_cell_new(); - tt_assert(p_cell); - channel_write_packed_cell(ch, p_cell); - /* It should be queued, so assert that we didn't write it */ - tt_int_op(test_cells_written, OP_EQ, init_count); - - /* Now allow writes through again */ - test_chan_accept_cells = 1; - - /* ...and flush */ - channel_flush_cells(ch); - - /* All three should have gone through */ - tt_int_op(test_cells_written, OP_EQ, init_count + 3); - - done: - tor_free(ch); - - return; -} - -/** - * Channel flush tests that require cmux mocking - */ - -static void -test_channel_flushmux(void *arg) -{ - channel_t *ch = NULL; - int old_count, q_len_before, q_len_after; - ssize_t result; - - (void)arg; - - /* Install mocks we need for this test */ - MOCK(channel_flush_from_first_active_circuit, - chan_test_channel_flush_from_first_active_circuit_mock); - MOCK(circuitmux_num_cells, - chan_test_circuitmux_num_cells_mock); - - ch = new_fake_channel(); - tt_assert(ch); - ch->cmux = circuitmux_alloc(); - - old_count = test_cells_written; - - test_target_cmux = ch->cmux; - test_cmux_cells = 1; - - /* Enable cell acceptance */ - test_chan_accept_cells = 1; - - result = channel_flush_some_cells(ch, 1); - - tt_int_op(result, OP_EQ, 1); - tt_int_op(test_cells_written, OP_EQ, old_count + 1); - tt_int_op(test_cmux_cells, OP_EQ, 0); - - /* Now try it without accepting to force them into the queue */ - test_chan_accept_cells = 0; - test_cmux_cells = 1; - q_len_before = chan_cell_queue_len(&(ch->outgoing_queue)); - - result = channel_flush_some_cells(ch, 1); - - /* We should not have actually flushed any */ - tt_int_op(result, OP_EQ, 0); - tt_int_op(test_cells_written, OP_EQ, old_count + 1); - /* But we should have gotten to the fake cellgen loop */ - tt_int_op(test_cmux_cells, OP_EQ, 0); - /* ...and we should have a queued cell */ - q_len_after = chan_cell_queue_len(&(ch->outgoing_queue)); - tt_int_op(q_len_after, OP_EQ, q_len_before + 1); - - /* Now accept cells again and drain the queue */ - test_chan_accept_cells = 1; - channel_flush_cells(ch); - tt_int_op(test_cells_written, OP_EQ, old_count + 2); - tt_int_op(chan_cell_queue_len(&(ch->outgoing_queue)), OP_EQ, 0); - - test_target_cmux = NULL; - test_cmux_cells = 0; - - done: - if (ch) - circuitmux_free(ch->cmux); - tor_free(ch); - - UNMOCK(channel_flush_from_first_active_circuit); - UNMOCK(circuitmux_num_cells); - - test_chan_accept_cells = 0; - - return; -} - -static void test_channel_incoming(void *arg) { channel_t *ch = NULL; @@ -820,7 +615,6 @@ test_channel_incoming(void *arg) cell = tor_malloc_zero(sizeof(cell_t)); make_fake_cell(cell); old_count = test_chan_fixed_cells_recved; - channel_queue_cell(ch, cell); tor_free(cell); tt_int_op(test_chan_fixed_cells_recved, OP_EQ, old_count + 1); @@ -828,7 +622,6 @@ test_channel_incoming(void *arg) var_cell = tor_malloc_zero(sizeof(var_cell_t) + CELL_PAYLOAD_SIZE); make_fake_var_cell(var_cell); old_count = test_chan_var_cells_recved; - channel_queue_var_cell(ch, var_cell); tor_free(cell); tt_int_op(test_chan_var_cells_recved, OP_EQ, old_count + 1); @@ -1192,9 +985,6 @@ test_channel_multi(void *arg) /* Allow cells through again */ test_chan_accept_cells = 1; - /* Flush chan 2 */ - channel_flush_cells(ch2); - /* Update and check queue sizes */ channel_update_xmit_queue_size(ch1); channel_update_xmit_queue_size(ch2); @@ -1203,9 +993,6 @@ test_channel_multi(void *arg) global_queue_estimate = channel_get_global_queue_estimate(); tt_u64_op(global_queue_estimate, OP_EQ, 512); - /* Flush chan 1 */ - channel_flush_cells(ch1); - /* Update and check queue sizes */ channel_update_xmit_queue_size(ch1); channel_update_xmit_queue_size(ch2); @@ -1266,277 +1053,6 @@ test_channel_multi(void *arg) return; } -/** - * Check some hopefully-impossible edge cases in the channel queue we - * can only trigger by doing evil things to the queue directly. - */ - -static void -test_channel_queue_impossible(void *arg) -{ - channel_t *ch = NULL; - cell_t *cell = NULL; - packed_cell_t *packed_cell = NULL; - var_cell_t *var_cell = NULL; - int old_count; - cell_queue_entry_t *q = NULL; - uint64_t global_queue_estimate; - uintptr_t cellintptr; - - /* Cache the global queue size (see below) */ - global_queue_estimate = channel_get_global_queue_estimate(); - - (void)arg; - - ch = new_fake_channel(); - tt_assert(ch); - - /* We test queueing here; tell it not to accept cells */ - test_chan_accept_cells = 0; - /* ...and keep it from trying to flush the queue */ - ch->state = CHANNEL_STATE_MAINT; - - /* Cache the cell written count */ - old_count = test_cells_written; - - /* Assert that the queue is initially empty */ - tt_int_op(chan_cell_queue_len(&(ch->outgoing_queue)), OP_EQ, 0); - - /* Get a fresh cell and write it to the channel*/ - cell = tor_malloc_zero(sizeof(cell_t)); - make_fake_cell(cell); - cellintptr = (uintptr_t)(void*)cell; - channel_write_cell(ch, cell); - - /* Now it should be queued */ - tt_int_op(chan_cell_queue_len(&(ch->outgoing_queue)), OP_EQ, 1); - q = TOR_SIMPLEQ_FIRST(&(ch->outgoing_queue)); - tt_assert(q); - if (q) { - tt_int_op(q->type, OP_EQ, CELL_QUEUE_FIXED); - tt_assert((uintptr_t)q->u.fixed.cell == cellintptr); - } - /* Do perverse things to it */ - tor_free(q->u.fixed.cell); - q->u.fixed.cell = NULL; - - /* - * Now change back to open with channel_change_state() and assert that it - * gets thrown away properly. - */ - test_chan_accept_cells = 1; - channel_change_state_open(ch); - tt_assert(test_cells_written == old_count); - tt_int_op(chan_cell_queue_len(&(ch->outgoing_queue)), OP_EQ, 0); - - /* Same thing but for a var_cell */ - - test_chan_accept_cells = 0; - ch->state = CHANNEL_STATE_MAINT; - var_cell = tor_malloc_zero(sizeof(var_cell_t) + CELL_PAYLOAD_SIZE); - make_fake_var_cell(var_cell); - cellintptr = (uintptr_t)(void*)var_cell; - channel_write_var_cell(ch, var_cell); - - /* Check that it's queued */ - tt_int_op(chan_cell_queue_len(&(ch->outgoing_queue)), OP_EQ, 1); - q = TOR_SIMPLEQ_FIRST(&(ch->outgoing_queue)); - tt_assert(q); - if (q) { - tt_int_op(q->type, OP_EQ, CELL_QUEUE_VAR); - tt_assert((uintptr_t)q->u.var.var_cell == cellintptr); - } - - /* Remove the cell from the queue entry */ - tor_free(q->u.var.var_cell); - q->u.var.var_cell = NULL; - - /* Let it drain and check that the bad entry is discarded */ - test_chan_accept_cells = 1; - channel_change_state_open(ch); - tt_assert(test_cells_written == old_count); - tt_int_op(chan_cell_queue_len(&(ch->outgoing_queue)), OP_EQ, 0); - - /* Same thing with a packed_cell */ - - test_chan_accept_cells = 0; - ch->state = CHANNEL_STATE_MAINT; - packed_cell = packed_cell_new(); - tt_assert(packed_cell); - cellintptr = (uintptr_t)(void*)packed_cell; - channel_write_packed_cell(ch, packed_cell); - - /* Check that it's queued */ - tt_int_op(chan_cell_queue_len(&(ch->outgoing_queue)), OP_EQ, 1); - q = TOR_SIMPLEQ_FIRST(&(ch->outgoing_queue)); - tt_assert(q); - if (q) { - tt_int_op(q->type, OP_EQ, CELL_QUEUE_PACKED); - tt_assert((uintptr_t)q->u.packed.packed_cell == cellintptr); - } - - /* Remove the cell from the queue entry */ - packed_cell_free(q->u.packed.packed_cell); - q->u.packed.packed_cell = NULL; - - /* Let it drain and check that the bad entry is discarded */ - test_chan_accept_cells = 1; - channel_change_state_open(ch); - tt_assert(test_cells_written == old_count); - tt_int_op(chan_cell_queue_len(&(ch->outgoing_queue)), OP_EQ, 0); - - /* Unknown cell type case */ - test_chan_accept_cells = 0; - ch->state = CHANNEL_STATE_MAINT; - cell = tor_malloc_zero(sizeof(cell_t)); - make_fake_cell(cell); - cellintptr = (uintptr_t)(void*)cell; - channel_write_cell(ch, cell); - - /* Check that it's queued */ - tt_int_op(chan_cell_queue_len(&(ch->outgoing_queue)), OP_EQ, 1); - q = TOR_SIMPLEQ_FIRST(&(ch->outgoing_queue)); - tt_assert(q); - if (q) { - tt_int_op(q->type, OP_EQ, CELL_QUEUE_FIXED); - tt_assert((uintptr_t)q->u.fixed.cell == cellintptr); - } - /* Clobber it, including the queue entry type */ - tor_free(q->u.fixed.cell); - q->u.fixed.cell = NULL; - q->type = CELL_QUEUE_PACKED + 1; - - /* Let it drain and check that the bad entry is discarded */ - test_chan_accept_cells = 1; - tor_capture_bugs_(1); - channel_change_state_open(ch); - tt_assert(test_cells_written == old_count); - tt_int_op(chan_cell_queue_len(&(ch->outgoing_queue)), OP_EQ, 0); - - tt_int_op(smartlist_len(tor_get_captured_bug_log_()), OP_EQ, 1); - tor_end_capture_bugs_(); - - done: - free_fake_channel(ch); - - /* - * Doing that meant that we couldn't correctly adjust the queue size - * for the var cell, so manually reset the global queue size estimate - * so the next test doesn't break if we run with --no-fork. - */ - estimated_total_queue_size = global_queue_estimate; - - return; -} - -static void -test_channel_queue_incoming(void *arg) -{ - channel_t *ch = NULL; - cell_t *cell = NULL; - var_cell_t *var_cell = NULL; - int old_fixed_count, old_var_count; - - (void)arg; - - /* Mock these for duration of the test */ - MOCK(scheduler_channel_doesnt_want_writes, - scheduler_channel_doesnt_want_writes_mock); - MOCK(scheduler_release_channel, - scheduler_release_channel_mock); - - /* Accept cells to lower layer */ - test_chan_accept_cells = 1; - /* Use default overhead factor */ - test_overhead_estimate = 1.0; - - ch = new_fake_channel(); - tt_assert(ch); - /* Start it off in OPENING */ - ch->state = CHANNEL_STATE_OPENING; - /* We'll need a cmux */ - ch->cmux = circuitmux_alloc(); - - /* Test cell handler getters */ - tt_ptr_op(channel_get_cell_handler(ch), OP_EQ, NULL); - tt_ptr_op(channel_get_var_cell_handler(ch), OP_EQ, NULL); - - /* Try to register it */ - channel_register(ch); - tt_assert(ch->registered); - - /* Open it */ - channel_change_state_open(ch); - tt_int_op(ch->state, OP_EQ, CHANNEL_STATE_OPEN); - - /* Assert that the incoming queue is empty */ - tt_assert(TOR_SIMPLEQ_EMPTY(&(ch->incoming_queue))); - - /* Queue an incoming fixed-length cell */ - cell = tor_malloc_zero(sizeof(cell_t)); - make_fake_cell(cell); - channel_queue_cell(ch, cell); - - /* Assert that the incoming queue has one entry */ - tt_int_op(chan_cell_queue_len(&(ch->incoming_queue)), OP_EQ, 1); - - /* Queue an incoming var cell */ - var_cell = tor_malloc_zero(sizeof(var_cell_t) + CELL_PAYLOAD_SIZE); - make_fake_var_cell(var_cell); - channel_queue_var_cell(ch, var_cell); - - /* Assert that the incoming queue has two entries */ - tt_int_op(chan_cell_queue_len(&(ch->incoming_queue)), OP_EQ, 2); - - /* - * Install cell handlers; this will drain the queue, so save the old - * cell counters first - */ - old_fixed_count = test_chan_fixed_cells_recved; - old_var_count = test_chan_var_cells_recved; - channel_set_cell_handlers(ch, - chan_test_cell_handler, - chan_test_var_cell_handler); - tt_ptr_op(channel_get_cell_handler(ch), OP_EQ, chan_test_cell_handler); - tt_ptr_op(channel_get_var_cell_handler(ch), OP_EQ, - chan_test_var_cell_handler); - - /* Assert cells were received */ - tt_int_op(test_chan_fixed_cells_recved, OP_EQ, old_fixed_count + 1); - tt_int_op(test_chan_var_cells_recved, OP_EQ, old_var_count + 1); - - /* - * Assert that the pointers are different from the cells we allocated; - * when queueing cells with no incoming cell handlers installed, the - * channel layer should copy them to a new buffer, and free them after - * delivery. These pointers will have already been freed by the time - * we get here, so don't dereference them. - */ - tt_ptr_op(test_chan_last_seen_fixed_cell_ptr, OP_NE, cell); - tt_ptr_op(test_chan_last_seen_var_cell_ptr, OP_NE, var_cell); - - /* Assert queue is now empty */ - tt_assert(TOR_SIMPLEQ_EMPTY(&(ch->incoming_queue))); - - /* Close it; this contains an assertion that the incoming queue is empty */ - channel_mark_for_close(ch); - tt_int_op(ch->state, OP_EQ, CHANNEL_STATE_CLOSING); - chan_test_finish_close(ch); - tt_int_op(ch->state, OP_EQ, CHANNEL_STATE_CLOSED); - channel_run_cleanup(); - ch = NULL; - - done: - free_fake_channel(ch); - tor_free(cell); - tor_free(var_cell); - - UNMOCK(scheduler_channel_doesnt_want_writes); - UNMOCK(scheduler_release_channel); - - return; -} - static void test_channel_queue_size(void *arg) { @@ -1633,7 +1149,6 @@ test_channel_queue_size(void *arg) test_chan_accept_cells = 1; /* ...and re-process the queue */ old_count = test_cells_written; - channel_flush_cells(ch); tt_int_op(test_cells_written, OP_EQ, old_count + 1); /* Should have 32 writeable now */ @@ -1881,14 +1396,10 @@ test_channel_id_map(void *arg) struct testcase_t channel_tests[] = { { "dumpstats", test_channel_dumpstats, TT_FORK, NULL, NULL }, - { "flush", test_channel_flush, TT_FORK, NULL, NULL }, - { "flushmux", test_channel_flushmux, TT_FORK, NULL, NULL }, { "incoming", test_channel_incoming, TT_FORK, NULL, NULL }, { "lifecycle", test_channel_lifecycle, TT_FORK, NULL, NULL }, { "lifecycle_2", test_channel_lifecycle_2, TT_FORK, NULL, NULL }, { "multi", test_channel_multi, TT_FORK, NULL, NULL }, - { "queue_impossible", test_channel_queue_impossible, TT_FORK, NULL, NULL }, - { "queue_incoming", test_channel_queue_incoming, TT_FORK, NULL, NULL }, { "queue_size", test_channel_queue_size, TT_FORK, NULL, NULL }, { "write", test_channel_write, TT_FORK, NULL, NULL }, { "id_map", test_channel_id_map, TT_FORK, NULL, NULL }, diff --git a/src/test/test_scheduler.c b/src/test/test_scheduler.c index d679d7cfe..d861b05b8 100644 --- a/src/test/test_scheduler.c +++ b/src/test/test_scheduler.c @@ -299,10 +299,6 @@ channel_more_to_flush_mock(channel_t *chan) flush_mock_channel_t *found_mock_ch = NULL; - /* Check if we have any queued */ - if (! TOR_SIMPLEQ_EMPTY(&chan->incoming_queue)) - return 1; - SMARTLIST_FOREACH_BEGIN(chans_for_flush_mock, flush_mock_channel_t *, flush_mock_ch) {

1 0

[tor/master] channel: Requeue cell to circuit if channnel failed
by nickm＠torproject.org 08 Dec '17

08 Dec '17

commit 56833bf4491ef774163bc263bb18fa66fb378292 Author: David Goulet <dgoulet(a)torproject.org> Date: Tue Nov 21 12:56:10 2017 -0500 channel: Requeue cell to circuit if channnel failed If the channel layer failed to write a cell from the circuit queue, requeue it so it can be retried on the same channel later. Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/or/relay.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/src/or/relay.c b/src/or/relay.c index d6c103c14..f36fc78cd 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -2521,6 +2521,17 @@ cell_queue_pop(cell_queue_t *queue) return cell; } +/** Insert cell as the head of the queue. */ +static void +cell_insert_head(cell_queue_t *queue, packed_cell_t *cell) +{ + tor_assert(queue); + tor_assert(cell); + + TOR_SIMPLEQ_INSERT_HEAD(&queue->head, cell, next); + ++queue->n; +} + /** Return the total number of bytes used for each packed_cell in a queue. * Approximate. */ size_t @@ -2746,7 +2757,10 @@ channel_flush_from_first_active_circuit, (channel_t *chan, int max)) /* this code is duplicated from some of the logic below. Ugly! XXXX */ tor_assert(destroy_queue->n > 0); cell = cell_queue_pop(destroy_queue); - channel_write_packed_cell(chan, cell); + if (channel_write_packed_cell(chan, cell) < 0) { + cell_insert_head(destroy_queue, cell); + continue; + } /* Update the cmux destroy counter */ circuitmux_notify_xmit_destroy(cmux); cell = NULL; @@ -2823,7 +2837,12 @@ channel_flush_from_first_active_circuit, (channel_t *chan, int max)) DIRREQ_CIRC_QUEUE_FLUSHED); /* Now send the cell */ - channel_write_packed_cell(chan, cell); + if (channel_write_packed_cell(chan, cell) < 0) { + /* Unable to send the cell, put it back at the start of the circuit + * queue so we can retry. */ + cell_insert_head(queue, cell); + continue; + } cell = NULL; /*

1 0

[tor/master] channel: Remove dead code
by nickm＠torproject.org 08 Dec '17

08 Dec '17

commit 163477b11eae5cebbf5b4298871838f618b50bd5 Author: David Goulet <dgoulet(a)torproject.org> Date: Tue Nov 21 15:08:19 2017 -0500 channel: Remove dead code This removed code that was either never reached or irrelevant after the incoming/outgoing queue removal such as the "timestamp_drained". Lots of things are also removed from channel.h that do not exists anymore or not used. Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/or/channel.c | 329 ------------------------------------------------------- src/or/channel.h | 71 ------------ 2 files changed, 400 deletions(-) diff --git a/src/or/channel.c b/src/or/channel.c index e8e7805e6..3a08d4cb0 100644 --- a/src/or/channel.c +++ b/src/or/channel.c @@ -599,33 +599,6 @@ channel_remove_from_digest_map(channel_t *chan) /* Assert that there is a digest */ tor_assert(!tor_digest_is_zero(chan->identity_digest)); -#if 0 - /* Make sure we have a map */ - if (!channel_identity_map) { - /* - * No identity map, so we can't find it by definition. This - * case is similar to digestmap_get() failing below. - */ - log_warn(LD_BUG, - "Trying to remove channel %p (global ID " U64_FORMAT ") " - "with digest %s from identity map, but didn't have any identity " - "map", - chan, U64_PRINTF_ARG(chan->global_identifier), - hex_str(chan->identity_digest, DIGEST_LEN)); - /* Clear out its next/prev pointers */ - if (chan->next_with_same_id) { - chan->next_with_same_id->prev_with_same_id = chan->prev_with_same_id; - } - if (chan->prev_with_same_id) { - chan->prev_with_same_id->next_with_same_id = chan->next_with_same_id; - } - chan->next_with_same_id = NULL; - chan->prev_with_same_id = NULL; - - return; - } -#endif /* 0 */ - /* Pull it out of its list, wherever that list is */ TOR_LIST_REMOVE(chan, next_with_same_id); @@ -1055,24 +1028,6 @@ channel_listener_force_free(channel_listener_t *chan_l) } /** - * Return the current registered listener for a channel listener - * - * This function returns a function pointer to the current registered - * handler for new incoming channels on a channel listener. - */ - -channel_listener_fn_ptr -channel_listener_get_listener_fn(channel_listener_t *chan_l) -{ - tor_assert(chan_l); - - if (chan_l->state == CHANNEL_LISTENER_STATE_LISTENING) - return chan_l->listener; - - return NULL; -} - -/** * Set the listener for a channel listener * * This function sets the handler for new incoming channels on a channel @@ -1284,36 +1239,6 @@ channel_close_from_lower_layer(channel_t *chan) } /** - * Close a channel listener from the lower layer - * - * Notify the channel code that the channel listener is being closed due to a - * non-error condition in the lower layer. This does not call the close() - * method, since the lower layer already knows. - */ - -void -channel_listener_close_from_lower_layer(channel_listener_t *chan_l) -{ - tor_assert(chan_l != NULL); - - /* If it's already in CLOSING, CLOSED or ERROR, this is a no-op */ - if (chan_l->state == CHANNEL_LISTENER_STATE_CLOSING || - chan_l->state == CHANNEL_LISTENER_STATE_CLOSED || - chan_l->state == CHANNEL_LISTENER_STATE_ERROR) return; - - log_debug(LD_CHANNEL, - "Closing channel listener %p (global ID " U64_FORMAT ") " - "due to lower-layer event", - chan_l, U64_PRINTF_ARG(chan_l->global_identifier)); - - /* Note closing by event from below */ - chan_l->reason_for_closing = CHANNEL_LISTENER_CLOSE_FROM_BELOW; - - /* Change state to CLOSING */ - channel_listener_change_state(chan_l, CHANNEL_LISTENER_STATE_CLOSING); -} - -/** * Notify that the channel is being closed due to an error condition * * This function is called by the lower layer implementing the transport @@ -1342,37 +1267,6 @@ channel_close_for_error(channel_t *chan) } /** - * Notify that the channel listener is being closed due to an error condition - * - * This function is called by the lower layer implementing the transport - * when a channel listener must be closed due to an error condition. This - * does not call the channel listener's close method, since the lower layer - * already knows. - */ - -void -channel_listener_close_for_error(channel_listener_t *chan_l) -{ - tor_assert(chan_l != NULL); - - /* If it's already in CLOSING, CLOSED or ERROR, this is a no-op */ - if (chan_l->state == CHANNEL_LISTENER_STATE_CLOSING || - chan_l->state == CHANNEL_LISTENER_STATE_CLOSED || - chan_l->state == CHANNEL_LISTENER_STATE_ERROR) return; - - log_debug(LD_CHANNEL, - "Closing channel listener %p (global ID " U64_FORMAT ") " - "due to lower-layer error", - chan_l, U64_PRINTF_ARG(chan_l->global_identifier)); - - /* Note closing by event from below */ - chan_l->reason_for_closing = CHANNEL_LISTENER_CLOSE_FOR_ERROR; - - /* Change state to CLOSING */ - channel_listener_change_state(chan_l, CHANNEL_LISTENER_STATE_CLOSING); -} - -/** * Notify that the lower layer is finished closing the channel * * This function should be called by the lower layer when a channel @@ -1406,33 +1300,6 @@ channel_closed(channel_t *chan) } /** - * Notify that the lower layer is finished closing the channel listener - * - * This function should be called by the lower layer when a channel listener - * is finished closing and it should be regarded as inactive and - * freed by the channel code. - */ - -void -channel_listener_closed(channel_listener_t *chan_l) -{ - tor_assert(chan_l); - tor_assert(chan_l->state == CHANNEL_LISTENER_STATE_CLOSING || - chan_l->state == CHANNEL_LISTENER_STATE_CLOSED || - chan_l->state == CHANNEL_LISTENER_STATE_ERROR); - - /* No-op if already inactive */ - if (chan_l->state == CHANNEL_LISTENER_STATE_CLOSED || - chan_l->state == CHANNEL_LISTENER_STATE_ERROR) return; - - if (chan_l->reason_for_closing != CHANNEL_LISTENER_CLOSE_FOR_ERROR) { - channel_listener_change_state(chan_l, CHANNEL_LISTENER_STATE_CLOSED); - } else { - channel_listener_change_state(chan_l, CHANNEL_LISTENER_STATE_ERROR); - } -} - -/** * Clear the identity_digest of a channel * * This function clears the identity digest of the remote endpoint for a @@ -1553,67 +1420,6 @@ channel_clear_remote_end(channel_t *chan) } /** - * Set the remote end metadata (identity_digest/nickname) of a channel - * - * This function sets new remote end info on a channel; this is intended - * for use by the lower layer. - */ - -void -channel_set_remote_end(channel_t *chan, - const char *identity_digest, - const char *nickname) -{ - int was_in_digest_map, should_be_in_digest_map, state_not_in_map; - - tor_assert(chan); - - log_debug(LD_CHANNEL, - "Setting remote endpoint identity on channel %p with " - "global ID " U64_FORMAT " to nickname %s, digest %s", - chan, U64_PRINTF_ARG(chan->global_identifier), - nickname ? nickname : "(null)", - identity_digest ? - hex_str(identity_digest, DIGEST_LEN) : "(null)"); - - state_not_in_map = CHANNEL_CONDEMNED(chan); - - was_in_digest_map = - !state_not_in_map && - chan->registered && - !tor_digest_is_zero(chan->identity_digest); - should_be_in_digest_map = - !state_not_in_map && - chan->registered && - (identity_digest && - !tor_digest_is_zero(identity_digest)); - - if (was_in_digest_map) - /* We should always remove it; we'll add it back if we're writing - * in a new digest. - */ - channel_remove_from_digest_map(chan); - - if (identity_digest) { - memcpy(chan->identity_digest, - identity_digest, - sizeof(chan->identity_digest)); - - } else { - memset(chan->identity_digest, 0, - sizeof(chan->identity_digest)); - } - - tor_free(chan->nickname); - if (nickname) - chan->nickname = tor_strdup(nickname); - - /* Put it in the digest map if we should */ - if (should_be_in_digest_map) - channel_add_to_digest_map(chan); -} - -/** * Write to a channel the given packed cell. * * Return 0 on success and the cell is freed so the caller MUST discard any @@ -1652,8 +1458,6 @@ write_packed_cell(channel_t *chan, packed_cell_t *cell) } /* Timestamp for transmission */ channel_timestamp_xmit(chan); - /* If we're here the queue is empty, so it's drained too */ - channel_timestamp_drained(chan); /* Update the counter */ ++(chan->n_cells_xmitted); chan->n_bytes_xmitted += cell_bytes; @@ -2874,12 +2678,6 @@ channel_dump_statistics, (channel_t *chan, int severity)) U64_PRINTF_ARG(chan->timestamp_client), U64_PRINTF_ARG(now - chan->timestamp_client)); tor_log(severity, LD_GENERAL, - " * Channel " U64_FORMAT " was last drained at " - U64_FORMAT " (" U64_FORMAT " seconds ago)", - U64_PRINTF_ARG(chan->global_identifier), - U64_PRINTF_ARG(chan->timestamp_drained), - U64_PRINTF_ARG(now - chan->timestamp_drained)); - tor_log(severity, LD_GENERAL, " * Channel " U64_FORMAT " last received a cell " "at " U64_FORMAT " (" U64_FORMAT " seconds ago)", U64_PRINTF_ARG(chan->global_identifier), @@ -3496,25 +3294,6 @@ channel_timestamp_client(channel_t *chan) } /** - * Update the last drained timestamp - * - * This is called whenever we transmit a cell which leaves the outgoing cell - * queue completely empty. It also updates the xmit time and the active time. - */ - -void -channel_timestamp_drained(channel_t *chan) -{ - time_t now = time(NULL); - - tor_assert(chan); - - chan->timestamp_active = now; - chan->timestamp_drained = now; - chan->timestamp_xmit = now; -} - -/** * Update the recv timestamp * * This is called whenever we get an incoming cell from the lower layer. @@ -3573,54 +3352,6 @@ channel_when_created(channel_t *chan) } /** - * Query created timestamp for a channel listener - */ - -time_t -channel_listener_when_created(channel_listener_t *chan_l) -{ - tor_assert(chan_l); - - return chan_l->timestamp_created; -} - -/** - * Query last active timestamp for a channel - */ - -time_t -channel_when_last_active(channel_t *chan) -{ - tor_assert(chan); - - return chan->timestamp_active; -} - -/** - * Query last active timestamp for a channel listener - */ - -time_t -channel_listener_when_last_active(channel_listener_t *chan_l) -{ - tor_assert(chan_l); - - return chan_l->timestamp_active; -} - -/** - * Query last accepted timestamp for a channel listener - */ - -time_t -channel_listener_when_last_accepted(channel_listener_t *chan_l) -{ - tor_assert(chan_l); - - return chan_l->timestamp_accepted; -} - -/** * Query client timestamp */ @@ -3633,30 +3364,6 @@ channel_when_last_client(channel_t *chan) } /** - * Query drained timestamp - */ - -time_t -channel_when_last_drained(channel_t *chan) -{ - tor_assert(chan); - - return chan->timestamp_drained; -} - -/** - * Query recv timestamp - */ - -time_t -channel_when_last_recv(channel_t *chan) -{ - tor_assert(chan); - - return chan->timestamp_recv; -} - -/** * Query xmit timestamp */ @@ -3669,42 +3376,6 @@ channel_when_last_xmit(channel_t *chan) } /** - * Query accepted counter - */ - -uint64_t -channel_listener_count_accepted(channel_listener_t *chan_l) -{ - tor_assert(chan_l); - - return chan_l->n_accepted; -} - -/** - * Query received cell counter - */ - -uint64_t -channel_count_recved(channel_t *chan) -{ - tor_assert(chan); - - return chan->n_cells_recved; -} - -/** - * Query transmitted cell counter - */ - -uint64_t -channel_count_xmitted(channel_t *chan) -{ - tor_assert(chan); - - return chan->n_cells_xmitted; -} - -/** * Check if a channel matches an extend_info_t * * This function calls the lower layer and asks if this channel matches a diff --git a/src/or/channel.h b/src/or/channel.h index e4d34d4fc..1dc378b3d 100644 --- a/src/or/channel.h +++ b/src/or/channel.h @@ -19,10 +19,6 @@ typedef void (*channel_listener_fn_ptr)(channel_listener_t *, channel_t *); typedef void (*channel_cell_handler_fn_ptr)(channel_t *, cell_t *); typedef void (*channel_var_cell_handler_fn_ptr)(channel_t *, var_cell_t *); -struct cell_queue_entry_s; -TOR_SIMPLEQ_HEAD(chan_cell_queue, cell_queue_entry_s); -typedef struct chan_cell_queue chan_cell_queue_t; - /** * This enum is used by channelpadding to decide when to pad channels. * Don't add values to it without updating the checks in @@ -314,7 +310,6 @@ struct channel_s { /** Channel timestamps for cell channels */ time_t timestamp_client; /* Client used this, according to relay.c */ - time_t timestamp_drained; /* Output queue empty */ time_t timestamp_recv; /* Cell received from lower layer */ time_t timestamp_xmit; /* Cell sent to lower layer */ @@ -331,14 +326,6 @@ struct channel_s { /** Channel counters for cell channels */ uint64_t n_cells_recved, n_bytes_recved; uint64_t n_cells_xmitted, n_bytes_xmitted; - - /** Our current contribution to the scheduler's total xmit queue */ - uint64_t bytes_queued_for_xmit; - - /** Number of bytes in this channel's cell queue; does not include - * lower-layer queueing. - */ - uint64_t bytes_in_queue; }; struct channel_listener_s { @@ -413,9 +400,6 @@ void channel_listener_mark_for_close(channel_listener_t *chan_l); /* Channel callback registrations */ /* Listener callback */ -channel_listener_fn_ptr -channel_listener_get_listener_fn(channel_listener_t *chan); - void channel_listener_set_listener_fn(channel_listener_t *chan, channel_listener_fn_ptr listener); @@ -449,31 +433,7 @@ void channel_set_cmux_policy_everywhere(circuitmux_policy_t *pol); #ifdef TOR_CHANNEL_INTERNAL_ #ifdef CHANNEL_PRIVATE_ -/* Cell queue structure (here rather than channel.c for test suite use) */ - -typedef struct cell_queue_entry_s cell_queue_entry_t; -struct cell_queue_entry_s { - TOR_SIMPLEQ_ENTRY(cell_queue_entry_s) next; - enum { - CELL_QUEUE_FIXED, - CELL_QUEUE_VAR, - CELL_QUEUE_PACKED - } type; - union { - struct { - cell_t *cell; - } fixed; - struct { - var_cell_t *var_cell; - } var; - struct { - packed_cell_t *packed_cell; - } packed; - } u; -}; -void channel_write_cell_generic_(channel_t *chan, const char *cell_type, - void *cell, cell_queue_entry_t *q); #endif /* defined(CHANNEL_PRIVATE_) */ /* Channel operations for subclasses and internal use only */ @@ -498,10 +458,6 @@ void channel_close_from_lower_layer(channel_t *chan); void channel_close_for_error(channel_t *chan); void channel_closed(channel_t *chan); -void channel_listener_close_from_lower_layer(channel_listener_t *chan_l); -void channel_listener_close_for_error(channel_listener_t *chan_l); -void channel_listener_closed(channel_listener_t *chan_l); - /* Free a channel */ void channel_free(channel_t *chan); void channel_listener_free(channel_listener_t *chan_l); @@ -519,9 +475,6 @@ void channel_mark_remote(channel_t *chan); void channel_set_identity_digest(channel_t *chan, const char *identity_digest, const ed25519_public_key_t *ed_identity); -void channel_set_remote_end(channel_t *chan, - const char *identity_digest, - const char *nickname); void channel_listener_change_state(channel_listener_t *chan_l, channel_listener_state_t to_state); @@ -529,7 +482,6 @@ void channel_listener_change_state(channel_listener_t *chan_l, /* Timestamp updates */ void channel_timestamp_created(channel_t *chan); void channel_timestamp_active(channel_t *chan); -void channel_timestamp_drained(channel_t *chan); void channel_timestamp_recv(channel_t *chan); void channel_timestamp_xmit(channel_t *chan); @@ -544,11 +496,6 @@ void channel_listener_queue_incoming(channel_listener_t *listener, /* Incoming cell handling */ void channel_process_cell(channel_t *chan, cell_t *cell); -void channel_queue_cell(channel_t *chan, cell_t *cell); -void channel_queue_var_cell(channel_t *chan, var_cell_t *var_cell); - -/* Outgoing cell handling */ -void channel_flush_cells(channel_t *chan); /* Request from lower layer for more cells if available */ MOCK_DECL(ssize_t, channel_flush_some_cells, @@ -563,10 +510,6 @@ void channel_notify_flushed(channel_t *chan); /* Handle stuff we need to do on open like notifying circuits */ void channel_do_open_actions(channel_t *chan); -#ifdef TOR_UNIT_TESTS -extern uint64_t estimated_total_queue_size; -#endif - #endif /* defined(TOR_CHANNEL_INTERNAL_) */ /* Helper functions to perform operations on channels */ @@ -667,7 +610,6 @@ MOCK_DECL(void,channel_set_circid_type,(channel_t *chan, crypto_pk_t *identity_rcvd, int consider_identity)); void channel_timestamp_client(channel_t *chan); -void channel_update_xmit_queue_size(channel_t *chan); const char * channel_listener_describe_transport(channel_listener_t *chan_l); void channel_listener_dump_statistics(channel_listener_t *chan_l, @@ -679,27 +621,14 @@ void channel_check_for_duplicates(void); void channel_update_bad_for_new_circs(const char *digest, int force); /* Flow control queries */ -uint64_t channel_get_global_queue_estimate(void); int channel_num_cells_writeable(channel_t *chan); /* Timestamp queries */ time_t channel_when_created(channel_t *chan); -time_t channel_when_last_active(channel_t *chan); time_t channel_when_last_client(channel_t *chan); -time_t channel_when_last_drained(channel_t *chan); -time_t channel_when_last_recv(channel_t *chan); time_t channel_when_last_xmit(channel_t *chan); -time_t channel_listener_when_created(channel_listener_t *chan_l); -time_t channel_listener_when_last_active(channel_listener_t *chan_l); -time_t channel_listener_when_last_accepted(channel_listener_t *chan_l); - /* Counter queries */ -uint64_t channel_count_recved(channel_t *chan); -uint64_t channel_count_xmitted(channel_t *chan); - -uint64_t channel_listener_count_accepted(channel_listener_t *chan_l); - int packed_cell_is_destroy(channel_t *chan, const packed_cell_t *packed_cell, circid_t *circid_out);

1 0

[tor/master] channel: Remove unused write cell functions
by nickm＠torproject.org 08 Dec '17

08 Dec '17

commit 6d1ea7766b4fa6265744523fb3797cf8cf40d691 Author: David Goulet <dgoulet(a)torproject.org> Date: Tue Nov 21 12:44:46 2017 -0500 channel: Remove unused write cell functions The channel_write_cell() and channel_write_var_cell() can't be possibly called nor are used by tor. We only write on the connection outbuf packed cell coming from the scheduler that takes them from the circuit queue. This makes channel_write_packed_cell() the only usable function. It is simplify and now returns a code value. The reason for this is that in the next commit(s), we'll re-queue the cell onto the circuit queue if the write fails. Finally, channel unit tests are being removed with this commit because they do not match the new semantic. They will be re-written in future commits. Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/or/channel.c | 208 +++++++++--------------------------------------- src/or/channel.h | 4 +- src/test/test_channel.c | 151 ++--------------------------------- 3 files changed, 44 insertions(+), 319 deletions(-) diff --git a/src/or/channel.c b/src/or/channel.c index 0586c05da..192ca57c6 100644 --- a/src/or/channel.c +++ b/src/or/channel.c @@ -148,9 +148,6 @@ HT_PROTOTYPE(channel_idmap, channel_idmap_entry_s, node, channel_idmap_hash, HT_GENERATE2(channel_idmap, channel_idmap_entry_s, node, channel_idmap_hash, channel_idmap_eq, 0.5, tor_reallocarray_, tor_free_) -static int is_destroy_cell(channel_t *chan, - const cell_queue_entry_t *q, circid_t *circid_out); - /* Functions to maintain the digest map */ static void channel_add_to_digest_map(channel_t *chan); static void channel_remove_from_digest_map(channel_t *chan); @@ -161,10 +158,6 @@ channel_free_list(smartlist_t *channels, int mark_for_close); static void channel_listener_free_list(smartlist_t *channels, int mark_for_close); static void channel_listener_force_free(channel_listener_t *chan_l); -static size_t channel_get_cell_queue_entry_size(channel_t *chan, - cell_queue_entry_t *q); -static void -channel_write_cell_queue_entry(channel_t *chan, cell_queue_entry_t *q); /*********************************** * Channel state utility functions * @@ -1629,185 +1622,81 @@ channel_set_remote_end(channel_t *chan, } /** - * Ask how big the cell contained in a cell_queue_entry_t is - */ - -static size_t -channel_get_cell_queue_entry_size(channel_t *chan, cell_queue_entry_t *q) -{ - size_t rv = 0; - - tor_assert(chan); - tor_assert(q); - - switch (q->type) { - case CELL_QUEUE_FIXED: - rv = get_cell_network_size(chan->wide_circ_ids); - break; - case CELL_QUEUE_VAR: - rv = get_var_cell_header_size(chan->wide_circ_ids) + - (q->u.var.var_cell ? q->u.var.var_cell->payload_len : 0); - break; - case CELL_QUEUE_PACKED: - rv = get_cell_network_size(chan->wide_circ_ids); - break; - default: - tor_assert_nonfatal_unreached_once(); - } - - return rv; -} - -/** * Write to a channel based on a cell_queue_entry_t * * Given a cell_queue_entry_t filled out by the caller, try to send the cell * and queue it if we can't. */ - -static void -channel_write_cell_queue_entry(channel_t *chan, cell_queue_entry_t *q) +static int +write_packed_cell(channel_t *chan, packed_cell_t *cell) { - int result = 0; + int ret = -1; size_t cell_bytes; tor_assert(chan); - tor_assert(q); + tor_assert(cell); /* Assert that the state makes sense for a cell write */ tor_assert(CHANNEL_CAN_HANDLE_CELLS(chan)); { circid_t circ_id; - if (is_destroy_cell(chan, q, &circ_id)) { + if (packed_cell_is_destroy(chan, cell, &circ_id)) { channel_note_destroy_not_pending(chan, circ_id); } } /* For statistical purposes, figure out how big this cell is */ - cell_bytes = channel_get_cell_queue_entry_size(chan, q); + cell_bytes = get_cell_network_size(chan->wide_circ_ids); /* Can we send it right out? If so, try */ - if (CHANNEL_IS_OPEN(chan)) { - /* Pick the right write function for this cell type and save the result */ - switch (q->type) { - case CELL_QUEUE_FIXED: - tor_assert(chan->write_cell); - tor_assert(q->u.fixed.cell); - result = chan->write_cell(chan, q->u.fixed.cell); - break; - case CELL_QUEUE_PACKED: - tor_assert(chan->write_packed_cell); - tor_assert(q->u.packed.packed_cell); - result = chan->write_packed_cell(chan, q->u.packed.packed_cell); - break; - case CELL_QUEUE_VAR: - tor_assert(chan->write_var_cell); - tor_assert(q->u.var.var_cell); - result = chan->write_var_cell(chan, q->u.var.var_cell); - break; - default: - tor_assert(1); - } + if (!CHANNEL_IS_OPEN(chan)) { + goto done; + } - /* Check if we got it out */ - if (result > 0) { - /* Timestamp for transmission */ - channel_timestamp_xmit(chan); - /* If we're here the queue is empty, so it's drained too */ - channel_timestamp_drained(chan); - /* Update the counter */ - ++(chan->n_cells_xmitted); - chan->n_bytes_xmitted += cell_bytes; - } + /* Write the cell on the connection's outbuf. */ + if (chan->write_packed_cell(chan, cell) < 0) { + goto done; } + /* Timestamp for transmission */ + channel_timestamp_xmit(chan); + /* If we're here the queue is empty, so it's drained too */ + channel_timestamp_drained(chan); + /* Update the counter */ + ++(chan->n_cells_xmitted); + chan->n_bytes_xmitted += cell_bytes; + /* Successfully sent the cell. */ + ret = 0; - /* XXX: If the cell wasn't sent, we need to propagate the error back so we - * can put it back in the circuit queue. */ + done: + return ret; } -/** Write a generic cell type to a channel +/** + * Write a packed cell to a channel * - * Write a generic cell to a channel. It is called by channel_write_cell(), - * channel_write_var_cell() and channel_write_packed_cell() in order to reduce - * code duplication. Notice that it takes cell as pointer of type void, - * this can be dangerous because no type check is performed. + * Write a packed cell to a channel using the write_cell() method. This is + * called by the transport-independent code to deliver a packed cell to a + * channel for transmission. */ - -void -channel_write_cell_generic_(channel_t *chan, const char *cell_type, - void *cell, cell_queue_entry_t *q) +int +channel_write_packed_cell(channel_t *chan, packed_cell_t *cell) { - tor_assert(chan); tor_assert(cell); if (CHANNEL_IS_CLOSING(chan)) { - log_debug(LD_CHANNEL, "Discarding %c %p on closing channel %p with " - "global ID "U64_FORMAT, *cell_type, cell, chan, + log_debug(LD_CHANNEL, "Discarding %p on closing channel %p with " + "global ID "U64_FORMAT, cell, chan, U64_PRINTF_ARG(chan->global_identifier)); tor_free(cell); - return; + return -1; } log_debug(LD_CHANNEL, - "Writing %c %p to channel %p with global ID " - U64_FORMAT, *cell_type, - cell, chan, U64_PRINTF_ARG(chan->global_identifier)); + "Writing %p to channel %p with global ID " + U64_FORMAT, cell, chan, U64_PRINTF_ARG(chan->global_identifier)); - channel_write_cell_queue_entry(chan, q); -} - -/** - * Write a cell to a channel - * - * Write a fixed-length cell to a channel using the write_cell() method. - * This is equivalent to the pre-channels connection_or_write_cell_to_buf(); - * it is called by the transport-independent code to deliver a cell to a - * channel for transmission. - */ - -void -channel_write_cell(channel_t *chan, cell_t *cell) -{ - cell_queue_entry_t q; - q.type = CELL_QUEUE_FIXED; - q.u.fixed.cell = cell; - channel_write_cell_generic_(chan, "cell_t", cell, &q); -} - -/** - * Write a packed cell to a channel - * - * Write a packed cell to a channel using the write_cell() method. This is - * called by the transport-independent code to deliver a packed cell to a - * channel for transmission. - */ - -void -channel_write_packed_cell(channel_t *chan, packed_cell_t *packed_cell) -{ - cell_queue_entry_t q; - q.type = CELL_QUEUE_PACKED; - q.u.packed.packed_cell = packed_cell; - channel_write_cell_generic_(chan, "packed_cell_t", packed_cell, &q); -} - -/** - * Write a variable-length cell to a channel - * - * Write a variable-length cell to a channel using the write_cell() method. - * This is equivalent to the pre-channels - * connection_or_write_var_cell_to_buf(); it's called by the transport- - * independent code to deliver a var_cell to a channel for transmission. - */ - -void -channel_write_var_cell(channel_t *chan, var_cell_t *var_cell) -{ - cell_queue_entry_t q; - q.type = CELL_QUEUE_VAR; - q.u.var.var_cell = var_cell; - channel_write_cell_generic_(chan, "var_cell_t", var_cell, &q); + return write_packed_cell(chan, cell); } /** @@ -2352,31 +2241,6 @@ packed_cell_is_destroy(channel_t *chan, return 0; } -/* DOCDOC */ -static int -is_destroy_cell(channel_t *chan, - const cell_queue_entry_t *q, circid_t *circid_out) -{ - *circid_out = 0; - switch (q->type) { - case CELL_QUEUE_FIXED: - if (q->u.fixed.cell->command == CELL_DESTROY) { - *circid_out = q->u.fixed.cell->circ_id; - return 1; - } - break; - case CELL_QUEUE_VAR: - if (q->u.var.var_cell->command == CELL_DESTROY) { - *circid_out = q->u.var.var_cell->circ_id; - return 1; - } - break; - case CELL_QUEUE_PACKED: - return packed_cell_is_destroy(chan, q->u.packed.packed_cell, circid_out); - } - return 0; -} - /** * Send destroy cell on a channel * diff --git a/src/or/channel.h b/src/or/channel.h index d72f232bc..e4d34d4fc 100644 --- a/src/or/channel.h +++ b/src/or/channel.h @@ -406,9 +406,7 @@ channel_listener_state_to_string(channel_listener_state_t state); /* Abstract channel operations */ void channel_mark_for_close(channel_t *chan); -void channel_write_cell(channel_t *chan, cell_t *cell); -void channel_write_packed_cell(channel_t *chan, packed_cell_t *cell); -void channel_write_var_cell(channel_t *chan, var_cell_t *cell); +int channel_write_packed_cell(channel_t *chan, packed_cell_t *cell); void channel_listener_mark_for_close(channel_listener_t *chan_l); diff --git a/src/test/test_channel.c b/src/test/test_channel.c index 3381a5c7d..bc173472e 100644 --- a/src/test/test_channel.c +++ b/src/test/test_channel.c @@ -26,7 +26,6 @@ static cell_t * test_chan_last_seen_fixed_cell_ptr = NULL; static int test_chan_var_cells_recved = 0; static var_cell_t * test_chan_last_seen_var_cell_ptr = NULL; static int test_cells_written = 0; -static int test_destroy_not_pending_calls = 0; static int test_doesnt_want_writes_count = 0; static int test_dumpstats_calls = 0; static int test_has_waiting_cells_count = 0; @@ -37,8 +36,6 @@ static int dump_statistics_mock_matches = 0; static void chan_test_channel_dump_statistics_mock( channel_t *chan, int severity); -static void channel_note_destroy_not_pending_mock(channel_t *ch, - circid_t circid); static void chan_test_cell_handler(channel_t *ch, cell_t *cell); static const char * chan_test_describe_transport(channel_t *ch); @@ -61,17 +58,6 @@ static void scheduler_channel_doesnt_want_writes_mock(channel_t *ch); static void test_channel_dumpstats(void *arg); static void test_channel_incoming(void *arg); static void test_channel_lifecycle(void *arg); -static void test_channel_write(void *arg); - -static void -channel_note_destroy_not_pending_mock(channel_t *ch, - circid_t circid) -{ - (void)ch; - (void)circid; - - ++test_destroy_not_pending_calls; -} static const char * chan_test_describe_transport(channel_t *ch) @@ -447,7 +433,7 @@ static void test_channel_dumpstats(void *arg) { channel_t *ch = NULL; - cell_t *cell = NULL; + packed_cell_t *p_cell = NULL; int old_count; (void)arg; @@ -513,12 +499,10 @@ test_channel_dumpstats(void *arg) tt_assert(time(NULL) > ch->timestamp_created); /* Put cells through it both ways to make the counters non-zero */ - cell = tor_malloc_zero(sizeof(*cell)); - make_fake_cell(cell); + p_cell = packed_cell_new(); test_chan_accept_cells = 1; old_count = test_cells_written; - channel_write_cell(ch, cell); - cell = NULL; + channel_write_packed_cell(ch, p_cell); tt_int_op(test_cells_written, OP_EQ, old_count + 1); tt_assert(ch->n_bytes_xmitted > 0); tt_assert(ch->n_cells_xmitted > 0); @@ -530,10 +514,8 @@ test_channel_dumpstats(void *arg) tt_ptr_op(channel_get_cell_handler(ch), OP_EQ, chan_test_cell_handler); tt_ptr_op(channel_get_var_cell_handler(ch), OP_EQ, chan_test_var_cell_handler); - cell = tor_malloc_zero(sizeof(cell_t)); - make_fake_cell(cell); + p_cell = packed_cell_new(); old_count = test_chan_fixed_cells_recved; - tor_free(cell); tt_int_op(test_chan_fixed_cells_recved, OP_EQ, old_count + 1); tt_assert(ch->n_bytes_recved > 0); tt_assert(ch->n_cells_recved > 0); @@ -555,7 +537,6 @@ test_channel_dumpstats(void *arg) ch = NULL; done: - tor_free(cell); free_fake_channel(ch); UNMOCK(scheduler_channel_doesnt_want_writes); @@ -652,7 +633,7 @@ static void test_channel_lifecycle(void *arg) { channel_t *ch1 = NULL, *ch2 = NULL; - cell_t *cell = NULL; + packed_cell_t *p_cell = NULL; int old_count, init_doesnt_want_writes_count; int init_releases_count; @@ -685,10 +666,9 @@ test_channel_lifecycle(void *arg) tt_assert(ch1->registered); /* Try to write a cell through (should queue) */ - cell = tor_malloc_zero(sizeof(cell_t)); - make_fake_cell(cell); + p_cell = packed_cell_new(); old_count = test_cells_written; - channel_write_cell(ch1, cell); + channel_write_packed_cell(ch1, p_cell); tt_int_op(old_count, OP_EQ, test_cells_written); /* Move it to OPEN and flush */ @@ -916,122 +896,6 @@ test_channel_lifecycle_2(void *arg) } static void -test_channel_write(void *arg) -{ - channel_t *ch = NULL; - cell_t *cell = tor_malloc_zero(sizeof(cell_t)); - packed_cell_t *packed_cell = NULL; - var_cell_t *var_cell = - tor_malloc_zero(sizeof(var_cell_t) + CELL_PAYLOAD_SIZE); - int old_count; - - (void)arg; - - packed_cell = packed_cell_new(); - tt_assert(packed_cell); - - ch = new_fake_channel(); - tt_assert(ch); - make_fake_cell(cell); - make_fake_var_cell(var_cell); - - /* Tell it to accept cells */ - test_chan_accept_cells = 1; - - old_count = test_cells_written; - channel_write_cell(ch, cell); - cell = NULL; - tt_assert(test_cells_written == old_count + 1); - - channel_write_var_cell(ch, var_cell); - var_cell = NULL; - tt_assert(test_cells_written == old_count + 2); - - channel_write_packed_cell(ch, packed_cell); - packed_cell = NULL; - tt_assert(test_cells_written == old_count + 3); - - /* Now we test queueing; tell it not to accept cells */ - test_chan_accept_cells = 0; - /* ...and keep it from trying to flush the queue */ - ch->state = CHANNEL_STATE_MAINT; - - /* Get a fresh cell */ - cell = tor_malloc_zero(sizeof(cell_t)); - make_fake_cell(cell); - - old_count = test_cells_written; - channel_write_cell(ch, cell); - tt_assert(test_cells_written == old_count); - - /* - * Now change back to open with channel_change_state() and assert that it - * gets drained from the queue. - */ - test_chan_accept_cells = 1; - channel_change_state_open(ch); - tt_assert(test_cells_written == old_count + 1); - - /* - * Check the note destroy case - */ - cell = tor_malloc_zero(sizeof(cell_t)); - make_fake_cell(cell); - cell->command = CELL_DESTROY; - - /* Set up the mock */ - MOCK(channel_note_destroy_not_pending, - channel_note_destroy_not_pending_mock); - - old_count = test_destroy_not_pending_calls; - channel_write_cell(ch, cell); - tt_assert(test_destroy_not_pending_calls == old_count + 1); - - /* Now send a non-destroy and check we don't call it */ - cell = tor_malloc_zero(sizeof(cell_t)); - make_fake_cell(cell); - channel_write_cell(ch, cell); - tt_assert(test_destroy_not_pending_calls == old_count + 1); - - UNMOCK(channel_note_destroy_not_pending); - - /* - * Now switch it to CLOSING so we can test the discard-cells case - * in the channel_write_*() functions. - */ - MOCK(scheduler_release_channel, scheduler_release_channel_mock); - channel_mark_for_close(ch); - UNMOCK(scheduler_release_channel); - - /* Send cells that will drop in the closing state */ - old_count = test_cells_written; - - cell = tor_malloc_zero(sizeof(cell_t)); - make_fake_cell(cell); - channel_write_cell(ch, cell); - cell = NULL; - tt_assert(test_cells_written == old_count); - - var_cell = tor_malloc_zero(sizeof(var_cell_t) + CELL_PAYLOAD_SIZE); - make_fake_var_cell(var_cell); - channel_write_var_cell(ch, var_cell); - var_cell = NULL; - tt_assert(test_cells_written == old_count); - - packed_cell = packed_cell_new(); - channel_write_packed_cell(ch, packed_cell); - packed_cell = NULL; - tt_assert(test_cells_written == old_count); - - done: - free_fake_channel(ch); - tor_free(var_cell); - tor_free(cell); - packed_cell_free(packed_cell); - return; -} - -static void test_channel_id_map(void *arg) { (void)arg; @@ -1142,7 +1006,6 @@ struct testcase_t channel_tests[] = { { "incoming", test_channel_incoming, TT_FORK, NULL, NULL }, { "lifecycle", test_channel_lifecycle, TT_FORK, NULL, NULL }, { "lifecycle_2", test_channel_lifecycle_2, TT_FORK, NULL, NULL }, - { "write", test_channel_write, TT_FORK, NULL, NULL }, { "id_map", test_channel_id_map, TT_FORK, NULL, NULL }, END_OF_TESTCASES };

1 0

[tor/master] channel: Add and cleanup comments
by nickm＠torproject.org 08 Dec '17

08 Dec '17

commit 0e7b23535c875042d23af4f245bb72b0f90379d9 Author: David Goulet <dgoulet(a)torproject.org> Date: Tue Nov 21 13:59:29 2017 -0500 channel: Add and cleanup comments No code nor behavior change, only documentation. Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/or/channel.c | 93 ++++++++++++++++++-------------------------------------- 1 file changed, 29 insertions(+), 64 deletions(-) diff --git a/src/or/channel.c b/src/or/channel.c index 192ca57c6..e8e7805e6 100644 --- a/src/or/channel.c +++ b/src/or/channel.c @@ -941,8 +941,6 @@ channel_free(channel_t *chan) chan->cmux = NULL; } - /* We're in CLOSED or ERROR, so the cell queue is already empty */ - tor_free(chan); } @@ -971,11 +969,6 @@ channel_listener_free(channel_listener_t *chan_l) /* Call a free method if there is one */ if (chan_l->free_fn) chan_l->free_fn(chan_l); - /* - * We're in CLOSED or ERROR, so the incoming channel queue is already - * empty. - */ - tor_free(chan_l); } @@ -1143,8 +1136,7 @@ channel_get_var_cell_handler(channel_t *chan) * Set both cell handlers for a channel * * This function sets both the fixed-length and variable length cell handlers - * for a channel and processes any incoming cells that had been blocked in the - * queue because none were available. + * for a channel. */ void @@ -1622,10 +1614,10 @@ channel_set_remote_end(channel_t *chan, } /** - * Write to a channel based on a cell_queue_entry_t + * Write to a channel the given packed cell. * - * Given a cell_queue_entry_t filled out by the caller, try to send the cell - * and queue it if we can't. + * Return 0 on success and the cell is freed so the caller MUST discard any + * reference to it. On error, -1 is returned and the cell is untouched. */ static int write_packed_cell(channel_t *chan, packed_cell_t *cell) @@ -1738,15 +1730,6 @@ channel_change_state_(channel_t *chan, channel_state_t to_state) tor_assert(chan->reason_for_closing != CHANNEL_NOT_CLOSING); } - /* - * We need to maintain the queues here for some transitions: - * when we enter CHANNEL_STATE_OPEN (especially from CHANNEL_STATE_MAINT) - * we may have a backlog of cells to transmit, so drain the queues in - * that case, and when going to CHANNEL_STATE_CLOSED the subclass - * should have made sure to finish sending things (or gone to - * CHANNEL_STATE_ERROR if not possible), so we assert for that here. - */ - log_debug(LD_CHANNEL, "Changing state of channel %p (global ID " U64_FORMAT ") from \"%s\" to \"%s\"", @@ -1867,15 +1850,6 @@ channel_listener_change_state(channel_listener_t *chan_l, tor_assert(chan_l->reason_for_closing != CHANNEL_LISTENER_NOT_CLOSING); } - /* - * We need to maintain the queues here for some transitions: - * when we enter CHANNEL_STATE_OPEN (especially from CHANNEL_STATE_MAINT) - * we may have a backlog of cells to transmit, so drain the queues in - * that case, and when going to CHANNEL_STATE_CLOSED the subclass - * should have made sure to finish sending things (or gone to - * CHANNEL_STATE_ERROR if not possible), so we assert for that here. - */ - log_debug(LD_CHANNEL, "Changing state of channel listener %p (global ID " U64_FORMAT "from \"%s\" to \"%s\"", @@ -1908,23 +1882,32 @@ channel_listener_change_state(channel_listener_t *chan_l, if (to_state == CHANNEL_LISTENER_STATE_CLOSED || to_state == CHANNEL_LISTENER_STATE_ERROR) { - /* Assert that the queue is empty */ tor_assert(!(chan_l->incoming_list) || smartlist_len(chan_l->incoming_list) == 0); } } -/** - * Try to flush cells to the lower layer - * - * this is called by the lower layer to indicate that it wants more cells; - * it will try to write up to num_cells cells from the channel's cell queue or - * from circuits active on that channel, or as many as it has available if - * num_cells == -1. - */ - +/* Maximum number of cells that is allowed to flush at once withing + * channel_flush_some_cells(). */ #define MAX_CELLS_TO_GET_FROM_CIRCUITS_FOR_UNLIMITED 256 +/* Try to flush cells of the given channel chan up to a maximum of num_cells. + * + * This is called by the scheduler when it wants to flush cells from the + * channel's circuit queue(s) to the connection outbuf (not yet on the wire). + * + * If the channel is not in state CHANNEL_STATE_OPEN, this does nothing and + * will return 0 meaning no cells were flushed. + * + * If num_cells is -1, we'll try to flush up to the maximum cells allowed + * defined in MAX_CELLS_TO_GET_FROM_CIRCUITS_FOR_UNLIMITED. + * + * On success, the number of flushed cells are returned and it can never be + * above num_cells. If 0 is returned, no cells were flushed either because the + * channel was not opened or we had no cells on the channel. A negative number + * can NOT be sent back. + * + * This function is part of the fast path. */ MOCK_IMPL(ssize_t, channel_flush_some_cells, (channel_t *chan, ssize_t num_cells)) { @@ -1965,16 +1948,14 @@ channel_flush_some_cells, (channel_t *chan, ssize_t num_cells)) /** * Check if any cells are available * - * This gets used from the lower layer to check if any more cells are - * available. + * This is used by the scheduler to know if the channel has more to flush + * after a scheduling round. */ - MOCK_IMPL(int, channel_more_to_flush, (channel_t *chan)) { tor_assert(chan); - /* Check if any circuits would like to queue some */ if (circuitmux_num_cells(chan->cmux) > 0) return 1; /* Else no */ @@ -2178,12 +2159,8 @@ channel_listener_queue_incoming(channel_listener_t *listener, } /** - * Process queued incoming cells - * - * Process as many queued cells as we can from the incoming - * cell queue. + * Process a cell from the given channel. */ - void channel_process_cell(channel_t *chan, cell_t *cell) { @@ -2196,15 +2173,6 @@ channel_process_cell(channel_t *chan, cell_t *cell) if (!chan->cell_handler) return; - /* - * Process the given cell - * - * We must free the cells here after calling the handler, since custody - * of the buffer was given to the channel layer when they were queued; - * see comments on memory management in channel_queue_cell() and in - * channel_queue_var_cell() below. - */ - /* Timestamp for receiving */ channel_timestamp_recv(chan); /* Update received counter. */ @@ -3154,13 +3122,10 @@ channel_get_addr_if_possible(channel_t *chan, tor_addr_t *addr_out) else return 0; } -/** - * Check if there are outgoing queue writes on this channel - * - * Indicate if either we have queued cells, or if not, whether the underlying - * lower-layer transport thinks it has an output queue. +/* + * Return true iff the channel has any cells on the connection outbuf waiting + * to be sent onto the network. */ - int channel_has_queued_writes(channel_t *chan) {

1 0

[tor/master] relay: Remove dead code
by nickm＠torproject.org 08 Dec '17

08 Dec '17

commit bf242ebe6c7e13cf30fedc006e8d25597f9e602d Author: David Goulet <dgoulet(a)torproject.org> Date: Mon Nov 20 15:46:51 2017 -0500 relay: Remove dead code append_cell_to_circuit_queue() had code disabled from commit 2a95f3171681ee53c97ccba9d80f4454b462aaa7 This code is 4+ years old related to bug #9072 so if we ever want to revisit it, lets inspect/revert this commit. Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/or/relay.c | 100 --------------------------------------------------------- 1 file changed, 100 deletions(-) diff --git a/src/or/relay.c b/src/or/relay.c index 09f70793d..eb9d03080 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -2858,20 +2858,6 @@ channel_flush_from_first_active_circuit, (channel_t *chan, int max)) return n_flushed; } -#if 0 -/** Indicate the current preferred cap for middle circuits; zero disables - * the cap. Right now it's just a constant, ORCIRC_MAX_MIDDLE_CELLS, but - * the logic in append_cell_to_circuit_queue() is written to be correct - * if we want to base it on a consensus param or something that might change - * in the future. - */ -static int -get_max_middle_cells(void) -{ - return ORCIRC_MAX_MIDDLE_CELLS; -} -#endif /* 0 */ - /** Add cell to the queue of circ writing to chan * transmitting in direction. */ void @@ -2882,10 +2868,6 @@ append_cell_to_circuit_queue(circuit_t *circ, channel_t *chan, or_circuit_t *orcirc = NULL; cell_queue_t *queue; int streams_blocked; -#if 0 - uint32_t tgt_max_middle_cells, p_len, n_len, tmp, hard_max_middle_cells; -#endif - int exitward; if (circ->marked_for_close) return; @@ -2900,88 +2882,6 @@ append_cell_to_circuit_queue(circuit_t *circ, channel_t *chan, streams_blocked = circ->streams_blocked_on_p_chan; } - /* - * Disabling this for now because of a possible guard discovery attack - */ -#if 0 - /* Are we a middle circuit about to exceed ORCIRC_MAX_MIDDLE_CELLS? */ - if ((circ->n_chan != NULL) && CIRCUIT_IS_ORCIRC(circ)) { - orcirc = TO_OR_CIRCUIT(circ); - if (orcirc->p_chan) { - /* We are a middle circuit if we have both n_chan and p_chan */ - /* We'll need to know the current preferred maximum */ - tgt_max_middle_cells = get_max_middle_cells(); - if (tgt_max_middle_cells > 0) { - /* Do we need to initialize middle_max_cells? */ - if (orcirc->max_middle_cells == 0) { - orcirc->max_middle_cells = tgt_max_middle_cells; - } else { - if (tgt_max_middle_cells > orcirc->max_middle_cells) { - /* If we want to increase the cap, we can do so right away */ - orcirc->max_middle_cells = tgt_max_middle_cells; - } else if (tgt_max_middle_cells < orcirc->max_middle_cells) { - /* - * If we're shrinking the cap, we can't shrink past either queue; - * compare tgt_max_middle_cells rather than tgt_max_middle_cells * - * ORCIRC_MAX_MIDDLE_KILL_THRESH so the queues don't shrink enough - * to generate spurious warnings, either. - */ - n_len = circ->n_chan_cells.n; - p_len = orcirc->p_chan_cells.n; - tmp = tgt_max_middle_cells; - if (tmp < n_len) tmp = n_len; - if (tmp < p_len) tmp = p_len; - orcirc->max_middle_cells = tmp; - } - /* else no change */ - } - } else { - /* tgt_max_middle_cells == 0 indicates we should disable the cap */ - orcirc->max_middle_cells = 0; - } - - /* Now we know orcirc->max_middle_cells is set correctly */ - if (orcirc->max_middle_cells > 0) { - hard_max_middle_cells = - (uint32_t)(((double)orcirc->max_middle_cells) * - ORCIRC_MAX_MIDDLE_KILL_THRESH); - - if ((unsigned)queue->n + 1 >= hard_max_middle_cells) { - /* Queueing this cell would put queue over the kill theshold */ - log_warn(LD_CIRC, - "Got a cell exceeding the hard cap of %u in the " - "%s direction on middle circ ID %u on chan ID " - U64_FORMAT "; killing the circuit.", - hard_max_middle_cells, - (direction == CELL_DIRECTION_OUT) ? "n" : "p", - (direction == CELL_DIRECTION_OUT) ? - circ->n_circ_id : orcirc->p_circ_id, - U64_PRINTF_ARG( - (direction == CELL_DIRECTION_OUT) ? - circ->n_chan->global_identifier : - orcirc->p_chan->global_identifier)); - circuit_mark_for_close(circ, END_CIRC_REASON_RESOURCELIMIT); - return; - } else if ((unsigned)queue->n + 1 == orcirc->max_middle_cells) { - /* Only use ==, not >= for this test so we don't spam the log */ - log_warn(LD_CIRC, - "While trying to queue a cell, reached the soft cap of %u " - "in the %s direction on middle circ ID %u " - "on chan ID " U64_FORMAT ".", - orcirc->max_middle_cells, - (direction == CELL_DIRECTION_OUT) ? "n" : "p", - (direction == CELL_DIRECTION_OUT) ? - circ->n_circ_id : orcirc->p_circ_id, - U64_PRINTF_ARG( - (direction == CELL_DIRECTION_OUT) ? - circ->n_chan->global_identifier : - orcirc->p_chan->global_identifier)); - } - } - } - } -#endif /* 0 */ - cell_queue_append_packed_copy(circ, queue, exitward, cell, chan->wide_circ_ids, 1);

1 0

[tor/master] channel: Remove nickname attribute from channel_t
by nickm＠torproject.org 08 Dec '17

08 Dec '17

commit 1dc4f96d9c630aa562a15958b29848a25b458c84 Author: David Goulet <dgoulet(a)torproject.org> Date: Tue Nov 21 15:13:20 2017 -0500 channel: Remove nickname attribute from channel_t This was never set thus never could have been used. Get rid of it to simplify the code. Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/or/channel.c | 40 +++++++++++----------------------------- src/or/channel.h | 3 --- src/or/circuitbuild.c | 3 +-- src/or/circuitlist.c | 3 +-- 4 files changed, 13 insertions(+), 36 deletions(-) diff --git a/src/or/channel.c b/src/or/channel.c index 3a08d4cb0..ffa827064 100644 --- a/src/or/channel.c +++ b/src/or/channel.c @@ -1389,7 +1389,7 @@ channel_set_identity_digest(channel_t *chan, } /** - * Clear the remote end metadata (identity_digest/nickname) of a channel + * Clear the remote end metadata (identity_digest) of a channel * * This function clears all the remote end info from a channel; this is * intended for use by the lower layer. @@ -1416,7 +1416,6 @@ channel_clear_remote_end(channel_t *chan) memset(chan->identity_digest, 0, sizeof(chan->identity_digest)); - tor_free(chan->nickname); } /** @@ -2583,35 +2582,18 @@ channel_dump_statistics, (channel_t *chan, int severity)) U64_PRINTF_ARG(chan->timestamp_active), U64_PRINTF_ARG(now - chan->timestamp_active)); - /* Handle digest and nickname */ + /* Handle digest. */ if (!tor_digest_is_zero(chan->identity_digest)) { - if (chan->nickname) { - tor_log(severity, LD_GENERAL, - " * Channel " U64_FORMAT " says it is connected " - "to an OR with digest %s and nickname %s", - U64_PRINTF_ARG(chan->global_identifier), - hex_str(chan->identity_digest, DIGEST_LEN), - chan->nickname); - } else { - tor_log(severity, LD_GENERAL, - " * Channel " U64_FORMAT " says it is connected " - "to an OR with digest %s and no known nickname", - U64_PRINTF_ARG(chan->global_identifier), - hex_str(chan->identity_digest, DIGEST_LEN)); - } + tor_log(severity, LD_GENERAL, + " * Channel " U64_FORMAT " says it is connected " + "to an OR with digest %s", + U64_PRINTF_ARG(chan->global_identifier), + hex_str(chan->identity_digest, DIGEST_LEN)); } else { - if (chan->nickname) { - tor_log(severity, LD_GENERAL, - " * Channel " U64_FORMAT " does not know the digest" - " of the OR it is connected to, but reports its nickname is %s", - U64_PRINTF_ARG(chan->global_identifier), - chan->nickname); - } else { - tor_log(severity, LD_GENERAL, - " * Channel " U64_FORMAT " does not know the digest" - " or the nickname of the OR it is connected to", - U64_PRINTF_ARG(chan->global_identifier)); - } + tor_log(severity, LD_GENERAL, + " * Channel " U64_FORMAT " does not know the digest" + " of the OR it is connected to", + U64_PRINTF_ARG(chan->global_identifier)); } /* Handle remote address and descriptions */ diff --git a/src/or/channel.h b/src/or/channel.h index 1dc378b3d..5a2457faf 100644 --- a/src/or/channel.h +++ b/src/or/channel.h @@ -255,9 +255,6 @@ struct channel_s { */ ed25519_public_key_t ed25519_identity; - /** Nickname of the OR on the other side, or NULL if none. */ - char *nickname; - /** * Linked list of channels with the same RSA identity digest, for use with * the digest->channel map diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 2e6b63b4d..4e9d2457c 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -631,8 +631,7 @@ circuit_n_chan_done(channel_t *chan, int status, int close_origin_circuits) tor_assert(chan); - log_debug(LD_CIRC,"chan to %s/%s, status=%d", - chan->nickname ? chan->nickname : "NULL", + log_debug(LD_CIRC,"chan to %s, status=%d", channel_get_canonical_remote_descr(chan), status); pending_circs = smartlist_new(); diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index d37d986f1..c879fdada 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -505,8 +505,7 @@ circuit_count_pending_on_channel(channel_t *chan) circuit_get_all_pending_on_channel(sl, chan); cnt = smartlist_len(sl); smartlist_free(sl); - log_debug(LD_CIRC,"or_conn to %s at %s, %d pending circs", - chan->nickname ? chan->nickname : "NULL", + log_debug(LD_CIRC,"or_conn to %s, %d pending circs", channel_get_canonical_remote_descr(chan), cnt); return cnt;

1 0

[tor/master] test: Improve the inbound channel cell test
by nickm＠torproject.org 08 Dec '17

08 Dec '17

commit 636eec32bf4cf39ccc6d2866d560837b5d5f9218 Author: David Goulet <dgoulet(a)torproject.org> Date: Tue Nov 21 16:06:46 2017 -0500 test: Improve the inbound channel cell test First, that test was broken from the previous commit because the channel_queue_cell() has been removed. This now tests the channel_process_cell() directly. Second, it wasn't testing much except if the channel subsystem actually went through the cell handler. This commit adds more checks on the state of a channel going from open, receiving a cell and closing. Third, this and the id_map unit test are working, not the others so they've been marked as not working and future commit will improve and fix those. Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/test/test_channel.c | 139 +++++++++++++++++++++++++++--------------------- 1 file changed, 79 insertions(+), 60 deletions(-) diff --git a/src/test/test_channel.c b/src/test/test_channel.c index bc173472e..008c31b6b 100644 --- a/src/test/test_channel.c +++ b/src/test/test_channel.c @@ -33,16 +33,14 @@ static double test_overhead_estimate = 1.0; static int test_releases_count = 0; static channel_t *dump_statistics_mock_target = NULL; static int dump_statistics_mock_matches = 0; +static int test_close_called = 0; static void chan_test_channel_dump_statistics_mock( channel_t *chan, int severity); -static void chan_test_cell_handler(channel_t *ch, - cell_t *cell); static const char * chan_test_describe_transport(channel_t *ch); static void chan_test_dumpstats(channel_t *ch, int severity); static void chan_test_var_cell_handler(channel_t *ch, var_cell_t *var_cell); -static void chan_test_close(channel_t *ch); static void chan_test_error(channel_t *ch); static void chan_test_finish_close(channel_t *ch); static const char * chan_test_get_remote_descr(channel_t *ch, int flags); @@ -56,7 +54,6 @@ static int chan_test_write_var_cell(channel_t *ch, var_cell_t *var_cell); static void scheduler_channel_doesnt_want_writes_mock(channel_t *ch); static void test_channel_dumpstats(void *arg); -static void test_channel_incoming(void *arg); static void test_channel_lifecycle(void *arg); static const char * @@ -93,10 +90,9 @@ chan_test_channel_dump_statistics_mock(channel_t *chan, int severity) */ static void -chan_test_cell_handler(channel_t *ch, - cell_t *cell) +chan_test_cell_handler(channel_t *chan, cell_t *cell) { - tt_assert(ch); + tt_assert(chan); tt_assert(cell); test_chan_last_seen_fixed_cell_ptr = cell; @@ -146,6 +142,8 @@ chan_test_close(channel_t *ch) { tt_assert(ch); + ++test_close_called; + done: return; } @@ -348,6 +346,8 @@ new_fake_channel(void) chan->write_var_cell = chan_test_write_var_cell; chan->state = CHANNEL_STATE_OPEN; + chan->cmux = circuitmux_alloc(); + return chan; } @@ -516,6 +516,7 @@ test_channel_dumpstats(void *arg) chan_test_var_cell_handler); p_cell = packed_cell_new(); old_count = test_chan_fixed_cells_recved; + channel_write_packed_cell(ch, p_cell); tt_int_op(test_chan_fixed_cells_recved, OP_EQ, old_count + 1); tt_assert(ch->n_bytes_recved > 0); tt_assert(ch->n_cells_recved > 0); @@ -545,82 +546,93 @@ test_channel_dumpstats(void *arg) return; } +/* Test inbound cell. The callstack is: + * channel_process_cell() + * -> chan->cell_handler() + * + * This test is about checking if we can process an inbound cell down to the + * channel handler. */ static void -test_channel_incoming(void *arg) +test_channel_inbound_cell(void *arg) { - channel_t *ch = NULL; + channel_t *chan = NULL; cell_t *cell = NULL; - var_cell_t *var_cell = NULL; int old_count; - (void)arg; + (void) arg; - /* Mock these for duration of the test */ - MOCK(scheduler_channel_doesnt_want_writes, - scheduler_channel_doesnt_want_writes_mock); - MOCK(scheduler_release_channel, - scheduler_release_channel_mock); + /* The channel will be freed so we need to hijack this so the scheduler + * doesn't get confused. */ + MOCK(scheduler_release_channel, scheduler_release_channel_mock); /* Accept cells to lower layer */ test_chan_accept_cells = 1; /* Use default overhead factor */ test_overhead_estimate = 1.0; - ch = new_fake_channel(); - tt_assert(ch); + chan = new_fake_channel(); + tt_assert(chan); /* Start it off in OPENING */ - ch->state = CHANNEL_STATE_OPENING; - /* We'll need a cmux */ - ch->cmux = circuitmux_alloc(); - - /* Install incoming cell handlers */ - channel_set_cell_handlers(ch, - chan_test_cell_handler, - chan_test_var_cell_handler); - /* Test cell handler getters */ - tt_ptr_op(channel_get_cell_handler(ch), OP_EQ, chan_test_cell_handler); - tt_ptr_op(channel_get_var_cell_handler(ch), OP_EQ, - chan_test_var_cell_handler); + chan->state = CHANNEL_STATE_OPENING; /* Try to register it */ - channel_register(ch); - tt_assert(ch->registered); + channel_register(chan); + tt_int_op(chan->registered, OP_EQ, 1); /* Open it */ - channel_change_state_open(ch); - tt_int_op(ch->state, OP_EQ, CHANNEL_STATE_OPEN); + channel_change_state_open(chan); + tt_int_op(chan->state, OP_EQ, CHANNEL_STATE_OPEN); + tt_int_op(chan->has_been_open, OP_EQ, 1); - /* Receive a fixed cell */ - cell = tor_malloc_zero(sizeof(cell_t)); + /* Receive a cell now. */ + cell = tor_malloc_zero(sizeof(*cell)); make_fake_cell(cell); old_count = test_chan_fixed_cells_recved; - tor_free(cell); + channel_process_cell(chan, cell); + tt_int_op(test_chan_fixed_cells_recved, OP_EQ, old_count); + tt_u64_op(chan->timestamp_xfer_ms, OP_EQ, 0); + tt_u64_op(chan->timestamp_active, OP_EQ, 0); + tt_u64_op(chan->timestamp_recv, OP_EQ, 0); + + /* Setup incoming cell handlers. We don't care about var cell, the channel + * layers is not handling those. */ + channel_set_cell_handlers(chan, chan_test_cell_handler, NULL); + tt_ptr_op(chan->cell_handler, OP_EQ, chan_test_cell_handler); + /* Now process the cell, we should see it. */ + old_count = test_chan_fixed_cells_recved; + channel_process_cell(chan, cell); tt_int_op(test_chan_fixed_cells_recved, OP_EQ, old_count + 1); - - /* Receive a variable-size cell */ - var_cell = tor_malloc_zero(sizeof(var_cell_t) + CELL_PAYLOAD_SIZE); - make_fake_var_cell(var_cell); - old_count = test_chan_var_cells_recved; - tor_free(cell); - tt_int_op(test_chan_var_cells_recved, OP_EQ, old_count + 1); + /* We should have a series of timestamp set. */ + tt_u64_op(chan->timestamp_xfer_ms, OP_NE, 0); + tt_u64_op(chan->timestamp_active, OP_NE, 0); + tt_u64_op(chan->timestamp_recv, OP_NE, 0); + tt_u64_op(chan->next_padding_time_ms, OP_EQ, 0); + tt_u64_op(chan->n_cells_recved, OP_EQ, 1); + tt_u64_op(chan->n_bytes_recved, OP_EQ, get_cell_network_size(0)); /* Close it */ - channel_mark_for_close(ch); - tt_int_op(ch->state, OP_EQ, CHANNEL_STATE_CLOSING); - chan_test_finish_close(ch); - tt_int_op(ch->state, OP_EQ, CHANNEL_STATE_CLOSED); + old_count = test_close_called; + channel_mark_for_close(chan); + tt_int_op(chan->state, OP_EQ, CHANNEL_STATE_CLOSING); + tt_int_op(chan->reason_for_closing, OP_EQ, CHANNEL_CLOSE_REQUESTED); + tt_int_op(test_close_called, OP_EQ, old_count + 1); + + /* This closes the channe so it calls in the scheduler, make sure of it. */ + old_count = test_releases_count; + chan_test_finish_close(chan); + tt_int_op(test_releases_count, OP_EQ, old_count + 1); + tt_int_op(chan->state, OP_EQ, CHANNEL_STATE_CLOSED); + + /* The channel will be free, lets make sure it is not accessible. */ + uint64_t chan_id = chan->global_identifier; + tt_ptr_op(channel_find_by_global_id(chan_id), OP_EQ, chan); channel_run_cleanup(); - ch = NULL; + chan = channel_find_by_global_id(chan_id); + tt_assert(chan == NULL); done: - free_fake_channel(ch); tor_free(cell); - tor_free(var_cell); - - UNMOCK(scheduler_channel_doesnt_want_writes); UNMOCK(scheduler_release_channel); - - return; } /** @@ -1002,11 +1014,18 @@ test_channel_id_map(void *arg) } struct testcase_t channel_tests[] = { - { "dumpstats", test_channel_dumpstats, TT_FORK, NULL, NULL }, - { "incoming", test_channel_incoming, TT_FORK, NULL, NULL }, - { "lifecycle", test_channel_lifecycle, TT_FORK, NULL, NULL }, - { "lifecycle_2", test_channel_lifecycle_2, TT_FORK, NULL, NULL }, - { "id_map", test_channel_id_map, TT_FORK, NULL, NULL }, + { "inbound_cell", test_channel_inbound_cell, TT_FORK, + NULL, NULL }, + { "id_map", test_channel_id_map, TT_FORK, + NULL, NULL }, + + /* NOT WORKING TEST. */ + { "dumpstats", test_channel_dumpstats, TT_FORK, + NULL, NULL }, + { "lifecycle", test_channel_lifecycle, TT_FORK, + NULL, NULL }, + { "lifecycle_2", test_channel_lifecycle_2, TT_FORK, + NULL, NULL }, END_OF_TESTCASES };

1 0

[tor/master] test: Add channel state unit test
by nickm＠torproject.org 08 Dec '17

08 Dec '17

commit 47aaaf44032aa689dddbc13d07794b50a603c883 Author: David Goulet <dgoulet(a)torproject.org> Date: Wed Nov 22 12:10:08 2017 -0500 test: Add channel state unit test Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/test/test_channel.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 139 insertions(+) diff --git a/src/test/test_channel.c b/src/test/test_channel.c index 5a20dbcf2..203fa1319 100644 --- a/src/test/test_channel.c +++ b/src/test/test_channel.c @@ -1142,6 +1142,143 @@ test_channel_id_map(void *arg) #undef N_CHAN } +static void +test_channel_state(void *arg) +{ + (void) arg; + + /* Test state validity. */ + tt_int_op(channel_state_is_valid(CHANNEL_STATE_CLOSED), OP_EQ, 1); + tt_int_op(channel_state_is_valid(CHANNEL_STATE_CLOSING), OP_EQ, 1); + tt_int_op(channel_state_is_valid(CHANNEL_STATE_ERROR), OP_EQ, 1); + tt_int_op(channel_state_is_valid(CHANNEL_STATE_OPEN), OP_EQ, 1); + tt_int_op(channel_state_is_valid(CHANNEL_STATE_OPENING), OP_EQ, 1); + tt_int_op(channel_state_is_valid(CHANNEL_STATE_MAINT), OP_EQ, 1); + tt_int_op(channel_state_is_valid(CHANNEL_STATE_LAST), OP_EQ, 0); + tt_int_op(channel_state_is_valid(INT_MAX), OP_EQ, 0); + + /* Test listener state validity. */ + tt_int_op(channel_listener_state_is_valid(CHANNEL_LISTENER_STATE_CLOSED), + OP_EQ, 1); + tt_int_op(channel_listener_state_is_valid(CHANNEL_LISTENER_STATE_LISTENING), + OP_EQ, 1); + tt_int_op(channel_listener_state_is_valid(CHANNEL_LISTENER_STATE_CLOSING), + OP_EQ, 1); + tt_int_op(channel_listener_state_is_valid(CHANNEL_LISTENER_STATE_ERROR), + OP_EQ, 1); + tt_int_op(channel_listener_state_is_valid(CHANNEL_LISTENER_STATE_LAST), + OP_EQ, 0); + tt_int_op(channel_listener_state_is_valid(INT_MAX), OP_EQ, 0); + + /* Test state transition. */ + tt_int_op(channel_state_can_transition(CHANNEL_STATE_CLOSED, + CHANNEL_STATE_OPENING), OP_EQ, 1); + tt_int_op(channel_state_can_transition(CHANNEL_STATE_CLOSED, + CHANNEL_STATE_ERROR), OP_EQ, 0); + tt_int_op(channel_state_can_transition(CHANNEL_STATE_CLOSING, + CHANNEL_STATE_ERROR), OP_EQ, 1); + tt_int_op(channel_state_can_transition(CHANNEL_STATE_CLOSING, + CHANNEL_STATE_CLOSED), OP_EQ, 1); + tt_int_op(channel_state_can_transition(CHANNEL_STATE_CLOSING, + CHANNEL_STATE_OPEN), OP_EQ, 0); + tt_int_op(channel_state_can_transition(CHANNEL_STATE_MAINT, + CHANNEL_STATE_CLOSING), OP_EQ, 1); + tt_int_op(channel_state_can_transition(CHANNEL_STATE_MAINT, + CHANNEL_STATE_ERROR), OP_EQ, 1); + tt_int_op(channel_state_can_transition(CHANNEL_STATE_MAINT, + CHANNEL_STATE_OPEN), OP_EQ, 1); + tt_int_op(channel_state_can_transition(CHANNEL_STATE_MAINT, + CHANNEL_STATE_OPENING), OP_EQ, 0); + tt_int_op(channel_state_can_transition(CHANNEL_STATE_OPENING, + CHANNEL_STATE_OPEN), OP_EQ, 1); + tt_int_op(channel_state_can_transition(CHANNEL_STATE_OPENING, + CHANNEL_STATE_CLOSING), OP_EQ, 1); + tt_int_op(channel_state_can_transition(CHANNEL_STATE_OPENING, + CHANNEL_STATE_ERROR), OP_EQ, 1); + tt_int_op(channel_state_can_transition(CHANNEL_STATE_OPEN, + CHANNEL_STATE_ERROR), OP_EQ, 1); + tt_int_op(channel_state_can_transition(CHANNEL_STATE_OPEN, + CHANNEL_STATE_CLOSING), OP_EQ, 1); + tt_int_op(channel_state_can_transition(CHANNEL_STATE_OPEN, + CHANNEL_STATE_ERROR), OP_EQ, 1); + tt_int_op(channel_state_can_transition(CHANNEL_STATE_OPEN, + CHANNEL_STATE_MAINT), OP_EQ, 1); + tt_int_op(channel_state_can_transition(CHANNEL_STATE_LAST, + CHANNEL_STATE_MAINT), OP_EQ, 0); + tt_int_op(channel_state_can_transition(CHANNEL_STATE_LAST, INT_MAX), + OP_EQ, 0); + + /* Test listener state transition. */ + tt_int_op(channel_listener_state_can_transition( + CHANNEL_LISTENER_STATE_CLOSED, + CHANNEL_LISTENER_STATE_LISTENING), + OP_EQ, 1); + tt_int_op(channel_listener_state_can_transition( + CHANNEL_LISTENER_STATE_CLOSED, + CHANNEL_LISTENER_STATE_ERROR), + OP_EQ, 0); + + tt_int_op(channel_listener_state_can_transition( + CHANNEL_LISTENER_STATE_CLOSING, + CHANNEL_LISTENER_STATE_CLOSED), + OP_EQ, 1); + + tt_int_op(channel_listener_state_can_transition( + CHANNEL_LISTENER_STATE_CLOSING, + CHANNEL_LISTENER_STATE_ERROR), + OP_EQ, 1); + tt_int_op(channel_listener_state_can_transition( + CHANNEL_LISTENER_STATE_ERROR, + CHANNEL_LISTENER_STATE_CLOSING), + OP_EQ, 0); + + tt_int_op(channel_listener_state_can_transition( + CHANNEL_LISTENER_STATE_LISTENING, + CHANNEL_LISTENER_STATE_CLOSING), + OP_EQ, 1); + tt_int_op(channel_listener_state_can_transition( + CHANNEL_LISTENER_STATE_LISTENING, + CHANNEL_LISTENER_STATE_ERROR), + OP_EQ, 1); + tt_int_op(channel_listener_state_can_transition( + CHANNEL_LISTENER_STATE_LAST, + INT_MAX), + OP_EQ, 0); + + /* Test state string. */ + tt_str_op(channel_state_to_string(CHANNEL_STATE_CLOSING), OP_EQ, + "closing"); + tt_str_op(channel_state_to_string(CHANNEL_STATE_ERROR), OP_EQ, + "channel error"); + tt_str_op(channel_state_to_string(CHANNEL_STATE_CLOSED), OP_EQ, + "closed"); + tt_str_op(channel_state_to_string(CHANNEL_STATE_OPEN), OP_EQ, + "open"); + tt_str_op(channel_state_to_string(CHANNEL_STATE_OPENING), OP_EQ, + "opening"); + tt_str_op(channel_state_to_string(CHANNEL_STATE_MAINT), OP_EQ, + "temporarily suspended for maintenance"); + tt_str_op(channel_state_to_string(CHANNEL_STATE_LAST), OP_EQ, + "unknown or invalid channel state"); + tt_str_op(channel_state_to_string(INT_MAX), OP_EQ, + "unknown or invalid channel state"); + + /* Test listener state string. */ + tt_str_op(channel_listener_state_to_string(CHANNEL_LISTENER_STATE_CLOSING), + OP_EQ, "closing"); + tt_str_op(channel_listener_state_to_string(CHANNEL_LISTENER_STATE_ERROR), + OP_EQ, "channel listener error"); + tt_str_op(channel_listener_state_to_string(CHANNEL_LISTENER_STATE_LISTENING), + OP_EQ, "listening"); + tt_str_op(channel_listener_state_to_string(CHANNEL_LISTENER_STATE_LAST), + OP_EQ, "unknown or invalid channel listener state"); + tt_str_op(channel_listener_state_to_string(INT_MAX), + OP_EQ, "unknown or invalid channel listener state"); + + done: + ; +} + struct testcase_t channel_tests[] = { { "inbound_cell", test_channel_inbound_cell, TT_FORK, NULL, NULL }, @@ -1155,6 +1292,8 @@ struct testcase_t channel_tests[] = { NULL, NULL }, { "dumpstats", test_channel_dumpstats, TT_FORK, NULL, NULL }, + { "state", test_channel_state, TT_FORK, + NULL, NULL }, END_OF_TESTCASES };

1 0