Make control_event_conf_changed() take a smartlist of config_line_t

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.

changed milestone to %Tor: 0.4.3.x-final

added actualpoints::0.1 component::core tor/tor extra-review-completed milestone::Tor: 0.4.3.x-final owner::neel points::0.5 priority::medium resolution::fixed reviewer::catalyst reviewer::teor severity::normal sponsor::31-can status::closed technical-debt type::defect labels

Trac:
Status: new to assigned
Cc: N/A to neel
Owner: N/A to neel

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

Trac:
Status: assigned to needs_review

Trac:
Milestone: Tor: unspecified to Tor: 0.4.3.x-final

Trac:
Reviewer: N/A to catalyst

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.

Trac:
Status: needs_review to needs_revision

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

Trac:
Status: needs_revision to needs_review

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.

Trac:
Status: needs_review to needs_revision

Fixed it.

Trac:
Status: needs_revision to needs_review

Replying to neel:

Fixed it. Thanks! Looks good now.

Trac:
Status: needs_review to merge_ready

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.

Trac:
Keywords: N/A deleted, extra-review added

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:

• master: https://github.com/torproject/tor/pull/1574

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

Trac:
Keywords: extra-review deleted, extra-review-completed added
Actualpoints: N/A to 0.1
Reviewer: catalyst to catalyst, teor

Merged to master.

Trac:
Status: merge_ready to closed
Resolution: N/A to fixed

closed

changed time estimate to 4h

added 48m of time spent

moved to tpo/core/tor#31531 (closed)