Opened 6 months ago

Closed 5 months ago

#29662 closed enhancement (implemented)

Introduce assert functions that allow us to printf error message

Reported by: rl1987 Owned by: rl1987
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: asn-merge, nickm-merge, teor-merge
Cc: nickm Actual Points:
Parent ID: Points:
Reviewer: ahf Sponsor:

Description


Child Tickets

Change History (15)

comment:1 Changed 6 months ago by rl1987

Owner: set to rl1987
Status: assignedaccepted

comment:2 Changed 6 months ago by rl1987

Status: acceptedneeds_review

comment:3 Changed 6 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.4.1.x-final

comment:4 Changed 6 months ago by teor

Component: Core TorCore Tor/Tor
Type: taskenhancement

This task wasn't in the Tor component, so it didn't get a reviewer this week.

comment:5 Changed 5 months ago by asn

Reviewer: ahf

comment:6 Changed 5 months ago by ahf

Status: needs_reviewmerge_ready

The code looks good and I like the idea. Let's see what one of the mergers says.

comment:7 Changed 5 months ago by teor

Cc: nickm added
Keywords: asn-merge nickm-merge teor-merge added
Status: merge_readyneeds_revision

I wonder why we're using static arrays rather than tor_asprintf()?
Are they long enough for printf-style assert messages?
Maybe nickm knows why they have to be static arrays.

Also, I found a grammar nitpick.

comment:8 Changed 5 months ago by rl1987

Status: needs_revisionneeds_review

comment:9 Changed 5 months ago by ahf

Status: needs_reviewmerge_ready

New commits looks good. Thanks teor for the extra review!

comment:10 Changed 5 months ago by nickm

Status: merge_readyneeds_revision

Can the "Non-fatal assertion %s failed" lines be reached with extra == NULL? I think it can, and I think that's a bug.

Shouldn't the tor_assertion_failed_ and tor_bug_occurred_ functions have CHECK_PRINTF annotations now?

comment:11 in reply to:  10 ; Changed 5 months ago by rl1987

Replying to nickm:

Can the "Non-fatal assertion %s failed" lines be reached with extra == NULL? I think it can, and I think that's a bug.

That preserves the old code behavior (for no expr and variable arguments, thus extra == NULL) and enables using the same functions for tor_assert{_nonfatal}() and the new tor_assertf{_nonfatal}() macros. Unless I'm missing something here?

Shouldn't the tor_assertion_failed_ and tor_bug_occurred_ functions have CHECK_PRINTF annotations now?

Fixed in 999137dcd7406812c99f496b231edaaca3fb7155.

comment:12 Changed 5 months ago by rl1987

Status: needs_revisionneeds_review

comment:13 in reply to:  11 Changed 5 months ago by rl1987

Replying to rl1987:

Replying to nickm:

Can the "Non-fatal assertion %s failed" lines be reached with extra == NULL? I think it can, and I think that's a bug.

That preserves the old code behavior (for no expr and variable arguments, thus extra == NULL) and enables using the same functions for tor_assert{_nonfatal}() and the new tor_assertf{_nonfatal}() macros. Unless I'm missing something here?

Fixed in 53192a6467771abddee08e7e8256f43d935037e9 after talking about this on IRC.

comment:14 Changed 5 months ago by nickm

I've done a squash, a merge, and a practracker fix as ticket29662_squashed_merged with a PR as https://github.com/torproject/tor/pull/870 . I'll merge it if the CI passes.

comment:15 Changed 5 months ago by nickm

Resolution: implemented
Status: needs_reviewclosed

CI passed; merged to master!

Note: See TracTickets for help on using tickets.