Opened 2 years ago

Last modified 15 months ago

#22717 assigned defect

Rename channelpadding.c's CHANNEL_IS_CLIENT to avoid confusion

Reported by: teor Owned by: mikeperry
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor: 0.3.1.1-alpha
Severity: Normal Keywords: technical-debt, disaster-waiting-to-happen, 031-deferrable, 033-triage-20180320, 033-removed-20180320
Cc: mikeperry Actual Points:
Parent ID: #24905 Points: 0.1
Reviewer: Sponsor:

Description

There is already a channel_is_client() function in channel.[ch], with different arguments and semantics.

And while we're doing that, we should make the newly renamed CHANNEL_IS_CLIENT() call channel.h's channel_is_client(), because it's the right abstraction to use.

We could survive releasing 0.3.1 without this fix, because it's not a bug (yet).

Child Tickets

Change History (9)

comment:1 Changed 2 years ago by nickm

Cc: mikeperry added

comment:2 Changed 2 years ago by nickm

Owner: set to mikeperry
Status: newassigned

comment:3 Changed 2 years ago by mikeperry

I ran into a bunch of cases where channel->is_client flag was not updated, was incorrect, or disagreed with the definition I needed (in cases like bridges). How are we on ensuring that flag always has the same meaning as the macro checks for?

comment:4 in reply to:  3 Changed 2 years ago by teor

Replying to mikeperry:

I ran into a bunch of cases where channel->is_client flag was not updated, was incorrect, or disagreed with the definition I needed (in cases like bridges).

Is this before or after the fix in #21406 in 0.3.1.1-alpha?
If there are any of these cases left, please list these cases in a different ticket in 0.3.1.

How are we on ensuring that flag always has the same meaning as the macro checks for?

The macro and the flag have a different meaning, so they should have different names.

The flag means:

/** True iff we have decided that the other end of this connection
   * is a client or bridge relay.  Connections with this flag set should never
   * be used to satisfy an EXTEND request. */

https://gitweb.torproject.org/tor.git/tree/src/or/channel.h#n302
We will clarify this by renaming the function in #22090.

The macro means:

/**
 * This macro tells us if either end of the channel is connected to a client.
 * (If we're not a server, we're definitely a client. If the channel thinks
 *  its a client, use that. Then finally verify in the consensus).
 */

https://gitweb.torproject.org/tor.git/tree/src/or/channelpadding.c#n63
Renaming the macro is what this ticket is for.

comment:5 Changed 20 months ago by nickm

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

comment:6 Changed 17 months ago by teor

Parent ID: #24905

comment:7 Changed 15 months ago by nickm

Keywords: 033-triage-20180320 added

Marking all tickets reached by current round of 033 triage.

comment:8 Changed 15 months ago by nickm

Keywords: 033-removed-20180320 added

Mark all not-already-included tickets as pending review for removal from 0.3.3 milestone.

comment:9 Changed 15 months ago by nickm

Milestone: Tor: 0.3.3.x-finalTor: unspecified

These tickets were marked as removed, and nobody has said that they can fix them. Let's remember to look at 033-removed-20180320 as we re-evaluate our triage process, to see whether we're triaging out unnecessarily, and to evaluate whether we're deferring anything unnecessarily. But for now, we can't do these: we need to fix the 033-must stuff now.

Note: See TracTickets for help on using tickets.