#25291 closed enhancement (implemented)

get rid of redundant should_record_bridge_info() call in options_act()

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 034-triage-20180328
Cc: neel@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In options_act() we have

    if ((!old_options || !old_options->EntryStatistics) &&
        options->EntryStatistics && !should_record_bridge_info(options)) {

But right above that we have

    /* Only collect other relay-only statistics on relays. */
    if (!public_server_mode(options)) {
      options->CellStatistics = 0;
      options->EntryStatistics = 0;

So the only way EntryStatistics could be non-zero at that lower point is if public_server_mode() is true.

So the check to !should_record_bridge_info() will always be true, and is redundant to check.

Child Tickets

Attachments (1)

b25291-p1.patch (1.3 KB) - added by neel 19 months ago.
[PATCH] Remove redundant should_record_bridge_info() call in options_act() (Revision 1)

Download all attachments as: .zip

Change History (10)

Changed 19 months ago by neel

Attachment: b25291-p1.patch added

[PATCH] Remove redundant should_record_bridge_info() call in options_act() (Revision 1)

comment:1 Changed 19 months ago by neel

I have a patch which (hopefully) solves this. The filename is b25291-p1.patch.

comment:2 Changed 19 months ago by arma

Status: newneeds_review

comment:3 Changed 19 months ago by teor

Status: needs_reviewneeds_revision

comment:4 Changed 19 months ago by neel

Cc: neel@… added

I noticed a needs_revision flag on this bug report. What am I missing in my patch? I can modify the code to include what's needed.

comment:5 Changed 19 months ago by teor

Status: needs_revisionmerge_ready

Oops, seems fine to me.

comment:6 Changed 19 months ago by nickm

Neel: this patch looks correct to me too. But...

Roger, I am actually thinking that maybe we should not make this change. In my opinion, checking should_record_bridge_info() is a good idea, in case we ever want to change its semantics, or earlier block of the function, in the future. What do you think?

comment:7 in reply to:  6 Changed 19 months ago by arma

Replying to nickm:

Neel: this patch looks correct to me too. But...

Roger, I am actually thinking that maybe we should not make this change. In my opinion, checking should_record_bridge_info() is a good idea, in case we ever want to change its semantics, or earlier block of the function, in the future. What do you think?

I could see this going either way. Do whichever one you think results more in readable and maintainable code, I say.

If we leave the redundant call in place, maybe we add a comment saying that we know (as of right now) it's redundant, but we're leaving it in defensively?

comment:8 Changed 19 months ago by nickm

Keywords: 034-triage-20180328 added

comment:9 Changed 18 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Added such a comment in e58555135a11e1

Note: See TracTickets for help on using tickets.