Allow additional header lines in bandwidth measurements documents

As commented by teor in: https://github.com/pastly/simple-bw-scanner/pull/112#issuecomment-385108911

get it backported to 0.2.9 and later. I think Tor should only warn if: an incomplete line contains a "bw=" key an incomplete line contains a "node_id=" key an incomplete line occurs after the first complete line

changed milestone to %Tor: 0.3.5.x-final

added 029-backport 031-backport 032-backport 033-backport 034-backport 035-proposed bwauth component::core tor/tor milestone::Tor: 0.3.5.x-final owner::juga parent::25925 priority::medium resolution::fixed reviewer::asn severity::normal status::closed tor-bwauth type::enhancement labels

https://github.com/juga0/tor/tree/bug25960_additional_headers

Thanks for this patch!

Here is my review:

• please base this commit on maint-0.2.9, so we can backport it
• Travis CI seems to have failed, is it set up on your account? Please check #tor-ci on IRC
• please implement and add unit tests for the two header_ended=0 cases:
  • an incomplete line contains a "bw=" key
  • an incomplete line contains a "node_id=" key
• please add a unit test for the header_ended=1 case:
  • an incomplete line occurs after the first complete line, please test:
    • a line missing node_id
    • a line missing bw
    • a line missing node_id and bw
• this log message is confusing, because it contains a double negative, "no" and "nor":

+    log_debug(LD_DIRSERV, "No bw nor node_id found in bandwidth file line: %s",
+             escaped(orig_line));

• Please use something like:

+    log_debug(LD_DIRSERV, "Missing bw or node_id in bandwidth file line: %s",
+             escaped(orig_line));

Edit: indenting Edit: we sorted out CI

Trac:
Keywords: N/A deleted, 031-backport, 029-backport, 032-backport, 033-backport added
Type: defect to enhancement
Status: assigned to needs_revision
Milestone: N/A to Tor: 0.3.4.x-final

I think i implemented all the points in branch bug25960_maint-0.2.9_additional_headers (github, my fork). Ready for review again.

Trac:
Status: needs_revision to needs_review

Thanks, I left some comments on https://github.com/juga0/tor/tree/bug25960_maint-0.2.9_additional_headers

1. A note about strict versus lax parsing. We don't need to change any code.
2. Please add unit tests where header lines fail to parse.

It would also be great if we had tests for entire version 1.0.0 and version 1.1.0 files, so we know we haven't broken parsing.

Here's one way to code these tests, if the existing function takes a filename:

1. Mock read_file_to_string to return the test data

Here's another way:

1. Refactor the existing tor code so it reads the file to a string in one function A, then passes the string to another function B
2. Call function B with the test data

Let me know if you need help with this code.

Please add extra commits for each change, and don't squash until we're ready to merge, (When we squash, we keep commits that do different things separate.)

Trac:
Status: needs_review to needs_revision

Thanks for the review.

Mock read_file_to_string to return the test data

it does not use read_file_to_str, so i can't mock that one, it uses tor_fopen_cloexec and fgets.

Refactor the existing tor code so it reads the file to a string in one function A

And A could actually use read_file_to_str

Now i should also subtitute fgets, and instead change the loop read 512 bytes from the previous str.

Replying to juga:

Thanks for the review.

Mock read_file_to_string to return the test data

it does not use read_file_to_str, so i can't mock that one, it uses tor_fopen_cloexec and fgets.

Refactor the existing tor code so it reads the file to a string in one function A

And A could actually use read_file_to_str

Now i should also subtitute fgets, and instead change the loop read 512 bytes from the previous str.

Hmm, using read_file_to_str() will change the error behaviour, so we'll need to be careful.

Next week is the last week of 0.3.4 features, and this feature is still being discussed on tor-dev@. https://lists.torproject.org/pipermail/tor-dev/2018-May/013154.html

We should decide whether to put the small or the large version of this ticket on the roadmap. The small change (format 1.1.0) adds backwards-compatible headers and silences warnings. The large change (format 2.0.0) turns the bandwidth file into a directory document-style format, and rewrites the code using the directory parsing functions.

Here are the things we need to do for the large change:

• The new format should have a new torrc option.
• Tor should be modified to support the new format, and we should put time on the roadmap for people to work on implementing, testing, or reviewing it.
• Either we should backport the new format to the latest stable release, or sbws should produce both formats.

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

Trac:
Keywords: N/A deleted, 034-backport added

If we decide 1.1.0, this ticket would be solved by https://trac.torproject.org/projects/tor/ticket/25947#comment:11

ticket25947_032_03_refactor should fix this ticket.

Trac:
Status: needs_revision to needs_review

This ticket is tracked as part of #25947 (moved). Adding myself as reviewer since I'm also reviewer of #25947 (moved).

Trac:
Reviewer: N/A to asn

Putting to needs_revision because of comments in #25947 (moved).

Trac:
Status: needs_review to needs_revision

Change to merge_ready because of #25947 (moved).

Probably there is no need to backport it to versions earlier than 032.

Trac:
Status: needs_revision to merge_ready

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

Since the fix here is included in #25947 (moved), we can close this ticket.

First, please either close or un-parent #8494 (moved) as appropriate?