Create unit test for dirserv_read_measured_bandwidths

So that we ensure that tor will still continue to read V3BandwidthsFile documents when we add new headers to it in version 1.1.0, as proposed in legacy/trac#25869 (moved)

changed milestone to %Tor: 0.3.4.x-final in legacy/trac

added bwauth in Legacy / Trac component::core tor/tor in Legacy / Trac milestone::Tor: 0.3.4.x-final in Legacy / Trac owner::juga in Legacy / Trac parent::25925 in Legacy / Trac priority::medium in Legacy / Trac resolution::implemented in Legacy / Trac reviewer::asn in Legacy / Trac severity::normal in Legacy / Trac status::closed in Legacy / Trac test in Legacy / Trac tor-bwauth in Legacy / Trac type::enhancement in Legacy / Trac labels

WIP in https://github.com/juga0/tor/tree/test_dirserv_read_mbws

This test is waiting on spec updates in: https://github.com/torproject/torspec/pull/4

Let us know when it's ready for review.

Trac:
Milestone: N/A to Tor: 0.3.4.x-final
Keywords: N/A deleted, test added

Trac:
Parent: N/A to legacy/trac#25925 (moved)

Added additional lines to test in https://github.com/torproject/tor/pull/77 Header is still not being tested.

To test the header, would it be possible to refactor the code as proposed in legacy/trac#25960 (moved), or there should not be any change to the code here?

You can read my response your question at: https://github.com/torproject/tor/pull/78#issuecomment-386196368

Here's how we can change the code safely:

• make a commit with unit tests for the relay lines
• make a commit that refactors the code so you can test a whole file
• make a commit that unit tests a whole torflow file
• make changes to accept the new headers
• make a commit that unit tests a whole sbws file

make a commit with unit tests for the relay lines

That's https://github.com/torproject/tor/pull/77, i'll assume i should wait for the rest of changes to review

And https://github.com/torproject/tor/pull/87, same but based on maint-0.2.9

(Should this be in needs_review? Assuming so.)

Trac:
Status: assigned to needs_review

Replying to juga:

make a commit with unit tests for the relay lines

That's https://github.com/torproject/tor/pull/77, i'll assume i should wait for the rest of changes to review

We're waiting on the final spec.

Trac:
Status: needs_review to needs_information

We're waiting on the final spec.

Even if we go for 2.0.0 (without passing by 1.1.0 first), the old format should still be backported, and it does not matter whether the additional header lines are KeyValue or use SP

legacy/trac69717 a commit with unit tests for the relay lines

This is already done by test_dir_measured_bw_kb. For the test where node_id failed to be at the end, it's done in legacy/trac#26004 (moved), commit dbc80ad1

legacy/trac69717 a commit that refactors the code so you can test a whole file

After nickm pointed how to create temporal files for tests in https://trac.torproject.org/projects/tor/ticket/26007#comment:11, this is not longer needed

legacy/trac69717 a commit that unit tests a whole torflow file

commit in branch ticket25947_029_squashed https://github.com/juga0/tor/commit/ea0e12ef31c157d43c949dda1469b43704836322

legacy/trac69717 changes to accept the new headers

This was technically legacy/trac#25960 (moved).

commit in branch ticket25947_029_squashed https://github.com/juga0/tor/commit/363cbb4196da91d63c3ff93319911636611018dc

legacy/trac69717 a commit that unit tests a whole sbws file

commit in branch ticket25947_029_squashed https://github.com/juga0/tor/commit/789f794f9fcbc37870e55de3364b3e1dee7dc22a

Trac:
Status: needs_information to needs_review

Trac:
Reviewer: N/A to asn

Second round of review (first one was over IRC):

• There is at least one more memleak in the unittests. Example:

+  tor_asprintf(&content, "%ld\n%s%s", timestamp, v110_header_lines,
+               sbws_relay_lines);
+  write_str_to_file(fname, content, 0);
+  log_debug(LD_DIRSERV, "Bandwidth list file with additional headers and "
+            "sbws RelayLine.");
+  tt_int_op(0, OP_EQ, dirserv_read_measured_bandwidths(fname, NULL));
+
+ done:
+  teardown_capture_of_logs();
+}

Please use ASAN or valgrind to ensure that all memleaks are fixed. Check out doc/HACKING/HelpfulTools.md on how to use valgrind.

• Commit 2e8c0b9054 adds header_ended argument to measured_bw_line_parse() but does not document what it does in the function doc. Also the name header_ended is a bit weird, what does it mean?

• It's also not entirely clear what header_ended is supposed to do in dirserv_read_measured_bandwidths(). Why do we change it to 1? Perhaps a comment would be helpful.

• There is trailing whitespace around the new code. Please use make check-spaces to fix that. It must be clean before merging.

• I think the changelog entries are too detailed and also too vague at the same time. Example of vagueness:

+  o Minor feature (unit tests):
+    - Test that incomplete lines only give warnings when the end of the header
+      has not been detected

Incomplete lines of what? There is no mention of bwauths in the changes entry.

IMO, we should just add a single entry under Minor feature (unit tests): that says Improve bandwidth measurement unittests and that should be enough. Anyone who cares about the specific unittests, can just look at the commit themselves.

Trac:
Status: needs_review to needs_revision

Solved those isues in ticket25947_032_03_refactor branch.

Trac:
Status: needs_revision to needs_review

Pushing to 035 along with legacy/trac#25960 (moved).

Trac:
Milestone: Tor: 0.3.4.x-final to Tor: 0.3.5.x-final

Hello,

I pushed a branch ticket25947_032_03_refactor with a few changes: a) Renamed end_header variable to line_is_after_headers to follow the boolean variable convention of tor. Also clarified a few comments. b) Fixed some trailing whitespace and double lines that were making make check-spaces complain. c) Slightly edited the changes file to follow usual convention.

Juga, if you like the changes, please squash them into your branch (so that in the end you only have 4 commits) and let's turn this to merge_ready.

Also, please test this branch against torflow and swbws to make sure sbws to make sure that it functions properly. Let me know if you can't do this and I will do it if needed.

Trac:
Status: needs_review to needs_revision

I agree with the changes and have squashed them in my ticket25947_032_03_refactor branch.

I've run the branch using torflow and sbws generated bandwidth file, and there was no any warnings (grep -r warn /pathtotorlogs/*.log|grep -i bandwidth).

Probably this should be backported to 032-backport, as legacy/trac#25960 (moved)

Trac:
Status: needs_revision to merge_ready

Trac:
Keywords: N/A deleted, tor-bwauth added

Merged to 0.3.4 and forward; marking for possible backport.

Trac:
Milestone: Tor: 0.3.5.x-final to Tor: 0.3.3.x-final

No backport on this one; it's test only.

Trac:
Milestone: Tor: 0.3.3.x-final to Tor: 0.3.4.x-final
Resolution: N/A to implemented
Status: merge_ready to closed

closed

mentioned in issue legacy/trac#25960 (moved)

moved from legacy/trac#25947 (moved)

added Feature label and removed 1 deleted label

added BwAuth Tests labels and removed 1 deleted label

mentioned in issue tpo/network-health/sbws#25925 (closed)