-
Notifications
You must be signed in to change notification settings - Fork 931
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ticket27678 #323
Ticket27678 #323
Conversation
This commit gives us a control port call to emit a CIRC_BW event for a single circuit instead of all circuits. This commit only moves code. No functionality has been changed.
We determine that a cell was dropped by inspecting CIRC_BW fields. If we did not update the delivered or overhead fields, the cell was dropped/not processed. Also emit CIRC_BW events for cases where we decide to close the circuit in this function, so vanguards can print messages about dropped cells in those cases, too.
src/core/or/command.c
Outdated
|
||
/* Stash the original delivered and overhead values, so we can inform the | ||
* control port about dropped cells immediately after processing below. */ | ||
orig_delivered_bw = ocirc->n_delivered_read_circ_bw; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What parts of the code can modify these values and we are keeping the originals? Perhaps we can put it in the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarified comment in 02b5a6c
src/core/or/command.c
Outdated
@@ -535,6 +542,7 @@ command_process_relay_cell(cell_t *cell, channel_t *chan) | |||
(unsigned)cell->circ_id); | |||
if (CIRCUIT_IS_ORIGIN(circ)) { | |||
circuit_log_path(LOG_WARN, LD_OR, TO_ORIGIN_CIRCUIT(circ)); | |||
control_event_circ_bandwidth_used_for_circ(TO_ORIGIN_CIRCUIT(circ)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We seem to be calling this function from three places. How can we know that these are the only places we need to call it from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We call it when we get an error to close the circuit, or when the codepaths called from circuit_receive_relay_cell() fails to update bw values (because the cell was not valid).
Comment 02b5a6c also clarifies this.
src/core/or/command.c
Outdated
* so the controller can react quickly. */ | ||
if (orig_delivered_bw == ocirc->n_delivered_read_circ_bw && | ||
orig_overhead_bw == ocirc->n_overhead_read_circ_bw) { | ||
control_event_circ_bandwidth_used_for_circ(ocirc); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it matter if we double-call this new function and emit two control events? Maybe we can add a guard in the function to make sure we don't double or triple call it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not matter if we call this function multiple times. It already has a guard to prevent redundant events. See updated comments in 66b05e3.
@@ -480,6 +480,8 @@ command_process_relay_cell(cell_t *cell, channel_t *chan) | |||
const or_options_t *options = get_options(); | |||
circuit_t *circ; | |||
int reason, direction; | |||
uint32_t orig_delivered_bw = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Commit msg is wrong, references 27679 instead of 27678.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I will fix this in a new branch. I don't like that commit summary anyway, since it doesn't change code.
Clarify comments about stored bw fields.
https://trac.torproject.org/projects/tor/ticket/27678