Opened 6 months ago

Closed 5 months ago

#30091 closed enhancement (fixed)

Unify parsing code for control.c

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

Description


Child Tickets

TicketTypeStatusOwnerSummary
#29984defectclosednickmController protocol parser can't find empty initial line

Change History (15)

comment:1 Changed 6 months ago by nickm

I have a WIP branch as control_command_refactor; there are a few more tests and commands to go.

comment:2 Changed 6 months ago by nickm

Sponsor: Sponsor31-can
Status: assignedneeds_review
Type: defectenhancement

Now it handles all the commands, and 'test-stem' passes. PR at https://github.com/torproject/tor/pull/919

comment:3 Changed 6 months ago by nickm

Actual Points: 3

comment:4 Changed 5 months ago by nickm

Per request from catalyst, I have rebased the branch as control_command_refactor_v2 and created a PR at https://github.com/torproject/tor/pull/940 .

comment:5 Changed 5 months ago by asn

Reviewer: catalyst

comment:6 Changed 5 months ago by catalyst

So far, I looked at

d1f5957c4e Improve handling of controller commands

through

e3fcbffbb8 Use parsing code for the simpler controller commands.

These look good. I commented on the pull request about a few technical debt issues and some minor spelling and naming things.

This set of commits seems to form a logical grouping and they could probably have been in their own child ticket/PR.

comment:7 Changed 5 months ago by catalyst

Looking at

bb2062dd76 kvline: handle empty alues as well as empty keys

through

5d3dcd6fc9 Update more controller commands, now that we have kvline support

Overall this looks good, and deletes a lot of repetitive code.

There's a typo in the bb2062dd76 summary.

See technical debt comment in the pull request for control_cmd_parse_args(), and maybe a few more minor comments.

comment:8 Changed 5 months ago by nickm

Okay, I've caught up on the requested revisions.

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

Replying to nickm:

Okay, I've caught up on the requested revisions.

Thanks! Those look good so far. I'm still working through the rest of that branch.

comment:10 Changed 5 months ago by catalyst

Looking at

3eed319e4f Use new parser logic for SETCONF/RESETCONF code.

through

62695a9307 squash! kvline: handle empty alues as well as empty keys

Overall this looks good! I like how much smaller control_setconf_helper() became.

I wrote a few small comments on the pull request.

comment:11 Changed 5 months ago by nickm

Updated the branch, responded to comments. Ready to go?

comment:12 in reply to:  11 Changed 5 months ago by catalyst

Status: needs_reviewmerge_ready

Replying to nickm:

Updated the branch, responded to comments. Ready to go?

Thanks! I left one more comment on the pull request, which you may address at your option.

comment:13 Changed 5 months ago by nickm

I've takedn your suggestion, and squashed the branch as control_command_refactor_v3.

New PR for merge at https://github.com/torproject/tor/pull/980 ; let's give CI another chance on it before we merge.

comment:14 Changed 5 months ago by nickm

Keywords: dgoulet-merge added

comment:15 Changed 5 months ago by dgoulet

Resolution: fixed
Status: merge_readyclosed

Merged! Woot

Note: See TracTickets for help on using tickets.