Opened 2 months ago

Closed 2 months ago

#32495 closed defect (fixed)

relay_config.h: avoid redundant typedef for or_options_t

Reported by: nickm Owned by: nickm
Priority: High Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-ci-failure
Cc: teor Actual Points: 0.1
Parent ID: Points:
Reviewer: catalyst Sponsor: Sponsor31-can

Description

Freebsd cc doesn't like us re-declaring the or_options_t typedef in relay_config.h: we need to use the struct declaration instead.

Child Tickets

Change History (12)

comment:1 Changed 2 months ago by nickm

Summary: relay_config.c: avoid redundant typedef for or_options_trelay_config.h: avoid redundant typedef for or_options_t

comment:2 Changed 2 months ago by nickm

Cc: teor added

Patch in bug32495; bug not in any Tor release. PR at https://github.com/torproject/tor/pull/1536

comment:3 Changed 2 months ago by nickm

Status: assignedneeds_review

comment:4 Changed 2 months ago by catalyst

Reviewer: catalyst

comment:5 Changed 2 months ago by catalyst

A quick grep shows some additional possible duplicates:

../src/core/or/or.h:typedef struct or_options_t or_options_t;
../src/feature/dirauth/dirauth_config.h:typedef struct or_options_t or_options_t;
../src/feature/relay/relay_config.h:typedef struct or_options_t or_options_t;
../src/feature/relay/transport_config.h:typedef struct or_options_t or_options_t;

Do we know why relay_config.h is the only one that causes problems?

comment:6 Changed 2 months ago by nickm

I'm afraid I don't; it might have to do with whether the typedef is seen after the definition of struct or_options_t or not.

comment:7 Changed 2 months ago by catalyst

Status: needs_reviewneeds_revision

The Jenkins log at https://jenkins.torproject.org/job/tor-ci-freebsd-amd64-master/388/consoleFull does show the other headers causing problems with redefining or_options_t.

Also from the Jenkins log, I also see redefinitions of smartlist_t that occur in relay_config.h, transport_config.h, but not dirauth_config.h.

For or_options_t, I see several possible options:

  • Leave the or_options_t typedef in or.h, but require that inclusion of or.h precede any other headers that require that typedef. That's not great from a standpoint of modularizing our headers.
  • Remove the or_options_t typedef from or.h, and define it in a shared header file.
  • Leave the or_options_t typedef in or.h, but remove all references to the typedef in header files (using the struct tag instead).

Other possibilities might exist. What do you think?

For the smartlist_t redefinitions, maybe we can explicitly include smartlist.h in those headers? Is there a reason not to?

Also, it's probably worth noting that when I tried write minimal test cases to produce this warning, I was only able to do so on clang, and even then only if using options such as -std=c99. So maybe FreeBSD adjusts some of the warning behaviors. Note this means that Travis CI probably won't detect future instances of this problem.

comment:8 Changed 2 months ago by nickm

Status: needs_revisionneeds_review

On IRC, catalyst helped me figure out how to reproduce this: using --std=gnu99.

The configure line that worked for me was: ./configure --enable-fatal-warnings CFLAGS='-std=gnu99 -O2' CC=clang.

It does seem that the other redundant typedefs were also causing warnings. I've force-pushed a better version of this fix.

comment:9 Changed 2 months ago by nickm

Actual Points: 00.1

comment:10 Changed 2 months ago by catalyst

Created #32500 for possible CI improvement to avoid recurrence.

comment:11 in reply to:  8 Changed 2 months ago by catalyst

Status: needs_reviewmerge_ready

Replying to nickm:

It does seem that the other redundant typedefs were also causing warnings. I've force-pushed a better version of this fix.

Thanks! Looks good.

comment:12 Changed 2 months ago by teor

Resolution: fixed
Status: merge_readyclosed

CI passes, and it also passes with -std=gnu99 in #32500.
Merged to master.

Note: See TracTickets for help on using tickets.