Ticket31684 #1328

ltbringer · 2019-09-16T18:44:35Z

Add control port GETINFO support for dumping the local consensus

Refer: (ticket)[https://trac.torproject.org/projects/tor/ticket/31684]

coveralls · 2019-09-16T19:28:17Z

Pull Request Test Coverage Report for Build 6594

22 of 27 (81.48%) changed or added relevant lines in 4 files are covered.
267 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.2%) to 63.145%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/feature/dircache/dirserv.c	0	1	0.0%
src/feature/control/control_getinfo.c	19	23	82.61%

Files with Coverage Reduction	New Missed Lines	%
src/feature/hs/hs_cell.c	60	41.79%
src/feature/dircache/dirserv.c	207	0.91%

Totals
Change from base Build 6587:	-0.2%
Covered Lines:	47835
Relevant Lines:	75754

💛 - Coveralls

- Allows control port to read microdesc consensus using: GETINFO dir/status-vote/microdesc/consensus refer: [ticket](https://trac.torproject.org/projects/tor/ticket/31684) refer: [comment](https://trac.torproject.org/projects/tor/ticket/31684#comment:6) add: Helper function `getinfo_helper_current_consensus` add(changes/ticket31684): track changes within ticket31684 fix(changes/ticket31684): formatting and ticket#number refactor: remove redundant conditionals fix: getinfo_helper_current_consensus returns -1 for any question that doesn't ask for microdesc or ns descriptors lint: wide condition

asn-d6

OK here is a review. Also please change the commit message to specify what the changes did, instead of which trac ticket they fixed. Example "Add GETINFO support for dumping microdesc consensus".

changes/ticket31684

src/feature/control/control_getinfo.c

src/test/test_controller.c

src/feature/control/control_getinfo.c

src/test/test_controller.c

src/feature/control/control_getinfo.c

changes/ticket31684

src/feature/control/control_getinfo.h

src/feature/control/control_getinfo.c

teor2345 · 2019-10-04T01:18:22Z

It seems like you're pushing a lot of commits here.
Can we help you get set up to run these tests locally?

Or maybe this task isn't a good fit for you.
Would you like us to take over this refactor?

ltbringer · 2019-10-04T07:15:24Z

I'll take it off on a separate branch and use the CI on my fork. I assumed I'd get this done faster but I should have done this before. Sincere apologies.

changes/ticket31684

src/feature/control/control_getinfo.c

src/test/test_controller.c

test: check if GETINFO commands return expected consensus data.. docs: function docs easier to understand update: Code simplification and refactoring change

src/feature/control/control_getinfo.c

…nd type of getinfo_helper_current_consensus accepts flavor as a type(enum) consensus_flavor_t instead of plain int lint: removed unnecessary line

teor2345

Fixes made by me in #1434

No action required.

teor2345 · 2019-10-21T03:19:01Z

src/feature/nodelist/networkstatus.h

+          (int flav,
+           const char *flavorname,
+           int unverified_consensus));
+tor_mmap_t * networkstatus_map_cached_consensus(const char *flavorname);


Extra space

teor2345 · 2019-10-21T03:19:20Z

src/feature/control/control_getinfo.c

+{
+  const char *flavor_name = networkstatus_get_flavor_name(flavor);
+  if (!strcmp(flavor_name, "??")) {
+    *errmsg = "Could not open cached consensus. "


Wrong error message

teor2345 · 2019-10-21T03:19:38Z

src/feature/control/control_getinfo.c

        return -1;
      }
-    }


Weird spacing?

teor2345 · 2019-10-21T03:19:46Z

src/feature/control/control_getinfo.c

@@ -1513,6 +1544,8 @@ static const getinfo_item_t getinfo_items[] = {
         "v2 networkstatus docs as retrieved from a DirPort."),
  ITEM("dir/status-vote/current/consensus", dir,
       "v3 Networkstatus consensus as retrieved from a DirPort."),
+  ITEM("dir/status-vote/current/consensus-microdesc", dir,
+       "v3 Microdescriptors consensus as retrieved from a DirPort."),


teor2345 · 2019-10-21T03:20:11Z

src/feature/dircache/dirserv.c

-dirserv_get_consensus(const char *flavor_name)
+MOCK_IMPL(cached_dir_t *,
+          dirserv_get_consensus,
+          (const char *flavor_name))


teor2345 · 2019-10-21T03:38:37Z

src/feature/control/control_getinfo.c

+  } else if (!strcmp(question, "dir/status-vote/current/consensus")) {
+    int consensus_result = getinfo_helper_current_consensus(FLAV_NS,
+                                                            answer, errmsg);
+    if (consensus_result == -1) {


We use < 0

teor2345 · 2019-10-21T04:22:56Z

src/test/test_controller.c

+  const char *errmsg = NULL;
+
+  (void)arg;
+  setup_bridge_mocks();


teor2345 · 2019-10-21T04:23:16Z

src/test/test_controller.c

+  tor_free(mock_microdesc_consensus_cache->dir);
+  tor_free(answer);
+  errmsg = NULL;
+  options->FetchUselessDescriptors = previous_fetch_value;


Should be after done:

teor2345 · 2019-10-21T04:23:42Z

src/test/test_controller.c

+  (void)arg;
+  setup_bridge_mocks();
+  or_options_t *options = get_options_mutable();
+  int previous_fetch_value = options->FetchUselessDescriptors;


teor2345 · 2019-10-21T04:23:51Z

src/test/test_controller.c

+  options->FetchUselessDescriptors = previous_fetch_value;
+
+ done:
+  clear_bridge_mocks();


teor2345 · 2019-10-21T04:28:41Z

Latest code in #1434.

ltbringer force-pushed the Ticket31684 branch 5 times, most recently from 683cb91 to eca91ff Compare September 23, 2019 21:20

ltbringer force-pushed the Ticket31684 branch from eca91ff to d41c4f1 Compare September 23, 2019 22:01

asn-d6 requested changes Sep 26, 2019

View reviewed changes

asn-d6 reviewed Sep 26, 2019

View reviewed changes

src/feature/control/control_getinfo.c Show resolved Hide resolved

ltbringer requested a review from asn-d6 September 26, 2019 16:55

asn-d6 reviewed Sep 27, 2019

View reviewed changes

src/feature/control/control_getinfo.c Outdated Show resolved Hide resolved

asn-d6 reviewed Sep 27, 2019

View reviewed changes

src/test/test_controller.c Show resolved Hide resolved

ltbringer force-pushed the Ticket31684 branch 8 times, most recently from 3d18911 to e8a060d Compare September 28, 2019 21:04

ltbringer requested a review from asn-d6 September 28, 2019 22:06

asn-d6 reviewed Sep 30, 2019

View reviewed changes

src/feature/control/control_getinfo.c Outdated Show resolved Hide resolved

asn-d6 reviewed Sep 30, 2019

View reviewed changes

src/feature/control/control_getinfo.c Outdated Show resolved Hide resolved

asn-d6 reviewed Sep 30, 2019

View reviewed changes

src/feature/control/control_getinfo.c Outdated Show resolved Hide resolved

asn-d6 reviewed Sep 30, 2019

View reviewed changes

src/test/test_controller.c Show resolved Hide resolved

asn-d6 reviewed Sep 30, 2019

View reviewed changes

src/feature/control/control_getinfo.c Outdated Show resolved Hide resolved

ltbringer force-pushed the Ticket31684 branch 3 times, most recently from 8f3a3f1 to e546a6a Compare September 30, 2019 18:25

teor2345 reviewed Oct 2, 2019

View reviewed changes

src/feature/control/control_getinfo.c Outdated Show resolved Hide resolved

teor2345 reviewed Oct 2, 2019

View reviewed changes

changes/ticket31684 Outdated Show resolved Hide resolved

teor2345 reviewed Oct 2, 2019

View reviewed changes

src/feature/control/control_getinfo.h Outdated Show resolved Hide resolved

teor2345 reviewed Oct 2, 2019

View reviewed changes

src/feature/control/control_getinfo.c Outdated Show resolved Hide resolved

teor2345 reviewed Oct 2, 2019

View reviewed changes

src/feature/control/control_getinfo.c Outdated Show resolved Hide resolved

teor2345 reviewed Oct 2, 2019

View reviewed changes

src/feature/control/control_getinfo.c Show resolved Hide resolved

ltbringer force-pushed the Ticket31684 branch 7 times, most recently from 42149a2 to 5b34a4d Compare October 3, 2019 22:42

ltbringer force-pushed the Ticket31684 branch from 856f4b2 to 18100e4 Compare October 6, 2019 12:28

ltbringer requested a review from asn-d6 October 6, 2019 12:59

asn-d6 requested changes Oct 7, 2019

View reviewed changes

ltbringer force-pushed the Ticket31684 branch 2 times, most recently from a4ebfbc to 933ea85 Compare October 9, 2019 18:18

ltbringer requested a review from asn-d6 October 10, 2019 05:48

Add: GETINFO support for dumping microdesc consensus

f91a826

test: check if GETINFO commands return expected consensus data.. docs: function docs easier to understand update: Code simplification and refactoring change

ltbringer force-pushed the Ticket31684 branch from 933ea85 to f91a826 Compare October 10, 2019 16:59

asn-d6 reviewed Oct 10, 2019

View reviewed changes

src/feature/control/control_getinfo.c Outdated Show resolved Hide resolved

asn-d6 reviewed Oct 10, 2019

View reviewed changes

src/feature/control/control_getinfo.c Outdated Show resolved Hide resolved

fix: check for '??' being returned by networkstatus_get_flavor_name a…

b6b38b8

…nd type of getinfo_helper_current_consensus accepts flavor as a type(enum) consensus_flavor_t instead of plain int lint: removed unnecessary line

ltbringer force-pushed the Ticket31684 branch from 8234941 to b6b38b8 Compare October 20, 2019 12:41

teor2345 reviewed Oct 21, 2019

View reviewed changes

teor2345 closed this Oct 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ticket31684 #1328

Ticket31684 #1328

ltbringer commented Sep 16, 2019

coveralls commented Sep 16, 2019 •

edited

asn-d6 left a comment

teor2345 commented Oct 4, 2019 •

edited

ltbringer commented Oct 4, 2019

teor2345 left a comment

teor2345 Oct 21, 2019

teor2345 Oct 21, 2019

teor2345 Oct 21, 2019

teor2345 Oct 21, 2019

teor2345 Oct 21, 2019

teor2345 Oct 21, 2019

teor2345 Oct 21, 2019

teor2345 Oct 21, 2019

teor2345 Oct 21, 2019

teor2345 Oct 21, 2019

teor2345 commented Oct 21, 2019

Ticket31684 #1328

Ticket31684 #1328

Conversation

ltbringer commented Sep 16, 2019

coveralls commented Sep 16, 2019 • edited

Pull Request Test Coverage Report for Build 6594

💛 - Coveralls

asn-d6 left a comment

Choose a reason for hiding this comment

teor2345 commented Oct 4, 2019 • edited

ltbringer commented Oct 4, 2019

teor2345 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teor2345 commented Oct 21, 2019

coveralls commented Sep 16, 2019 •

edited

teor2345 commented Oct 4, 2019 •

edited