Opened 3 years ago

Closed 3 years ago

#20684 closed defect (fixed)

DIRCACHE_MIN_MB_BANDWIDTH is actually used for RAM

Reported by: teor Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version: Tor: 0.2.8.1-alpha
Severity: Normal Keywords: easy intro typo
Cc: neel@… Actual Points:
Parent ID: Points: 0.1
Reviewer: Sponsor:

Description

We should change DIRCACHE_MIN_BANDWIDTH and DIRCACHE_MIN_MB_BANDWIDTH to end in *_MEM, as they are only ever used for memory, not bandwidth.

Introduced in commit 997f779 in tor-0.2.8.1-alpha.

Child Tickets

Attachments (2)

tor_patch_typo.patch (3.9 KB) - added by neel 3 years ago.
Patch to rename DIRCACHE_MIN_BANDWIDTH and DIRCACHE_MIN_MB_BANDWIDTH to DIRCACHE_MIN_BANDWIDTH_MEM and DIRCACHE_MIN_MB_BANDWIDTH_MEM
0001-rename-DIRCACHE_MIN_BANDWIDTH-and-DIRCACHE_MIN_MB_BA.patch (2.3 KB) - added by neel 3 years ago.
Updated patch to rename DIRCACHE_MIN_BANDWIDTH and DIRCACHE_MIN_MB_BANDWIDTH to DIRCACHE_MIN_MEM and DIRCACHE_MIN_MB_MEM

Download all attachments as: .zip

Change History (12)

Changed 3 years ago by neel

Attachment: tor_patch_typo.patch added

Patch to rename DIRCACHE_MIN_BANDWIDTH and DIRCACHE_MIN_MB_BANDWIDTH to DIRCACHE_MIN_BANDWIDTH_MEM and DIRCACHE_MIN_MB_BANDWIDTH_MEM

comment:1 Changed 3 years ago by neel

Hi,

I have created a patch to fix this. Please tell me what you think about this.

Thank You,
Neel Chauhan

comment:2 Changed 3 years ago by teor

Hi Neel, thanks for the patch.

Can you please rename them to:
DIRCACHE_MIN_MEM and DIRCACHE_MIN_MB_MEM
(There is no need to say "BANDWIDTH" at all, because it is confusing to say "BANDWIDTH" when talking about memory.)
I am sorry I did not explain better in the ticket.
Since these new names are shorter, there is no need to change the log messages to pass make check-spaces.

Also, would you like to do a changes file for this one?
Changes files help us make the changelog at the end of the release.
You can say "patch by Neel Chauhan" at the end if you want.
Here's how we do a changes file:
https://gitweb.torproject.org/tor.git/tree/doc/HACKING/CodingStandards.md#n56
Or you can look at the one I made on your last ticket for an example.

For the changes file, this bug was introduced in tor-0.2.8.1-alpha.

I used git blame to find out this change was made in commit 997f779, and git describe --contains 997f779 to find out the release for this commit was tor-0.2.8.1-alpha.

comment:3 Changed 3 years ago by teor

Status: newneeds_revision

comment:4 Changed 3 years ago by teor

Bug #20887 is another bug in this code.

Changed 3 years ago by neel

Updated patch to rename DIRCACHE_MIN_BANDWIDTH and DIRCACHE_MIN_MB_BANDWIDTH to DIRCACHE_MIN_MEM and DIRCACHE_MIN_MB_MEM

comment:5 Changed 3 years ago by neel

Cc: neel@… added

comment:6 Changed 3 years ago by nickm

Status: needs_revisionneeds_review

comment:7 Changed 3 years ago by teor

Status: needs_reviewneeds_revision

Since this fix changes the message output when the memory is too low, the following unit test fails:

options/mem_dircache: [forking] 
  FAIL src/test/test_options.c:236: assert(r OP_EQ -1): 0 vs -1
  [mem_dircache FAILED]

The best fix for this is to fix #20887, then change the message expected by the unit test.

comment:8 Changed 3 years ago by dgoulet

Owner: set to dgoulet
Status: needs_revisionassigned

Actually, the issue was that DIRCACHE_MIN_MEM was used instead of DIRCACHE_MIN_MB_MEM in the test. I've fixed it using the attached commit. Test passes now.

See branch bug20684_030_01

comment:9 Changed 3 years ago by dgoulet

Status: assignedmerge_ready

comment:10 Changed 3 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

I think the convention here is (or should be) to use a suffix that denotes the units of the constant. So I've added an extra commit (e0e729d4b52ef4a14e63e194ff0a47a0aa0d0f99) and merged this.

(We don't need to add a "BYTES" suffix to everything that is already in bytes.)

Note: See TracTickets for help on using tickets.