Opened 8 months ago

Closed 7 months ago

#25081 closed defect (fixed)

use get_uptime() consistently

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: easy
Cc: aegis Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We have this nice function get_uptime() that shields the global variable stats_n_seconds_working from the rest of the Tor files.

But then we undermine that by saying

extern long stats_n_seconds_working;

in main.h and we just start using that variable directly all over.

There are a few lonely users of get_uptime().

We should move everybody over to using get_uptime, and get rid of the scary extern.

(Also check out how we're *writing* to the extern variable, in hibernate.c. There's no way that could confuse anybody down the road!)

Child Tickets

Change History (9)

comment:1 Changed 8 months ago by valentecaio

It's my first time contributing to Tor Project, but I think I've done this ticket.

I did a small refactor:
1- stats_n_seconds_working is not extern anymore. Because of this, all direct accesses to it were changed to get_uptime() or reset_uptime() calls.
2- reset_uptime() sets stats_n_seconds_working to zero.

Please take a look at my patch here
https://github.com/valentecaio/torproject-tor/commit/a4c85312604c1d215f9f46a24ac242baf666ed56

Last edited 8 months ago by valentecaio (previous) (diff)

comment:2 Changed 8 months ago by valentecaio

Status: newneeds_revision

comment:3 Changed 8 months ago by teor

Status: needs_revisionneeds_review

This should get reviewed in the next week or two

comment:4 Changed 8 months ago by valentecaio

I've just added the changes file for this ticket, in the same branch:

Last commit, with changes file: https://github.com/valentecaio/torproject-tor/commit/ad06e0b406f5b3dfaa28da0f03ede4b0f850d00f

I'm sorry for sending it in a new commit, I'll add the changes in the same commit for next patches.

comment:5 Changed 8 months ago by teor

That's fine, extra commits aren't a big deal.
Did you run "make check" on your patch?

comment:6 Changed 8 months ago by valentecaio

Yes. Actually, I ran a "make test-full" and it seems that everything is ok.

comment:7 Changed 8 months ago by nickm

Status: needs_reviewmerge_ready

Patch lgtm. Thanks for writing it! Let's merge this once the 0.3.4.x merge window is open (should be next week.)

comment:8 Changed 8 months ago by nickm

(when we merge it, we'll need to make "stats_n_seconds_working" be declared static in src/or/main.c.)

comment:9 Changed 7 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged, with the fix mentioned above. Thanks again!

Note: See TracTickets for help on using tickets.