Opened 9 months ago

Closed 8 months ago

#29897 closed defect (fixed)

Refactor handle_get_next_bandwidth() to use connection_dir_buf_add()

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: consider-backport-after-authority-test, consider-backport-after-040-stable, tor-dirauth, metrics-needs, tor-bwauth, 035-removed-20180711, 040-roadmap-proposed, 040-backport
Cc: starlight@… Actual Points: 0.1
Parent ID: #21377 Points: 0.1
Reviewer: nickm Sponsor:

Description

After #21377 merges, we want to refactor handle_get_next_bandwidth() to use connection_dir_buf_add().

If we backport this refactor to 0.4.0 or earlier, we'll also need #29896 for the connection_dir_buf_add() backport.

Child Tickets

Change History (10)

comment:1 Changed 9 months ago by teor

Status: assignedneeds_review

I made PRs with #21377, #29896, and this change:
0.3.5: https://github.com/torproject/tor/pull/858
master: https://github.com/torproject/tor/pull/857 (testing, but maybe needs practracker?)

comment:2 Changed 9 months ago by teor

The last commit is new, the others have already been reviewed in #21377 and #28816.

comment:3 Changed 9 months ago by teor

Keywords: asn-merge nickm-merge added

comment:4 Changed 9 months ago by teor

I merged #21377 to 0.4.0 and later.
I could make a rebased branch for 0.4.0, but that seems like a waste of effort.

comment:5 Changed 9 months ago by teor

Keywords: 035-backport-maybe-in-21377 added; 035-backport-maybe removed

Here's the rebased branch for 0.4.0:
https://github.com/torproject/tor/pull/867

We'll backport to 0.3.5 in #21377.

comment:6 Changed 9 months ago by asn

Reviewer: nickm

comment:7 Changed 9 months ago by nickm

Status: needs_reviewmerge_ready

PR 867 looks okay -- I'm assuming that's what I'm supposed to review here? Please let me know if I reviewed the wrong thing.

Before we backport it, though, let's talk briefly about why we're doing this. We usually don't backport refactoring stuff.

comment:8 Changed 8 months ago by asn

Keywords: asn-merge removed

Merged to 040. Leaving open in case of backport.

comment:9 Changed 8 months ago by teor

Keywords: 040-backport added; nickm-merge 040-backport-maybe removed
Milestone: Tor: 0.4.1.x-finalTor: 0.3.5.x-final

asn, when you merge a ticket that might be backported, please move it into the milestone for the backport release.

comment:10 in reply to:  7 Changed 8 months ago by teor

Keywords: 035-backport-maybe-in-21377 removed
Milestone: Tor: 0.3.5.x-finalTor: 0.4.0.x-final
Resolution: fixed
Status: merge_readyclosed

Replying to nickm:

PR 867 looks okay -- I'm assuming that's what I'm supposed to review here? Please let me know if I reviewed the wrong thing.

Before we backport it, though, let's talk briefly about why we're doing this. We usually don't backport refactoring stuff.

You're right, the code should be equivalent, so we don't need to backport it.

I felt a little uncomfortable having different code in a bunch of different releases. But I don't think we can avoid that problem, because backporting refactoring is higher-risk than letting code diverge.

Note: See TracTickets for help on using tickets.