Allow additional header lines in bandwidth measurements documents

[juga]

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

[juga]

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

[teor]

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

[juga]

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

[teor]

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.)

[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.

[teor]

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.

[teor]

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.

[juga]

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

[juga]

ticket25947_032_03_refactor should fix this ticket.

[asn]

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

[asn]

Putting to needs_revision because of comments in #25947.

[juga]

Change to merge_ready because of #25947.

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

[nickm]

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

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