Opened 4 months ago

Closed 3 months ago

#30007 closed task (fixed)

refactor control.c output to be more abstract

Reported by: catalyst Owned by: catalyst
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: technical-debt control-port dgoulet-merge
Cc: Actual Points:
Parent ID: #29210 Points: 3
Reviewer: nickm Sponsor: Sponsor31-can

Description

Lots of code explicitly assembles control protocol responses, including the numeric response codes, the line separators, etc.

We should abstract this better to simplify code and for greater consistency in the protocol.

Child Tickets

Change History (11)

comment:1 Changed 3 months ago by catalyst

WIP branch at https://github.com/tlyu/tor/tree/ticket30007

older WIP branch (superseded by recent control.c refactor) at https://github.com/tlyu/tor/tree/control-proto

The new iteration might use a Coccinelle script to rewrite calls to formatting functions.

comment:2 Changed 3 months ago by catalyst

The WIP branch at https://github.com/tlyu/tor/tree/ticket30007 now has a Coccinelle script that I ran to migrate existing code to using the new abstractions. It's almost ready to review. Once Nick rebases the #30091 branch I based it on, I'll probably make a WIP pull request to check Coveralls.

There are a few places that still explicitly use connection_write_str_to_buf() and connection_buf_add() to send replies to the control port, but they're somewhat more difficult to change. I'm tempted to defer them to a later ticket.

In particular, these could benefit from additional refactoring that includes helper functions that collect a list of strings (or key-value pairs, etc.) to write and automatically takes care of the 250-some_text vs 250 some_text, etc. distinction.

comment:4 Changed 3 months ago by nickm

Keywords: control-port added
Milestone: Tor: unspecifiedTor: 0.4.1.x-final
Reviewer: nickm

FWIW, I am happy to merge_ready this once the control-cmd branch is merge-readied. I want to do a final fine-grained review, but the broad strokes of this branch look great.

comment:5 Changed 3 months ago by catalyst

Status: assignedneeds_revision

I noticed it needs a changes file and a WIP notation deleted from a commit message. I also should rebase it after #30091 is merged and verify the Coccinelle output again.

comment:6 Changed 3 months ago by catalyst

Status: needs_revisionneeds_review

comment:7 Changed 3 months ago by nickm

This still lgtm; let's merge it once #30091 is in.

comment:8 Changed 3 months ago by catalyst

Force-pushed that pull request. Also re-verified the Coccinelle output. Waiting for CI to finish.

comment:9 in reply to:  8 Changed 3 months ago by catalyst

Replying to catalyst:

Force-pushed that pull request. Also re-verified the Coccinelle output. Waiting for CI to finish.

CI has passed.

comment:10 Changed 3 months ago by nickm

Keywords: dgoulet-merge added
Status: needs_reviewmerge_ready

comment:11 Changed 3 months ago by dgoulet

Resolution: fixed
Status: merge_readyclosed

Merged!

Note: See TracTickets for help on using tickets.