#31531 closed defect (fixed)

Make control_event_conf_changed() take a smartlist of config_line_t

Reported by: teor Owned by: neel
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: technical-debt, extra-review-completed
Cc: neel Actual Points: 0.1
Parent ID: Points: 0.5
Reviewer: catalyst, teor Sponsor: Sponsor31-can

Description

control_event_conf_changed() currently takes smartlist(k, v, k, v, …), which is an unexpected API.

We should change it so the keys and values are part of a config_line_t struct, and the smartlist contains config_line_t.

Child Tickets

Change History (13)

comment:1 Changed 13 months ago by neel

Cc: neel added
Owner: set to neel
Status: newassigned

comment:2 Changed 13 months ago by neel

Status: assignedneeds_review

comment:3 Changed 13 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.4.3.x-final

comment:4 Changed 12 months ago by dgoulet

Reviewer: catalyst

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

Status: needs_reviewneeds_revision

Replying to neel:

PR: https://github.com/torproject/tor/pull/1558

Thanks! This mostly looks good.

config_line_t is already a linked list; why not pass that linked list to control_event_conf_changed() instead of repackaging it into a smartlist? That seems to me like it would be simpler.

comment:6 Changed 12 months ago by neel

comment:7 Changed 12 months ago by neel

Status: needs_revisionneeds_review

comment:8 in reply to:  6 Changed 12 months ago by catalyst

Status: needs_reviewneeds_revision

Replying to neel:

New PR using config_line_t: https://github.com/torproject/tor/pull/1570

Thanks! That looks simpler. I made some comments on that pull request, mostly about minor things. Note that currently the pull request fails Travis due to a memory leak that I describe in the comments.

Please feel free to force-push this pull request with your updates.

comment:9 Changed 12 months ago by neel

Status: needs_revisionneeds_review

Fixed it.

comment:10 in reply to:  9 Changed 12 months ago by catalyst

Status: needs_reviewmerge_ready

Replying to neel:

Fixed it.

Thanks! Looks good now.

comment:11 Changed 12 months ago by teor

Keywords: extra-review added

Hi, I'm marking this pull request as needing extra review, because it's from an external contributor.

The merger should do another review before they merge.

comment:12 Changed 12 months ago by teor

Actual Points: 0.1
Keywords: extra-review-completed added; extra-review removed
Reviewer: catalystcatalyst, teor

I found a few issues with this patch:

  • the function comment for control_event_conf_changed() was not updated
  • the "elements" argument name is now confusing
  • the extra include in control_events.h could be replaced with a struct forward declaration

I also found a few issues in control_event_conf_changed() unrelated to this patch:

  • the function comment has not been maintained, even before this PR
  • the return value is always the same, and it is not checked by the caller

I fixed these issues in this PR:

These are all obviously correct minor changes, so any merger can merge after it passes CI.

comment:13 Changed 12 months ago by teor

Resolution: fixed
Status: merge_readyclosed

Merged to master.

Note: See TracTickets for help on using tickets.