Opened 10 months ago
Closed 8 months ago
#25960 closed enhancement (fixed)
Allow additional header lines in bandwidth measurements documents
Reported by: | juga | Owned by: | juga |
---|---|---|---|
Priority: | Medium | Milestone: | Tor: 0.3.5.x-final |
Component: | Core Tor/Tor | Version: | |
Severity: | Normal | Keywords: | 035-proposed, bwauth, 034-backport, 029-backport, 031-backport, 032-backport, 033-backport, tor-bwauth |
Cc: | teor | Actual Points: | |
Parent ID: | #25925 | Points: | |
Reviewer: | asn | Sponsor: |
Description
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
Child Tickets
Change History (19)
comment:1 Changed 10 months ago by
comment:2 Changed 10 months ago by
Keywords: | 029-backport 031-backport 032-backport 033-backport added |
---|---|
Milestone: | → Tor: 0.3.4.x-final |
Status: | assigned → needs_revision |
Type: | defect → enhancement |
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
- an incomplete line occurs after the first complete line, please test:
- 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
comment:3 Changed 10 months ago by
I think i implemented all the points in branch bug25960_maint-0.2.9_additional_headers (github, my fork). Ready for review again.
comment:4 Changed 10 months ago by
Status: | needs_revision → needs_review |
---|
comment:5 Changed 10 months ago by
Thanks, I left some comments on https://github.com/juga0/tor/tree/bug25960_maint-0.2.9_additional_headers
- A note about strict versus lax parsing. We don't need to change any code.
- 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:
- Mock read_file_to_string to return the test data
Here's another way:
- 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
- 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.)
comment:6 Changed 10 months ago by
Status: | needs_review → needs_revision |
---|
comment:7 follow-up: 8 Changed 10 months ago by
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.
comment:8 Changed 10 months ago by
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.
comment:9 Changed 10 months ago by
Keywords: | 035-proposed added |
---|---|
Milestone: | Tor: 0.3.4.x-final → Tor: 0.3.5.x-final |
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.
comment:10 Changed 10 months ago by
Keywords: | 034-backport added |
---|
comment:11 Changed 9 months ago by
If we decide 1.1.0, this ticket would be solved by https://trac.torproject.org/projects/tor/ticket/25947#comment:11
comment:13 Changed 9 months ago by
Status: | needs_revision → needs_review |
---|
comment:14 Changed 9 months ago by
Reviewer: | → asn |
---|
comment:15 Changed 9 months ago by
Status: | needs_review → needs_revision |
---|
Putting to needs_revision because of comments in #25947.
comment:16 Changed 9 months ago by
Status: | needs_revision → merge_ready |
---|
Change to merge_ready because of #25947.
Probably there is no need to backport it to versions earlier than 032.
comment:17 Changed 9 months ago by
Keywords: | tor-bwauth added |
---|
comment:18 Changed 8 months ago by
comment:19 Changed 8 months ago by
Resolution: | → fixed |
---|---|
Status: | merge_ready → closed |
https://github.com/juga0/tor/tree/bug25960_additional_headers