Opened 8 weeks ago

Last modified 6 weeks ago

#30984 accepted defect

Make a key-value line abstraction to output control replies

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

Description

A few controller commands still use connection_buf_add() or similar low-level functions after constructing a list of reply lines. Almost all of these are key-value pairs. Create a new abstraction to output these, including by automatically including the correct separator character between the numeric code and the rest of the line.

Child Tickets

Change History (8)

comment:1 Changed 8 weeks ago by nickm

Could we reuse config_line_t, or is there a mismatch?

comment:2 in reply to:  1 Changed 8 weeks ago by catalyst

Replying to nickm:

Could we reuse config_line_t, or is there a mismatch?

I think it would need to be extended or wrapped to add the numeric result code on each line. Some commands like MAPADDRESS can have multiple result codes in a single response. (This doesn't conform with control-spec.txt currently, but that's a different problem.) Also some replies are one key-value pair per line (e.g., 250-key=value from GETCONF), but some contain multiple key-value pairs (e.g., the ORCONN async reply).

Also for async events, there's a separate numeric event type code that's distinct from the 650 reply code.

comment:3 Changed 7 weeks ago by catalyst

Sketch so far:

  • New struct control_reply_line_t wraps config_line_t with additional fields for control protocol numeric reply codes and for control event async event type numbers (these are different)
  • Make smartlists of control_reply_line_t hold entire multiline control replies

comment:4 Changed 6 weeks ago by nickm

I'm not sure that wrapping config_line_t and putting the results in a smartlist_t is the route I would recommend here: config_line_t expects to be in a linked list.

comment:5 in reply to:  4 Changed 6 weeks ago by catalyst

Replying to nickm:

I'm not sure that wrapping config_line_t and putting the results in a smartlist_t is the route I would recommend here: config_line_t expects to be in a linked list.

I was thinking that each smartlist element would be a length-one linked list of config_line_t, under most circumstances. Exceptions would be reply lines with multiple machine-parsable key-value elements, like many of the async event control replies. For these, it would be nice to use kvline_encode().

There are three main categories of reply lines that I see (ignoring the difference between a MidReplyLine and EndReplyLine for now -- part of the goal of this is to automatically figure out which to generate):

  • Entire line after the numeric code and separator is meant to be human readable, and not generally machine-parsed
  • Entire line after the numeric code and separator is a single key=value pair
  • Line is a DataReplyLine, followed by multi-line CmdData
  • After numeric code and separator, a list of machine-parsable keywords or key=value pairs. These are mostly async control events, but there are some synchronous replies that are like this, including PROTOCOLINFO. These are the reply formats that would benefit most from kvline_encode().

Or maybe I should start by making a new structure for control replies, starting with reply code numbers and flat strings? Then stuff that could benefit by building up key=value argument lists could still use kvline_encode() explicitly, at least at first. Some of the existing control replies like GETINFO and GETCONF don't quote things the way kvline_encode() would like to.

comment:6 Changed 6 weeks ago by nickm

Thanks for explaining this to me; it makes a lot more sense to me now. Based on what you've said above, I think you should go with whatever approach seems best to you based on your analysis.

comment:7 Changed 6 weeks ago by catalyst

Status: assignedaccepted

Oops should have moved this to "accepted".

comment:8 Changed 6 weeks ago by catalyst

WIP branch in my GitHub repository: control-refactor; still needs some memory management work.

Note: See TracTickets for help on using tickets.