Opened 9 years ago

Closed 3 years ago

Last modified 3 years ago

#961 closed enhancement (implemented)

TotalTraffic option (upload+download)

Reported by: Athaba Owned by:
Priority: Low Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version: 0.2.0.31
Severity: Keywords: accounting easy tor-relay
Cc: Athaba, nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by nickm)

Could a TotalTraffic option similar to AccountingMax be added?
I know, it isn't to hard to divide by two, but you usually need
to limit the sum of upload and download to not exceed your
traffic limit.

Maybe the description of AccountingMax should also be changed to
make it clear, if one, either upstream traffic or downstream
traffic exceeds the limit Tor will hibernate.

[Automatically added by flyspray2trac: Operating System: All]

Child Tickets

Attachments (3)

fix_961.patch (3.1 KB) - added by chobe 3 years ago.
fix_961_v2.patch (8.7 KB) - added by chobe 3 years ago.
fix_961_v3.patch (14.1 KB) - added by chobe 3 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 9 years ago by nickm

See also discussion on bug 871.

comment:2 Changed 7 years ago by nickm

Description: modified (diff)
Keywords: accounting easy added
Milestone: post 0.2.1.xTor: unspecified

I'd personally favor an interface like "AccountingRule sum" or "AccountingRule max", to say whether the accountingmax applies to sum(r,w), or max(r,w). Moving this to the 'unspecified' benchmark; it's a fine thing to merge if somebody wants to implement it.

comment:3 Changed 5 years ago by nickm

Keywords: tor-relay added

comment:4 Changed 5 years ago by nickm

Component: Tor RelayTor

Changed 3 years ago by chobe

Attachment: fix_961.patch added

comment:5 Changed 3 years ago by chobe

Cc: Athaba,nickmAthaba, nickm

I implemented the AccountingRule idea. It should have a unit test. I'll work on learning how to add tests. I tested the code manually by printf'ing the in/out bytes and catching when it shuts down with both options (max and sum).

comment:6 Changed 3 years ago by chobe

Status: newneeds_review

comment:7 Changed 3 years ago by nickm

Milestone: Tor: unspecifiedTor: 0.2.6.x-final

comment:8 Changed 3 years ago by nickm

Status: needs_reviewneeds_revision

Thanks for the patch! Some quick comments:

  • Instead of doing log_info(LD_GENERAL, "Weird AccountingRule given, defaulting to 'max'"); inside check_reached_limit(), we should probably check that AccountingRule is either "max" or "sum" inside options_validate in config.c.
  • We should document AccountingRule in doc/tor.1.txt
  • The type of the "limit" needs to be uint64_t, which can be larger than 'int'
  • I think the calculation in update_expected_bandwidth_usage might need to be different for this case too, perhaps.

Changed 3 years ago by chobe

Attachment: fix_961_v2.patch added

comment:9 Changed 3 years ago by chobe

Thanks for reviewing my patch.

I fixed those issues plus another calculation was done in hibernate_begin.

Also I modified getinfo_helper_accounting to return a more accurate bytes-left when queried. I left it in the same format so that automated programs like tor-arm don't break, but I'm not entirely sure if it makes things too confusing. I'd need to look into tor-arm to make a better solution.

Not sure why giving a uint64_t to a function that only accepts an int doesn't give an warning.

I attempted to write a test, but I need to learn more about auto-tools to make files link properly.

Last edited 3 years ago by chobe (previous) (diff)

comment:10 Changed 3 years ago by chobe

Status: needs_revisionneeds_review

comment:11 Changed 3 years ago by nickm

Status: needs_reviewneeds_revision

Thanks! A couple of quick notes:

  • We write "} else {" on one line, with spaces as noted. Running "make check-spaces" can tell you about this and other formatting problems.
  • At the end of "update_expected_bandwidth_usage", is it still correct to use "max_configured" as the upper limit for "expected"? Or should it be "2*max_configured" in the "sum" case? (I don't actually know.)

Changed 3 years ago by chobe

Attachment: fix_961_v3.patch added

comment:12 Changed 3 years ago by chobe

Status: needs_revisionneeds_review

I looked into various places where AccountingMax was used and modified them accordingly. I also wrote a unit test to test AccountingRule.

comment:13 Changed 3 years ago by nickm

Looks good to me. I've added a new branch "ticket961" in my public repository. (see https://gitweb.torproject.org/nickm/tor.git) If my changes look okay to you, I can merge.

(I made AccountingRule get passed to strcmp only once, and I fixed some compilation warnings.)

comment:14 Changed 3 years ago by nickm

Resolution: Noneimplemented
Status: needs_reviewclosed

Still looks okay to me. Squashing and merging!

comment:15 Changed 3 years ago by chobe

Wow thanks for helping me contribute to the project! Looks like the test I wrote is still working, and I don't see anything wrong with the strcmp removal you did. I recently got busy with school but I will try to contribute when I get free time.

Note: See TracTickets for help on using tickets.