Opened 2 years ago

Closed 6 months ago

#20918 closed defect (wontfix)

Switch onion.c to use TRUNNEL_OPAQUE

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: technical-debt refactor opaque data-hiding trunnel
Cc: Actual Points: .2
Parent ID: #15054 Points: .1
Reviewer: dgoulet Sponsor:

Description

The TRUNNEL_OPAQUE macro stops trunnel from exposing object internals in its headers; we should use that in onion.c. (And possibly elsewhere.) Noted by dgoulet during code review.

Child Tickets

Change History (16)

comment:1 Changed 2 years ago by nickm

Owner: set to nickm
Status: newaccepted

comment:2 Changed 2 years ago by nickm

This will need us to bump to trunnel 1.5.1

comment:3 Changed 2 years ago by nickm

Parent ID: #15056#15054

comment:4 Changed 2 years ago by nickm

Bumped to trunnel 1.5.1 in bc68eedd79286420f8dcab5fa4ed83401299c89c. Now I'm going to see about making all our trunnel usage opaque.

comment:5 Changed 2 years ago by nickm

Actual Points: .1
Cc: dgoulet added
Status: acceptedneeds_review

So, in my ticket20918 branch, I tried turning on trunnel_opaque globally. What do you think? Is this actually cleaner?

comment:6 Changed 2 years ago by dgoulet

Cc: dgoulet removed
Reviewer: dgoulet
Status: needs_reviewmerge_ready

It is very much so cleaner yes. lgtm;

comment:7 Changed 2 years ago by nickm

I'd like a second opinion, actually. To me the original is a bit more readable, but my preference is only slight, and I could believe that most people find it more readable the other way.

comment:8 Changed 2 years ago by nickm

Actual Points: .1.2
Points: .1

comment:9 Changed 2 years ago by asn

Took a quick look here after Nick's request on IRC. I actually don't have a strong opinion and I'm fine both ways. I don't feel the change is that grave.

Negatives: I do feel like the ticket20918 code is more messy than before. That's mainly because some getters forced us to split code into multiple steps, and also some function names are lengthy so code had to wrap to fit into 80 columns. It's not terribly messy, but I imagine that it might be less readable to someone not familiar with trunnel. Also, if opaqueness is on by default, it makes it slightly harder to write trunnel code correctly. Also also, if this is a new coding pattern, do we need to change all the other places that use trunnel to actually solely use getters instead of direct struct access?

Positives: All in all, I guess making structs opaque is a positive step wrt encapsulation, information-hiding yada yada, so I can see why it would a good idea in principle to do this change.

Personally, I'm OK with merging this patch. I'm also fine with postponing this patch until we have a more complete plan on how to properly do info-hiding over our whole codebase (and not just trunnel).

comment:10 Changed 2 years ago by nickm

Also also, if this is a new coding pattern, do we need to change all the other places that use trunnel to actually solely use getters instead of direct struct access?

(The patch actually does make all the trunnel structures all over the codebase opaque. That's why it's so big.)

comment:11 Changed 2 years ago by nickm

Keywords: review-group-15 added

comment:12 Changed 2 years ago by dgoulet

@nickm, if you feel "uneasy" about this, let's drop it, I don't have a strong opinion on this. My only argument is really about trying to always use our getter/setter from the trunnel API and not poke the monster directly since this is generated code.

comment:13 Changed 2 years ago by nickm

Milestone: Tor: 0.3.0.x-finalTor: unspecified

Moving this to "unspecified" pending more discussion.

(Does it help if I mention that changing the struct members by hand was always meant to be explicitly allowed in the trunnel API? ;) )

comment:14 Changed 2 years ago by nickm

Keywords: review-group-15 removed

comment:15 Changed 22 months ago by nickm

Keywords: technical-debt refactor opaque data-hiding trunnel added

comment:16 Changed 6 months ago by dgoulet

Resolution: wontfix
Status: merge_readyclosed

Won't happen for now...

Note: See TracTickets for help on using tickets.