Opened 3 years ago

Last modified 18 months ago

#18757 needs_revision defect

Memunit config defaults say "KB" rather than "KBytes"

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: easy, 032-unreached
Cc: atagar Actual Points:
Parent ID: Points: 1
Reviewer: Sponsor:

Description

E.g.

  V(AuthDirFastGuarantee,        MEMUNIT,  "100 KB"),
  V(AuthDirGuardBWGuarantee,     MEMUNIT,  "2 MB"),

and others. These should say "100 KBytes", "2 MBytes", etc, in the same way that we've updated the documentation and sample configs and so on.

Child Tickets

Attachments (5)

first (4.9 KB) - added by neerajbattan 2 years ago.
second (54.4 KB) - added by neerajbattan 2 years ago.
third (54.4 KB) - added by neerajbattan 2 years ago.
updateFallbackDirs (1.8 KB) - added by neerajbattan 2 years ago.
non_updateFallbackDirs (34.8 KB) - added by neerajbattan 2 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 3 years ago by arma

Cc: atagar added

atagar, you're the only one I can think of that might be affected by this change to the defaults in config.c. Does Nyx care?

comment:2 Changed 3 years ago by atagar

Thanks for checking! Nope, Nyx should be happy. The current git repository doesn't have anything that cares about these units. The last release (arm 1.4.5) does, but it recognizes both.

On a personal note I like 'KB' more than 'KBytes' (former is a common unit you see everywhere, and the later is weird). If we want to standardize I'd go toward the former. But no concerns with breakage on my side.

comment:3 Changed 3 years ago by arma

This one's easy! Anybody who wants to get into Tor development and pick something where most of the work will be getting your git working, this is your ticket!

comment:4 Changed 2 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

Changed 2 years ago by neerajbattan

Attachment: first added

comment:5 Changed 2 years ago by neerajbattan

Hi, I've edited the corresponding file, please have a look at it.

comment:6 Changed 2 years ago by nickm

Milestone: Tor: 0.3.???Tor: 0.3.0.x-final
Status: newneeds_review

comment:7 Changed 2 years ago by cypherpunks

The units in the manual should also be updated.

Changed 2 years ago by neerajbattan

Attachment: second added

comment:8 Changed 2 years ago by neerajbattan

I changed the corresponding files. But I didn't change KB to KBytes at some places because of language, like '"kilobytes" or "KB", that "Kbits" may also be written as'. Please let me know if those lines are needed to be changed.

Changed 2 years ago by neerajbattan

Attachment: third added

comment:9 Changed 2 years ago by neerajbattan

There was a minor mistake of "1.01 GByte" in second.

comment:10 Changed 2 years ago by teor

Status: needs_reviewneeds_revision

Thanks for the patch!

I'm not sure if we should change the ChangeLog (arma and nickm might have an opinion about this).

I think the manual changes are fine.

The changes to the updateFallbackDirs.py should be ok, but they need to be put into a separate patch, so they can be included when the script is run over the next few days. This will make the change to fallback_dirs.inc unnecessary - we're going to replace it very soon.

I think we have a special process for staging changes to the torrc files. nickm knows more about this than I do.

Please don't delete the final empty line from src/config/torrc.sample.in

"(byte, KBytes, M, etc)" should be "(byte, KBytes, MBytes, etc)"

Would you mind changing "We just generated an extra-info descriptors that" to "We just generated an extra-info descriptor that" while you're there?

You might want to run make check-spaces to ensure that the line lengths are still ok.

Since you have changed log messages, please run make check to ensure all the unit tests still pass.

comment:11 Changed 2 years ago by nickm

Points: 1

I don't like changing the changelogs for previously released versions of tor.

comment:12 Changed 2 years ago by nickm

Milestone: Tor: 0.3.0.x-finalTor: 0.3.1.x-final

Changed 2 years ago by neerajbattan

Attachment: updateFallbackDirs added

Changed 2 years ago by neerajbattan

Attachment: non_updateFallbackDirs added

comment:13 Changed 2 years ago by neerajbattan

I have added the removed extra line from src/config/torrc.sample.in, created a different patch for updateFallbackDirs.py and changed "We just generated an extra-info descriptors that" to "We just generated an extra-info descriptor that". Please let me know if still some changes are needed.

comment:14 Changed 2 years ago by nickm

Status: needs_revisionneeds_review

comment:15 Changed 2 years ago by nickm

Status: needs_reviewneeds_revision

We should not change the changelog or release notes for old versions of Tor.

comment:16 Changed 23 months ago by arma

I'm excited to get these fixes in! I am ok with having them in either 0.3.1 or 0.3.2.

neerajbattan, maybe you will find this a more fun experience if you attend one of the patch parties?

They are once a week on IRC:
https://trac.torproject.org/projects/tor/wiki/org/teams/NetworkTeam

I agree with Nick that changing the ChangeLog retroactively for this isn't a smart move (since people rely on the ChangeLog to be a record of what we said we did in the past).

teor, did we get all the changes we need for the fallback directory stuff, or does some of that still need to be resolved in this ticket?

comment:17 Changed 22 months ago by nickm

Milestone: Tor: 0.3.1.x-finalTor: 0.3.2.x-final

comment:18 Changed 18 months ago by nickm

Keywords: 032-unreached added
Milestone: Tor: 0.3.2.x-finalTor: unspecified

Mark a large number of tickets that I do not think we will do for 0.3.2.

Note: See TracTickets for help on using tickets.