#20980 closed task (fixed)

Use the standard OpenBSD preprocessor definition

Reported by: cypherpunks Owned by:
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords:
Cc: teor Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

A few places uses OPENBSD instead of the typical __OpenBSD__.

Child Tickets

Attachments (1)

0001-Use-the-standard-OpenBSD-preprocessor-definition.patch (2.6 KB) - added by cypherpunks 11 months ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 11 months ago by cypherpunks

Status: newneeds_review

comment:2 Changed 11 months ago by nickm

Milestone: Tor: 0.3.0.x-final

comment:3 Changed 11 months ago by teor

Status: needs_reviewneeds_information

Is __OpenBSD__ defined for bitrig and other OpenBSD variants?

If so, this patch is fine, flip it into merge-ready.
If not, we need to rethink how we make this change.

comment:4 in reply to:  3 Changed 11 months ago by cypherpunks

Replying to teor:

Is __OpenBSD__ defined for bitrig and other OpenBSD variants?

For context, Bitrig support was added in #6982 (commit d92d3f33356af002892ba5754d9d36cc4504c95f).

The Pre-defined Compiler Macros wiki contains a bunch of operating systems but not Bitrig. Issue 72 on the Bitrig bug tracker was about getting Bitrig added to this list.

The param.h header typically contains these preprocessor definitions. That header file in the Bitrig repository contains no __OpenBSD__. The current patch would therefore remove support for Bitrig. It does contain OpenBSD which seems to exists for other forks (like LibertyBSD) also.

If so, this patch is fine, flip it into merge-ready.

IMO the patch is fine, because Bitrig isn't explicitly supported and other parts of the code already uses __OpenBSD__ without caring about Bitrig (a quick grep shows 10 lines using __OpenBSD__ and 4 lines using OPENBSD).

If not, we need to rethink how we make this change.

If we really care about Bitrig and other OpenBSD forks, the patch could be fixed by replacing OPENBSD with OpenBSD but -1 from me on that.

I'll leave it to teor for the final word on this.

comment:5 Changed 10 months ago by nickm

Type: defecttask

comment:6 Changed 10 months ago by dgoulet

Cc: teor added
Status: needs_informationneeds_review

Back in needs_review and there is an action item for teor.

comment:7 Changed 10 months ago by teor

Status: needs_reviewneeds_revision

Given cypherpunks' comment, I think we should switch all uses of OpenBSD and OPENBSD to OpenBSD instead. (Unless there's some reason we only want those OSs defining one of the other macros.)

Ideally, we should avoid using these macros at all, and check for features with configure instead.

comment:8 Changed 10 months ago by dgoulet

Status: needs_revisionneeds_review

See branch bug20980_030_01.

I followed teor's latest recommendation here using the original patch.

comment:9 Changed 10 months ago by nickm

Resolution: fixed
Status: needs_reviewclosed

ok, merging that!

Note: See TracTickets for help on using tickets.