Opened 4 months ago

Closed 3 months ago

#31080 closed defect (fixed)

Stop using the same value for LD_NO_MOCK and LD_MESG

Reported by: teor Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version: Tor: 0.4.1.1-alpha
Severity: Normal Keywords: 041-must 041-regression dgoulet-merge
Cc: gaba Actual Points: .1
Parent ID: Points: 0.5
Reviewer: asn Sponsor: Sponsor31-must

Description

LD_NO_MOCK and LD_MESG have the same value, which is a bug in 0.4.1.1-alpha and later.

We'll need to make log_domain_mask_t into a u64, and change the values of the domain flags so they go down from 63 (not 31).

We'll also need to change the corresponding type in Rust to u64. Maybe we should define a type, rather than using u64 everywhere.

This is a bug on #28226, which is Sponsor 31 must, so I'm marking this ticket as sponsor 31 must.

Child Tickets

Change History (11)

comment:1 Changed 3 months ago by nickm

Keywords: 041-regression added

comment:2 Changed 3 months ago by nickm

Owner: set to nickm
Status: newaccepted

comment:3 Changed 3 months ago by nickm

Actual Points: .1
Status: acceptedneeds_review

See my branch bug31080_041. It renumbers the domain, makes the mask 64 bits wide, and adds a compile-time assertion to prevent recurrences of the bug.

PR at https://github.com/torproject/tor/pull/1171

comment:4 in reply to:  description Changed 3 months ago by teor

I did a quick review on this patch, I had some questions about using u64 some places, and log_domain_t in others.

Replying to teor:

We'll also need to change the corresponding type in Rust to u64. Maybe we should define a type, rather than using u64 everywhere.

This patch does not fix the Rust type, and it needs to. See the tor_log Rust module.

comment:5 Changed 3 months ago by teor

Status: needs_reviewneeds_revision

comment:6 Changed 3 months ago by nickm

Status: needs_revisionneeds_review

Branch updated with two commits: one to fix rust, and one to move log_domain_t to a lower level header. Better now?

comment:7 Changed 3 months ago by dgoulet

Reviewer: asn

comment:8 Changed 3 months ago by asn

Status: needs_reviewneeds_revision

Latest patch looks good to me, however travis with clang fails with two compilation errors. After those are resolved, we can mark this as merge_ready (since I will likely be on leave by then).

comment:9 Changed 3 months ago by nickm

Okay, I've reproduced them and added a commit to fix them. Once travis passes I'll mark the branch merge_ready.

comment:10 Changed 3 months ago by nickm

Keywords: dgoulet-merge added
Status: needs_revisionmerge_ready

comment:11 Changed 3 months ago by dgoulet

Resolution: fixed
Status: merge_readyclosed

Merged 041 and master!

Note: See TracTickets for help on using tickets.