Opened 2 months ago

Closed 2 weeks ago

#31334 closed defect (fixed)

Use SEVERITY_MASK_IDX() to find the LOG_ERR index in the unit tests

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version: Tor: 0.2.5.2-alpha
Severity: Normal Keywords: 042-can asn-merge
Cc: Actual Points: 0.1
Parent ID: Points: 0.1
Reviewer: nickm Sponsor:

Description

Part of #30901, just needed a bug number.

Child Tickets

Change History (10)

comment:1 Changed 2 months ago by teor

Resolution: fixed
Status: assignedclosed

We'll deal with this in #30901.

comment:2 Changed 2 months ago by teor

Version: Tor: 0.2.5.2-alpha

comment:3 Changed 3 weeks ago by teor

Cc: nickm catalyst teor removed
Keywords: BugSmashFund added; network-team-roadmap-august refactor removed
Parent ID: #30901
Resolution: fixed
Sponsor: Sponsor31-can
Status: closedreopened

Let's do this separately.

See my PR:

comment:4 Changed 3 weeks ago by teor

Status: reopenedneeds_review

comment:5 Changed 3 weeks ago by nickm

Keywords: BugSmashFund removed

(I don't think this is a BugSmashFund candidate, since it changes no user-visible behavior.)

comment:6 Changed 3 weeks ago by nickm

Keywords: 042-can added

comment:7 Changed 3 weeks ago by nickm

Reviewer: nickm
Status: needs_reviewneeds_revision

Looks good, but should we also fix these instances?

% git grep 'masks' src/test
src/test/fuzz/fuzzing_common.c:    s.masks[LOG_WARN-LOG_ERR] |= LD_BUG;
src/test/test_logging.c:  no_bug.masks[0] &= ~(LD_BUG|LD_GENERAL);
src/test/test_options.c:  lst.masks[LOG_ERR - LOG_ERR] = ~0;
src/test/test_options.c:  lst.masks[LOG_WARN - LOG_ERR] = ~0;
src/test/test_options.c:  lst.masks[LOG_NOTICE - LOG_ERR] = ~0;
src/test/testing_common.c:    s.masks[LOG_WARN-LOG_ERR] |= LD_BUG;
src/test/testing_common.c:    s.masks[LOG_WARN-LOG_ERR] |= LD_BUG;

comment:8 Changed 3 weeks ago by teor

Status: needs_revisionneeds_review

Oops yes my original branch was just focused on test_logging.c.

I fixed all the instances, split into two commits (code movement and fixes), and force-pushed.

comment:9 Changed 3 weeks ago by nickm

Keywords: asn-merge added
Status: needs_reviewmerge_ready

LGTM. I say "no backport" since this is a readability issue.

I also noticed that we are still using ~0 here, when the type has become 64 bits. I opened #31854 for that.

comment:10 Changed 2 weeks ago by asn

Resolution: fixed
Status: merge_readyclosed

Merged!

Note: See TracTickets for help on using tickets.