Opened 5 months ago

Closed 4 months ago

#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 5 months ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 5 months ago by cypherpunks

  • Status changed from new to needs_review

comment:2 Changed 5 months ago by nickm

  • Milestone set to Tor: 0.3.0.x-final

comment:3 follow-up: Changed 5 months ago by teor

  • Status changed from needs_review to needs_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 5 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 4 months ago by nickm

  • Type changed from defect to task

comment:6 Changed 4 months ago by dgoulet

  • Cc teor added
  • Status changed from needs_information to needs_review

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

comment:7 Changed 4 months ago by teor

  • Status changed from needs_review to needs_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 4 months ago by dgoulet

  • Status changed from needs_revision to needs_review

See branch bug20980_030_01.

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

comment:9 Changed 4 months ago by nickm

  • Resolution set to fixed
  • Status changed from needs_review to closed

ok, merging that!

Note: See TracTickets for help on using tickets.