Opened 7 weeks ago

Closed 6 weeks ago

#31532 closed defect (fixed)

Use ptrdiff_t for struct_member_t.offset, etc

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version:
Severity: Trivial Keywords: network-team-roadmap-august, asn-merge, dgoulet-merge
Cc: nickm, teor, gaba Actual Points:
Parent ID: #29211 Points:
Reviewer: Sponsor: Sponsor31-can

Description

We have several fields in our configuration code that use int for a struct offset:

  • struct_member_t.offset
  • struct_magic_decl_t.magic_offset
  • config_format_t.config_suite_offset

These should all use ptrdiff_t instead.

Child Tickets

Change History (8)

comment:1 Changed 7 weeks ago by teor

Owner: set to nickm
Status: newassigned

comment:2 Changed 6 weeks ago by nickm

We should also use ptrdiff_t for our smartlist_pqueue_* functions.

comment:3 Changed 6 weeks ago by nickm

Also we should audit our use of off_t: some of them should be ptrdiff_t.

comment:4 Changed 6 weeks ago by nickm

I have made a branch to clean up all the places I could find in our code that should be using ptrdiff_t. I made it by first changing the three points listed in the description of this ticket, and then by grepping for off_t, offsetof, and STRUCT_VAR_P.

Please let me know if you think this should be split into separate PRs or tickets.

The branch is ticket31532; the PR is https://github.com/torproject/tor/pull/1291 . I'll put this in needs_review once CI passes.

comment:5 Changed 6 weeks ago by nickm

Status: assignedneeds_review

(The original version of this branch failed to compile with clang. I force-pushed an updated version, which passed CI.)

comment:6 Changed 6 weeks ago by teor

Status: needs_reviewmerge_ready

Looks fine to me. I also did a quick check of the remaining off_t instances in the code: they all correspond to files.

comment:7 Changed 6 weeks ago by teor

Keywords: asn-merge dgoulet-merge added

There might be some minor merge conflicts with these tickets, feel free to put back in needs_revision, or ask nickm how to fix the conflict.

comment:8 Changed 6 weeks ago by asn

Resolution: fixed
Status: merge_readyclosed

Merged! No conflicts were seen.

Note: See TracTickets for help on using tickets.